| Message ID | 20200909004939.1942347-9-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | propose config-based hooks | expand |
Hi Emily On 09/09/2020 01:49, Emily Shaffer wrote: > As part of the adoption of config-based hooks, teach run_commit_hook() > to call hook.h instead of run-command.h. This covers 'pre-commit', > 'commit-msg', and 'prepare-commit-msg'. Additionally, ask the hook > library - not run-command - whether any hooks will be run, as it's > possible hooks may exist in the config but not the hookdir. > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > builtin/commit.c | 3 ++- > builtin/merge.c | 3 ++- > commit.c | 13 ++++++++++++- > t/t7503-pre-commit-and-pre-merge-commit-hooks.sh | 13 +++++++++++++ > 4 files changed, 29 insertions(+), 3 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index 69ac78d5e5..a19c6478eb 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -36,6 +36,7 @@ > #include "help.h" > #include "commit-reach.h" > #include "commit-graph.h" > +#include "hook.h" > > static const char * const builtin_commit_usage[] = { > N_("git commit [<options>] [--] <pathspec>..."), > @@ -985,7 +986,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > return 0; > } > > - if (!no_verify && find_hook("pre-commit")) { > + if (!no_verify && hook_exists("pre-commit")) { > /* > * Re-read the index as pre-commit hook could have updated it, > * and write it out as a tree. We must do this before we invoke > diff --git a/builtin/merge.c b/builtin/merge.c > index 74829a838e..c1a9d0083d 100644 > --- a/builtin/merge.c > +++ b/builtin/merge.c > @@ -41,6 +41,7 @@ > #include "commit-reach.h" > #include "wt-status.h" > #include "commit-graph.h" > +#include "hook.h" > > #define DEFAULT_TWOHEAD (1<<0) > #define DEFAULT_OCTOPUS (1<<1) > @@ -829,7 +830,7 @@ static void prepare_to_commit(struct commit_list *remoteheads) > * and write it out as a tree. We must do this before we invoke > * the editor and after we invoke run_status above. > */ > - if (find_hook("pre-merge-commit")) > + if (hook_exists("pre-merge-commit")) > discard_cache(); > read_cache_from(index_file); > strbuf_addbuf(&msg, &merge_msg); > diff --git a/commit.c b/commit.c > index 4ce8cb38d5..c7a243e848 100644 > --- a/commit.c > +++ b/commit.c > @@ -21,6 +21,7 @@ > #include "commit-reach.h" > #include "run-command.h" > #include "shallow.h" > +#include "hook.h" > > static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **); > > @@ -1632,8 +1633,13 @@ int run_commit_hook(int editor_is_used, const char *index_file, > { > struct strvec hook_env = STRVEC_INIT; > va_list args; > + const char *arg; > + struct strvec hook_args = STRVEC_INIT; > + struct strbuf hook_name = STRBUF_INIT; > int ret; > > + strbuf_addstr(&hook_name, name); Seeing this makes me wonder if it would be better for run_hooks() to take a string for the name rather than an strbuf, I suspect that virtually all callers have a fixed hook name. Best Wishes Phillip > strvec_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file); > > /* > @@ -1643,9 +1649,14 @@ int run_commit_hook(int editor_is_used, const char *index_file, > strvec_push(&hook_env, "GIT_EDITOR=:"); > > va_start(args, name); > - ret = run_hook_ve(hook_env.v, name, args); > + while ((arg = va_arg(args, const char *))) > + strvec_push(&hook_args, arg); > va_end(args); > + > + ret = run_hooks(hook_env.v, &hook_name, &hook_args); > strvec_clear(&hook_env); > + strvec_clear(&hook_args); > + strbuf_release(&hook_name); > > return ret; > } > diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh > index b3485450a2..cef8085dcc 100755 > --- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh > +++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh > @@ -103,6 +103,19 @@ test_expect_success 'with succeeding hook' ' > test_cmp expected_hooks actual_hooks > ' > > +# NEEDSWORK: when 'git hook add' and 'git hook remove' have been added, use that > +# instead > +test_expect_success 'with succeeding hook (config-based)' ' > + test_when_finished "git config --unset hook.pre-commit.command success.sample" && > + test_when_finished "rm -f expected_hooks actual_hooks" && > + git config hook.pre-commit.command "$HOOKDIR/success.sample" && > + echo "$HOOKDIR/success.sample" >expected_hooks && > + echo "more" >>file && > + git add file && > + git commit -m "more" && > + test_cmp expected_hooks actual_hooks > +' > + > test_expect_success 'with succeeding hook (merge)' ' > test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" && > cp "$HOOKDIR/success.sample" "$PREMERGE" && >
Phillip Wood <phillip.wood123@gmail.com> writes: >> + const char *arg; >> + struct strvec hook_args = STRVEC_INIT; >> + struct strbuf hook_name = STRBUF_INIT; >> int ret; >> + strbuf_addstr(&hook_name, name); > > Seeing this makes me wonder if it would be better for run_hooks() to > take a string for the name rather than an strbuf, I suspect that > virtually all callers have a fixed hook name. Yeah, that is a good point. It is always a good discipline to keep the type of the parameters callers need to pass to the minimum.
> - if (!no_verify && find_hook("pre-commit")) { > + if (!no_verify && hook_exists("pre-commit")) { A reviewer would probably need to look at all instances of "pre-commit" (and likewise for the other hooks) but if the plan is to convert all hooks, then the reviewer wouldn't need to do this since we could just delete the "find_hook" function. Overall comments about the design and scope of the patch set: - I think that the abilities of the current patch set regarding overriding order of globally-set hook commands is sufficient. We should also have some way of disabling globally-set hooks, perhaps by implementing the "skip" variable mentioned in patch 1 or by allowing the redefinition of hookcmd sections (e.g. by redefining a command to "/usr/bin/true"). To me, these provide substantial user-facing value, and would be sufficient for a first version - and other things like parallelization can come later. - As for the UI that should be exposed through the "git hook" command, I think that "git hook list" and "git hook run" are sufficient. Editing the config files are not too difficult, and "git hook add" etc. can be added later. - As for whether (1) it is OK for none of the hooks to be converted (and instead rely on the user to edit their hook scripts to call "git hook run ???"), or if (2) we should require some hooks to be converted, or if (3) we should require all hooks to be converted: I'd rather have (2) or (3) so that we don't have dead code. I prefer (3), especially since a reviewer wouldn't have to worry about leftover usages of old functions like find_hook() (as I mentioned at the start of this email), but I'm not fully opposed to (2) either.
On Wed, Sep 23, 2020 at 04:47:34PM -0700, Jonathan Tan wrote: > > > - if (!no_verify && find_hook("pre-commit")) { > > + if (!no_verify && hook_exists("pre-commit")) { > > A reviewer would probably need to look at all instances of "pre-commit" > (and likewise for the other hooks) but if the plan is to convert all > hooks, then the reviewer wouldn't need to do this since we could just > delete the "find_hook" function. > > Overall comments about the design and scope of the patch set: > > - I think that the abilities of the current patch set regarding > overriding order of globally-set hook commands is sufficient. We > should also have some way of disabling globally-set hooks, perhaps > by implementing the "skip" variable mentioned in patch 1 or by > allowing the redefinition of hookcmd sections (e.g. by redefining a > command to "/usr/bin/true"). To me, these provide substantial > user-facing value, and would be sufficient for a first version - and > other things like parallelization can come later. OK. I will send 'skip' in the next reroll. Thanks for pointing it out! > > - As for the UI that should be exposed through the "git hook" command, > I think that "git hook list" and "git hook run" are sufficient. > Editing the config files are not too difficult, and "git hook add" > etc. can be added later. > > - As for whether (1) it is OK for none of the hooks to be converted (and > instead rely on the user to edit their hook scripts to call "git hook > run ???"), or if (2) we should require some hooks to be > converted, or if (3) we should require all hooks to be converted: I'd > rather have (2) or (3) so that we don't have dead code. I prefer (3), > especially since a reviewer wouldn't have to worry about leftover > usages of old functions like find_hook() (as I mentioned at the start > of this email), but I'm not fully opposed to (2) either. I personally prefer (3) - I think the user experience with (2) in a release (or even in 'next', which all Googlers use) is pretty bad. The downside, of course, is that a large topic gets merged all at once and makes some pretty nasty reviewer overhead. Junio, I wonder if you can give any advice here? What would be really ideal for me would be to do something like Stolee has been doing with his maintenance series - config-based hooks pt. I containing the library code and config-based hooks pt. II containing the conversion of preexisting hooks. Does that make the overhead for you significantly worse? - Emily
Emily Shaffer wrote: > On Wed, Sep 23, 2020 at 04:47:34PM -0700, Jonathan Tan wrote: >> - As for whether (1) it is OK for none of the hooks to be converted (and >> instead rely on the user to edit their hook scripts to call "git hook >> run ???"), or if (2) we should require some hooks to be >> converted, or if (3) we should require all hooks to be converted: I'd >> rather have (2) or (3) so that we don't have dead code. I prefer (3), >> especially since a reviewer wouldn't have to worry about leftover >> usages of old functions like find_hook() (as I mentioned at the start >> of this email), but I'm not fully opposed to (2) either. > > I personally prefer (3) - I think the user experience with (2) in a > release (or even in 'next', which all Googlers use) is pretty bad. The > downside, of course, is that a large topic gets merged all at once and > makes some pretty nasty reviewer overhead. One approach is to build up a series with "git hook run" and "git hook list" demonstrating and testing the functionality and [PATCH n+1/n] extra patches at the end converting existing hooks. The user experience from "git hook run" and even "git hook list" supporting a preview of the future without built-in commands living in that future yet would not be so bad, methinks. And then a final series could update the built-in commands' usage of hooks and would still be fairly small. In other words, I think I like (1), except *without* the recommendation for users to edit their hook scripts to call "git hook run" --- instead, the recommendation would be "try running this command if you want to see what hooks will do in the future". Thanks, Jonathan
On Mon, Oct 05, 2020 at 04:48:39PM -0700, Jonathan Nieder wrote: > > Emily Shaffer wrote: > > On Wed, Sep 23, 2020 at 04:47:34PM -0700, Jonathan Tan wrote: > > >> - As for whether (1) it is OK for none of the hooks to be converted (and > >> instead rely on the user to edit their hook scripts to call "git hook > >> run ???"), or if (2) we should require some hooks to be > >> converted, or if (3) we should require all hooks to be converted: I'd > >> rather have (2) or (3) so that we don't have dead code. I prefer (3), > >> especially since a reviewer wouldn't have to worry about leftover > >> usages of old functions like find_hook() (as I mentioned at the start > >> of this email), but I'm not fully opposed to (2) either. > > > > I personally prefer (3) - I think the user experience with (2) in a > > release (or even in 'next', which all Googlers use) is pretty bad. The > > downside, of course, is that a large topic gets merged all at once and > > makes some pretty nasty reviewer overhead. > > One approach is to build up a series with "git hook run" and "git hook > list" demonstrating and testing the functionality and [PATCH n+1/n] > extra patches at the end converting existing hooks. The user > experience from "git hook run" and even "git hook list" supporting a > preview of the future without built-in commands living in that future > yet would not be so bad, methinks. And then a final series could > update the built-in commands' usage of hooks and would still be fairly > small. > > In other words, I think I like (1), except *without* the > recommendation for users to edit their hook scripts to call "git hook > run" --- instead, the recommendation would be "try running this > command if you want to see what hooks will do in the future". Ok. I'll fix up the wording in the design doc and follow through with my plan to split the series into two parts. - Emily
diff --git a/builtin/commit.c b/builtin/commit.c index 69ac78d5e5..a19c6478eb 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -36,6 +36,7 @@ #include "help.h" #include "commit-reach.h" #include "commit-graph.h" +#include "hook.h" static const char * const builtin_commit_usage[] = { N_("git commit [<options>] [--] <pathspec>..."), @@ -985,7 +986,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, return 0; } - if (!no_verify && find_hook("pre-commit")) { + if (!no_verify && hook_exists("pre-commit")) { /* * Re-read the index as pre-commit hook could have updated it, * and write it out as a tree. We must do this before we invoke diff --git a/builtin/merge.c b/builtin/merge.c index 74829a838e..c1a9d0083d 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -41,6 +41,7 @@ #include "commit-reach.h" #include "wt-status.h" #include "commit-graph.h" +#include "hook.h" #define DEFAULT_TWOHEAD (1<<0) #define DEFAULT_OCTOPUS (1<<1) @@ -829,7 +830,7 @@ static void prepare_to_commit(struct commit_list *remoteheads) * and write it out as a tree. We must do this before we invoke * the editor and after we invoke run_status above. */ - if (find_hook("pre-merge-commit")) + if (hook_exists("pre-merge-commit")) discard_cache(); read_cache_from(index_file); strbuf_addbuf(&msg, &merge_msg); diff --git a/commit.c b/commit.c index 4ce8cb38d5..c7a243e848 100644 --- a/commit.c +++ b/commit.c @@ -21,6 +21,7 @@ #include "commit-reach.h" #include "run-command.h" #include "shallow.h" +#include "hook.h" static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **); @@ -1632,8 +1633,13 @@ int run_commit_hook(int editor_is_used, const char *index_file, { struct strvec hook_env = STRVEC_INIT; va_list args; + const char *arg; + struct strvec hook_args = STRVEC_INIT; + struct strbuf hook_name = STRBUF_INIT; int ret; + strbuf_addstr(&hook_name, name); + strvec_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file); /* @@ -1643,9 +1649,14 @@ int run_commit_hook(int editor_is_used, const char *index_file, strvec_push(&hook_env, "GIT_EDITOR=:"); va_start(args, name); - ret = run_hook_ve(hook_env.v, name, args); + while ((arg = va_arg(args, const char *))) + strvec_push(&hook_args, arg); va_end(args); + + ret = run_hooks(hook_env.v, &hook_name, &hook_args); strvec_clear(&hook_env); + strvec_clear(&hook_args); + strbuf_release(&hook_name); return ret; } diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh index b3485450a2..cef8085dcc 100755 --- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh +++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh @@ -103,6 +103,19 @@ test_expect_success 'with succeeding hook' ' test_cmp expected_hooks actual_hooks ' +# NEEDSWORK: when 'git hook add' and 'git hook remove' have been added, use that +# instead +test_expect_success 'with succeeding hook (config-based)' ' + test_when_finished "git config --unset hook.pre-commit.command success.sample" && + test_when_finished "rm -f expected_hooks actual_hooks" && + git config hook.pre-commit.command "$HOOKDIR/success.sample" && + echo "$HOOKDIR/success.sample" >expected_hooks && + echo "more" >>file && + git add file && + git commit -m "more" && + test_cmp expected_hooks actual_hooks +' + test_expect_success 'with succeeding hook (merge)' ' test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" && cp "$HOOKDIR/success.sample" "$PREMERGE" &&
As part of the adoption of config-based hooks, teach run_commit_hook() to call hook.h instead of run-command.h. This covers 'pre-commit', 'commit-msg', and 'prepare-commit-msg'. Additionally, ask the hook library - not run-command - whether any hooks will be run, as it's possible hooks may exist in the config but not the hookdir. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- builtin/commit.c | 3 ++- builtin/merge.c | 3 ++- commit.c | 13 ++++++++++++- t/t7503-pre-commit-and-pre-merge-commit-hooks.sh | 13 +++++++++++++ 4 files changed, 29 insertions(+), 3 deletions(-)