| Message ID | 20201205014945.1502660-11-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | use config-based hooks (config-based hooks part | expand |
On 2020.12.04 17:49, Emily Shaffer wrote: > diff --git a/sequencer.c b/sequencer.c > index 5a98fd2fbc..4befd862ff 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -35,6 +35,7 @@ > #include "rebase-interactive.h" > #include "reset.h" > #include "hook.h" > +#include "string-list.h" > > #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" > > @@ -1143,33 +1144,23 @@ int update_head_with_reflog(const struct commit *old_head, > static int run_rewrite_hook(const struct object_id *oldoid, > const struct object_id *newoid) > { > - struct child_process proc = CHILD_PROCESS_INIT; > - const char *argv[3]; > + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC; > + struct strbuf tmp = STRBUF_INIT; > int code; > - struct strbuf sb = STRBUF_INIT; > > - argv[0] = find_hook("post-rewrite"); > - if (!argv[0]) > - return 0; > + strvec_push(&opt.args, "amend"); > > - argv[1] = "amend"; > - argv[2] = NULL; > - > - proc.argv = argv; > - proc.in = -1; > - proc.stdout_to_stderr = 1; > - proc.trace2_hook_name = "post-rewrite"; > - > - code = start_command(&proc); > - if (code) > - return code; > - strbuf_addf(&sb, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid)); > - sigchain_push(SIGPIPE, SIG_IGN); Here and in a few other later patches, we're removing some signal handling that doesn't seem to be replicated in the run_hooks() implementation. Can you add a note to the commit message about why this is OK? > - write_in_full(proc.in, sb.buf, sb.len); > - close(proc.in); > - strbuf_release(&sb); > - sigchain_pop(SIGPIPE); > - return finish_command(&proc); > + strbuf_addf(&tmp, > + "%s %s", > + oid_to_hex(oldoid), > + oid_to_hex(newoid)); > + string_list_append(&opt.str_stdin, tmp.buf); > + > + code = run_hooks("post-rewrite", &opt); > + > + run_hooks_opt_clear(&opt); > + strbuf_release(&tmp); > + return code; > } > > void commit_post_rewrite(struct repository *r, > @@ -4317,30 +4308,21 @@ static int pick_commits(struct repository *r, > flush_rewritten_pending(); > if (!stat(rebase_path_rewritten_list(), &st) && > st.st_size > 0) { > - struct child_process child = CHILD_PROCESS_INIT; > - const char *post_rewrite_hook = > - find_hook("post-rewrite"); > - > - child.in = open(rebase_path_rewritten_list(), O_RDONLY); > - child.git_cmd = 1; > - strvec_push(&child.args, "notes"); > - strvec_push(&child.args, "copy"); > - strvec_push(&child.args, "--for-rewrite=rebase"); > + struct child_process notes_cp = CHILD_PROCESS_INIT; > + struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_ASYNC; > + > + notes_cp.in = open(rebase_path_rewritten_list(), O_RDONLY); > + notes_cp.git_cmd = 1; > + strvec_push(¬es_cp.args, "notes"); > + strvec_push(¬es_cp.args, "copy"); > + strvec_push(¬es_cp.args, "--for-rewrite=rebase"); > /* we don't care if this copying failed */ > - run_command(&child); > - > - if (post_rewrite_hook) { > - struct child_process hook = CHILD_PROCESS_INIT; > - > - hook.in = open(rebase_path_rewritten_list(), > - O_RDONLY); > - hook.stdout_to_stderr = 1; > - hook.trace2_hook_name = "post-rewrite"; > - strvec_push(&hook.args, post_rewrite_hook); > - strvec_push(&hook.args, "rebase"); > - /* we don't care if this hook failed */ > - run_command(&hook); > - } > + run_command(¬es_cp); > + > + hook_opt.path_to_stdin = rebase_path_rewritten_list(); > + strvec_push(&hook_opt.args, "rebase"); > + run_hooks("post-rewrite", &hook_opt); > + run_hooks_opt_clear(&hook_opt); > } > apply_autostash(rebase_path_autostash()); > > -- > 2.28.0.rc0.142.g3c755180ce-goog >
On Tue, Dec 08, 2020 at 03:02:13PM -0800, Josh Steadmon wrote: > > On 2020.12.04 17:49, Emily Shaffer wrote: > > diff --git a/sequencer.c b/sequencer.c > > index 5a98fd2fbc..4befd862ff 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -35,6 +35,7 @@ > > #include "rebase-interactive.h" > > #include "reset.h" > > #include "hook.h" > > +#include "string-list.h" > > > > #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" > > > > @@ -1143,33 +1144,23 @@ int update_head_with_reflog(const struct commit *old_head, > > static int run_rewrite_hook(const struct object_id *oldoid, > > const struct object_id *newoid) > > { > > - struct child_process proc = CHILD_PROCESS_INIT; > > - const char *argv[3]; > > + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC; > > + struct strbuf tmp = STRBUF_INIT; > > int code; > > - struct strbuf sb = STRBUF_INIT; > > > > - argv[0] = find_hook("post-rewrite"); > > - if (!argv[0]) > > - return 0; > > + strvec_push(&opt.args, "amend"); > > > > - argv[1] = "amend"; > > - argv[2] = NULL; > > - > > - proc.argv = argv; > > - proc.in = -1; > > - proc.stdout_to_stderr = 1; > > - proc.trace2_hook_name = "post-rewrite"; > > - > > - code = start_command(&proc); > > - if (code) > > - return code; > > - strbuf_addf(&sb, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid)); > > - sigchain_push(SIGPIPE, SIG_IGN); > > Here and in a few other later patches, we're removing some signal > handling that doesn't seem to be replicated in the run_hooks() > implementation. Can you add a note to the commit message about why this > is OK? Yeah, I have added it to the commit message for next rollup. I think offline you said you found it - but this signal handling was added when I taught run_processes_parallel() to use a callback to provide stdin. It lives in run-command.h now. - Emily
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index 14035ef725..db290984f6 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -585,6 +585,8 @@ The hook always runs after the automatic note copying (see "notes.rewrite.<command>" in linkgit:git-config[1]) has happened, and thus has access to these notes. +Hooks run during 'post-rewrite' will be run in parallel by default. + The following command-specific comments apply: rebase:: diff --git a/builtin/am.c b/builtin/am.c index 22d147bc19..c7cad2cb32 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -449,23 +449,15 @@ static int run_applypatch_msg_hook(struct am_state *state) */ static int run_post_rewrite_hook(const struct am_state *state) { - struct child_process cp = CHILD_PROCESS_INIT; - const char *hook = find_hook("post-rewrite"); + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC; int ret; - if (!hook) - return 0; - - strvec_push(&cp.args, hook); - strvec_push(&cp.args, "rebase"); + strvec_push(&opt.args, "rebase"); + opt.path_to_stdin = am_path(state, "rewritten"); - cp.in = xopen(am_path(state, "rewritten"), O_RDONLY); - cp.stdout_to_stderr = 1; - cp.trace2_hook_name = "post-rewrite"; + ret = run_hooks("post-rewrite", &opt); - ret = run_command(&cp); - - close(cp.in); + run_hooks_opt_clear(&opt); return ret; } diff --git a/sequencer.c b/sequencer.c index 5a98fd2fbc..4befd862ff 100644 --- a/sequencer.c +++ b/sequencer.c @@ -35,6 +35,7 @@ #include "rebase-interactive.h" #include "reset.h" #include "hook.h" +#include "string-list.h" #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" @@ -1143,33 +1144,23 @@ int update_head_with_reflog(const struct commit *old_head, static int run_rewrite_hook(const struct object_id *oldoid, const struct object_id *newoid) { - struct child_process proc = CHILD_PROCESS_INIT; - const char *argv[3]; + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC; + struct strbuf tmp = STRBUF_INIT; int code; - struct strbuf sb = STRBUF_INIT; - argv[0] = find_hook("post-rewrite"); - if (!argv[0]) - return 0; + strvec_push(&opt.args, "amend"); - argv[1] = "amend"; - argv[2] = NULL; - - proc.argv = argv; - proc.in = -1; - proc.stdout_to_stderr = 1; - proc.trace2_hook_name = "post-rewrite"; - - code = start_command(&proc); - if (code) - return code; - strbuf_addf(&sb, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid)); - sigchain_push(SIGPIPE, SIG_IGN); - write_in_full(proc.in, sb.buf, sb.len); - close(proc.in); - strbuf_release(&sb); - sigchain_pop(SIGPIPE); - return finish_command(&proc); + strbuf_addf(&tmp, + "%s %s", + oid_to_hex(oldoid), + oid_to_hex(newoid)); + string_list_append(&opt.str_stdin, tmp.buf); + + code = run_hooks("post-rewrite", &opt); + + run_hooks_opt_clear(&opt); + strbuf_release(&tmp); + return code; } void commit_post_rewrite(struct repository *r, @@ -4317,30 +4308,21 @@ static int pick_commits(struct repository *r, flush_rewritten_pending(); if (!stat(rebase_path_rewritten_list(), &st) && st.st_size > 0) { - struct child_process child = CHILD_PROCESS_INIT; - const char *post_rewrite_hook = - find_hook("post-rewrite"); - - child.in = open(rebase_path_rewritten_list(), O_RDONLY); - child.git_cmd = 1; - strvec_push(&child.args, "notes"); - strvec_push(&child.args, "copy"); - strvec_push(&child.args, "--for-rewrite=rebase"); + struct child_process notes_cp = CHILD_PROCESS_INIT; + struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_ASYNC; + + notes_cp.in = open(rebase_path_rewritten_list(), O_RDONLY); + notes_cp.git_cmd = 1; + strvec_push(¬es_cp.args, "notes"); + strvec_push(¬es_cp.args, "copy"); + strvec_push(¬es_cp.args, "--for-rewrite=rebase"); /* we don't care if this copying failed */ - run_command(&child); - - if (post_rewrite_hook) { - struct child_process hook = CHILD_PROCESS_INIT; - - hook.in = open(rebase_path_rewritten_list(), - O_RDONLY); - hook.stdout_to_stderr = 1; - hook.trace2_hook_name = "post-rewrite"; - strvec_push(&hook.args, post_rewrite_hook); - strvec_push(&hook.args, "rebase"); - /* we don't care if this hook failed */ - run_command(&hook); - } + run_command(¬es_cp); + + hook_opt.path_to_stdin = rebase_path_rewritten_list(); + strvec_push(&hook_opt.args, "rebase"); + run_hooks("post-rewrite", &hook_opt); + run_hooks_opt_clear(&hook_opt); } apply_autostash(rebase_path_autostash());
By using 'hook.h' for 'post-rewrite', we simplify hook invocations by not needing to put together our own 'struct child_process' and we also learn to run hooks specified in the config as well as the hook dir. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Documentation/githooks.txt | 2 + builtin/am.c | 18 +++------ sequencer.c | 76 +++++++++++++++----------------------- 3 files changed, 36 insertions(+), 60 deletions(-)