You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "PokIsemaine (via GitHub)" <gi...@apache.org> on 2024/06/09 08:03:12 UTC
[PR] feat: git hook pre-push [kvrocks]
PokIsemaine opened a new pull request, #2359:
URL: https://github.com/apache/kvrocks/pull/2359
fix: https://github.com/apache/kvrocks/issues/2301
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "PokIsemaine (via GitHub)" <gi...@apache.org>.
PokIsemaine commented on code in PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#discussion_r1633144259
##########
x.py:
##########
Review Comment:
Designed the `config-git-hooks` subcommand to support `enable`, `disable`, and `list` of git hooks.
```
ubuntu% ./x.py config-git-hooks -h
usage: x.py config-git-hooks [-h] {enable,disable,list} ...
Config git hooks
positional arguments:
{enable,disable,list}
enable enable git hooks in utils/git-hooks
disable disable git hooks in .git/hooks
list list status of git hooks in utils/git-hooks
options:
-h, --help show this help message and exit
```
The following are examples of continuous operations:
```
ubuntu% ./x.py config-git-hooks enable
commit-msg installed at .git/hooks/commit-msg
pre-push installed at .git/hooks/pre-push
ubuntu% ./x.py config-git-hooks list
commit-msg enabled
pre-push enabled
```
```
ubuntu% ./x.py config-git-hooks disable
.git/hooks/commit-msg disabled
.git/hooks/pre-push disabled
ubuntu% ./x.py config-git-hooks list
commit-msg disabled
pre-push disabled
```
```
ubuntu% ./x.py config-git-hooks enable --target_hook_name=commit-msg
commit-msg installed at .git/hooks/commit-msg
ubuntu% ./x.py config-git-hooks list
commit-msg enabled
pre-push disabled
```
```
ubuntu% ./x.py config-git-hooks disable --target_hook_name=commit-msg
.git/hooks/commit-msg disabled
ubuntu% ./x.py config-git-hooks list
commit-msg disabled
pre-push disabled
```
```
ubuntu% ./x.py config-git-hooks disable --target_hook_name=xxx
ubuntu% ./x.py config-git-hooks enable --target_hook_name=xxx
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#discussion_r1639175924
##########
x.py:
##########
@@ -92,6 +93,20 @@ def check_version(current: str, required: Tuple[int, int, int], prog_name: Optio
return semver
+def prepare() -> None:
+ basedir = Path(__file__).parent.absolute()
+ # install git hooks
+ git_hooks_dir = basedir / ".git/hooks/"
+ kvrocks_git_hooks_dir = basedir / "utils/git-hooks/"
+ os.makedirs(git_hooks_dir, exist_ok=True)
+ for hook in os.listdir(kvrocks_git_hooks_dir):
+ dst = os.path.join(git_hooks_dir, hook.split('.')[0])
+ hook_path = os.path.join(kvrocks_git_hooks_dir, hook)
+ if os.path.exists(dst):
+ os.remove(dst)
Review Comment:
We can :
- just abort to let user delete it manually
- ask user for confirming
- move user's file to another name and send a warning
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#issuecomment-2167501463
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2359) **Quality Gate passed**
Issues
 [19 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2359&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2359&resolutions=WONTFIX)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2359&resolved=false&sinceLeakPeriod=true)
 [55.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2359&metric=new_coverage&view=list)
 [1.1% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2359&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2359)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "PokIsemaine (via GitHub)" <gi...@apache.org>.
PokIsemaine commented on code in PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#discussion_r1639178853
##########
x.py:
##########
@@ -92,6 +93,20 @@ def check_version(current: str, required: Tuple[int, int, int], prog_name: Optio
return semver
+def prepare() -> None:
+ basedir = Path(__file__).parent.absolute()
+ # install git hooks
+ git_hooks_dir = basedir / ".git/hooks/"
+ kvrocks_git_hooks_dir = basedir / "utils/git-hooks/"
+ os.makedirs(git_hooks_dir, exist_ok=True)
+ for hook in os.listdir(kvrocks_git_hooks_dir):
+ dst = os.path.join(git_hooks_dir, hook.split('.')[0])
+ hook_path = os.path.join(kvrocks_git_hooks_dir, hook)
+ if os.path.exists(dst):
+ os.remove(dst)
Review Comment:
> One of these methods:
>
> * just abort to let user delete it manually
> * ask user for confirming
> * move user's file to another name and send a warning
I used the second one in my new commit. Please help me review it again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "PokIsemaine (via GitHub)" <gi...@apache.org>.
PokIsemaine commented on code in PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#discussion_r1639174400
##########
x.py:
##########
@@ -92,6 +93,20 @@ def check_version(current: str, required: Tuple[int, int, int], prog_name: Optio
return semver
+def prepare() -> None:
+ basedir = Path(__file__).parent.absolute()
+ # install git hooks
+ git_hooks_dir = basedir / ".git/hooks/"
+ kvrocks_git_hooks_dir = basedir / "utils/git-hooks/"
+ os.makedirs(git_hooks_dir, exist_ok=True)
+ for hook in os.listdir(kvrocks_git_hooks_dir):
+ dst = os.path.join(git_hooks_dir, hook.split('.')[0])
+ hook_path = os.path.join(kvrocks_git_hooks_dir, hook)
+ if os.path.exists(dst):
+ os.remove(dst)
Review Comment:
Thanks for the suggestion, I tried adding an interactive query.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#discussion_r1638207030
##########
x.py:
##########
@@ -92,6 +93,20 @@ def check_version(current: str, required: Tuple[int, int, int], prog_name: Optio
return semver
+def prepare() -> None:
+ basedir = Path(__file__).parent.absolute()
+ # install git hooks
+ git_hooks_dir = basedir / ".git/hooks/"
+ kvrocks_git_hooks_dir = basedir / "utils/git-hooks/"
+ os.makedirs(git_hooks_dir, exist_ok=True)
+ for hook in os.listdir(kvrocks_git_hooks_dir):
+ dst = os.path.join(git_hooks_dir, hook.split('.')[0])
+ hook_path = os.path.join(kvrocks_git_hooks_dir, hook)
+ if os.path.exists(dst):
+ os.remove(dst)
+ os.symlink(hook_path, dst)
+ os.chmod(dst, os.stat(dst).st_mode | stat.S_IEXEC)
+ print(hook.split('.')[0], "installed at", os.path.abspath(dst))
Review Comment:
This patch is good to merge, while:
1. We can keep use `pathlib` instead of sometimes `os.path.join`.
2. We can chmod for the shell script checked in directly.
3. We can name the shell script as `pre-push` directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#discussion_r1632220363
##########
x.py:
##########
@@ -122,6 +130,9 @@ def build(dir: str, jobs: Optional[int], ghproxy: bool, ninja: bool, unittest: b
run(cmake, str(basedir), *cmake_options, verbose=True, cwd=dir)
+ os.makedirs("./git/hooks", exist_ok=True)
Review Comment:
Maybe move it into `enable_git_hook`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#discussion_r1638209159
##########
x.py:
##########
@@ -92,6 +93,20 @@ def check_version(current: str, required: Tuple[int, int, int], prog_name: Optio
return semver
+def prepare() -> None:
+ basedir = Path(__file__).parent.absolute()
+ # install git hooks
+ git_hooks_dir = basedir / ".git/hooks/"
+ kvrocks_git_hooks_dir = basedir / "utils/git-hooks/"
+ os.makedirs(git_hooks_dir, exist_ok=True)
+ for hook in os.listdir(kvrocks_git_hooks_dir):
+ dst = os.path.join(git_hooks_dir, hook.split('.')[0])
+ hook_path = os.path.join(kvrocks_git_hooks_dir, hook)
+ if os.path.exists(dst):
+ os.remove(dst)
Review Comment:
If we remove files that may not owned by us, I suppose we at least print a warning, or let users to delete it manually.
Silently delete user-owned files is not a good idea.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "PokIsemaine (via GitHub)" <gi...@apache.org>.
PokIsemaine commented on code in PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#discussion_r1639178853
##########
x.py:
##########
@@ -92,6 +93,20 @@ def check_version(current: str, required: Tuple[int, int, int], prog_name: Optio
return semver
+def prepare() -> None:
+ basedir = Path(__file__).parent.absolute()
+ # install git hooks
+ git_hooks_dir = basedir / ".git/hooks/"
+ kvrocks_git_hooks_dir = basedir / "utils/git-hooks/"
+ os.makedirs(git_hooks_dir, exist_ok=True)
+ for hook in os.listdir(kvrocks_git_hooks_dir):
+ dst = os.path.join(git_hooks_dir, hook.split('.')[0])
+ hook_path = os.path.join(kvrocks_git_hooks_dir, hook)
+ if os.path.exists(dst):
+ os.remove(dst)
Review Comment:
> One of these methods:
>
> * just abort to let user delete it manually
> * ask user for confirming
> * move user's file to another name and send a warning
I used the second one in my new commit. Please help me review it again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "PokIsemaine (via GitHub)" <gi...@apache.org>.
PokIsemaine commented on PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#issuecomment-2158353708
> `add-git-hooks` sounds wordy and too specific to use. Follwoing npm's practice that uses git hook heavily, we can add a `prepare` target in x.py. No need for disable and list, that looks strange .. .
How to trigger `prepare`? Triggered as a separate subcommand or when `x.py build`?
In addition to git hooks, will `prepare` have other actions in the future?
I'm okay with both, @PragmaTwice do you have any suggestions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#discussion_r1632220486
##########
x.py:
##########
Review Comment:
Maybe we can add a new subcommand for it, like `./x.py add-git-hook`
##########
x.py:
##########
Review Comment:
Maybe we can add a new subcommand for it, like `./x.py add-git-hooks`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#discussion_r1639175924
##########
x.py:
##########
@@ -92,6 +93,20 @@ def check_version(current: str, required: Tuple[int, int, int], prog_name: Optio
return semver
+def prepare() -> None:
+ basedir = Path(__file__).parent.absolute()
+ # install git hooks
+ git_hooks_dir = basedir / ".git/hooks/"
+ kvrocks_git_hooks_dir = basedir / "utils/git-hooks/"
+ os.makedirs(git_hooks_dir, exist_ok=True)
+ for hook in os.listdir(kvrocks_git_hooks_dir):
+ dst = os.path.join(git_hooks_dir, hook.split('.')[0])
+ hook_path = os.path.join(kvrocks_git_hooks_dir, hook)
+ if os.path.exists(dst):
+ os.remove(dst)
Review Comment:
We can :
- just abort to let user delete it manually
- ask user for confirming
- move user's file to another name
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#discussion_r1636665351
##########
x.py:
##########
@@ -92,6 +93,19 @@ def check_version(current: str, required: Tuple[int, int, int], prog_name: Optio
return semver
+GIT_HOOKS_DIR = ".git/hooks/"
+KVROCKS_GIT_HOOKS_DIR = "./utils/git-hooks/"
+def prepare() -> None:
+ # install git hooks
+ os.makedirs(GIT_HOOKS_DIR, exist_ok=True)
+ for hook in os.listdir(KVROCKS_GIT_HOOKS_DIR):
+ hook_path = os.path.join(KVROCKS_GIT_HOOKS_DIR, hook)
+ dst = os.path.join(GIT_HOOKS_DIR, hook.split('.')[0])
+ if os.path.exists(dst):
+ os.remove(dst)
+ os.link(hook_path, dst)
Review Comment:
```suggestion
os.symlink(hook_path, dst)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#issuecomment-2167575630
Thanks for your contribution @PokIsemaine!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun merged PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#discussion_r1632219941
##########
pre-push.sh:
##########
Review Comment:
Could you move this file to `util/git-hooks`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#issuecomment-2158245280
`add-git-hooks` sounds wordy and too specific to use. Follwoing npm's practice that uses git hook heavily, we can add a `prepare` target in x.py. No need for disable and list, that looks strange ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#issuecomment-2158362494
Sounds good to me. We can just have a `./x.py prepare`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: git hook pre-push [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2359:
URL: https://github.com/apache/kvrocks/pull/2359#discussion_r1639175924
##########
x.py:
##########
@@ -92,6 +93,20 @@ def check_version(current: str, required: Tuple[int, int, int], prog_name: Optio
return semver
+def prepare() -> None:
+ basedir = Path(__file__).parent.absolute()
+ # install git hooks
+ git_hooks_dir = basedir / ".git/hooks/"
+ kvrocks_git_hooks_dir = basedir / "utils/git-hooks/"
+ os.makedirs(git_hooks_dir, exist_ok=True)
+ for hook in os.listdir(kvrocks_git_hooks_dir):
+ dst = os.path.join(git_hooks_dir, hook.split('.')[0])
+ hook_path = os.path.join(kvrocks_git_hooks_dir, hook)
+ if os.path.exists(dst):
+ os.remove(dst)
Review Comment:
One of these methods:
- just abort to let user delete it manually
- ask user for confirming
- move user's file to another name and send a warning
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org