| Message ID | 20190514002332.121089-4-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Multiple hook support | expand |
On Tue, May 14, 2019 at 7:24 AM brian m. carlson <sandals@crustytoothpaste.net> wrote: > diff --git a/builtin/am.c b/builtin/am.c > index 912d9821b1..340eacbd44 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -441,24 +441,8 @@ 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"); > - int ret; > - > - if (!hook) > - return 0; > - > - argv_array_push(&cp.args, hook); > - argv_array_push(&cp.args, "rebase"); > - > - cp.in = xopen(am_path(state, "rewritten"), O_RDONLY); > - cp.stdout_to_stderr = 1; > - cp.trace2_hook_name = "post-rewrite"; > - > - ret = run_command(&cp); > - > - close(cp.in); In the old code, we close cp.in... > +int post_rewrite_rebase_hook(const char *name, const char *path, void *input) > +{ > + struct child_process child = CHILD_PROCESS_INIT; > + > + child.in = open(input, O_RDONLY); > + child.stdout_to_stderr = 1; > + child.trace2_hook_name = "post-rewrite"; maybe use "name" and avoid hard coding "post-rewrite". > + argv_array_push(&child.args, path); > + argv_array_push(&child.args, "rebase"); > + return run_command(&child); ... but in the new one we don't. Smells fd leaking to me.
Am 14.05.19 um 14:56 schrieb Duy Nguyen: > On Tue, May 14, 2019 at 7:24 AM brian m. carlson > <sandals@crustytoothpaste.net> wrote: >> diff --git a/builtin/am.c b/builtin/am.c >> index 912d9821b1..340eacbd44 100644 >> --- a/builtin/am.c >> +++ b/builtin/am.c >> @@ -441,24 +441,8 @@ 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"); >> - int ret; >> - >> - if (!hook) >> - return 0; >> - >> - argv_array_push(&cp.args, hook); >> - argv_array_push(&cp.args, "rebase"); >> - >> - cp.in = xopen(am_path(state, "rewritten"), O_RDONLY); >> - cp.stdout_to_stderr = 1; >> - cp.trace2_hook_name = "post-rewrite"; >> - >> - ret = run_command(&cp); >> - >> - close(cp.in); > > In the old code, we close cp.in... > >> +int post_rewrite_rebase_hook(const char *name, const char *path, void *input) >> +{ >> + struct child_process child = CHILD_PROCESS_INIT; >> + >> + child.in = open(input, O_RDONLY); >> + child.stdout_to_stderr = 1; >> + child.trace2_hook_name = "post-rewrite"; > >> + argv_array_push(&child.args, path); >> + argv_array_push(&child.args, "rebase"); >> + return run_command(&child); > > ... but in the new one we don't. Smells fd leaking to me. IIRC, run_command always closes the fds that it receives (even on error paths). Therefore, the old code is incorrect and should not call close(cp.in). -- Hannes
On Tue, May 14, 2019 at 07:56:49PM +0700, Duy Nguyen wrote: > On Tue, May 14, 2019 at 7:24 AM brian m. carlson > <sandals@crustytoothpaste.net> wrote: > > - close(cp.in); > > In the old code, we close cp.in... > > > +int post_rewrite_rebase_hook(const char *name, const char *path, void *input) > > +{ > > + struct child_process child = CHILD_PROCESS_INIT; > > + > > + child.in = open(input, O_RDONLY); > > + child.stdout_to_stderr = 1; > > + child.trace2_hook_name = "post-rewrite"; > > maybe use "name" and avoid hard coding "post-rewrite". > > > + argv_array_push(&child.args, path); > > + argv_array_push(&child.args, "rebase"); > > + return run_command(&child); > > ... but in the new one we don't. Smells fd leaking to me. Ah, good point. Will fix.
On Thu, May 16, 2019 at 5:55 AM brian m. carlson <sandals@crustytoothpaste.net> wrote: > > On Tue, May 14, 2019 at 07:56:49PM +0700, Duy Nguyen wrote: > > On Tue, May 14, 2019 at 7:24 AM brian m. carlson > > <sandals@crustytoothpaste.net> wrote: > > > - close(cp.in); > > > > In the old code, we close cp.in... > > > > > +int post_rewrite_rebase_hook(const char *name, const char *path, void *input) > > > +{ > > > + struct child_process child = CHILD_PROCESS_INIT; > > > + > > > + child.in = open(input, O_RDONLY); > > > + child.stdout_to_stderr = 1; > > > + child.trace2_hook_name = "post-rewrite"; > > > > maybe use "name" and avoid hard coding "post-rewrite". > > > > > + argv_array_push(&child.args, path); > > > + argv_array_push(&child.args, "rebase"); > > > + return run_command(&child); > > > > ... but in the new one we don't. Smells fd leaking to me. > > Ah, good point. Will fix. Actually I think Johannes is right (in the other mail, same thread) that the old code is buggy. The only good thing is, I don't think the old code could actually result in double close() problem. It would just return EBADF, which we don't care. So no hurry fixing the old code until you replace it with a better one.
diff --git a/builtin/am.c b/builtin/am.c index 912d9821b1..340eacbd44 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -441,24 +441,8 @@ 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"); - int ret; - - if (!hook) - return 0; - - argv_array_push(&cp.args, hook); - argv_array_push(&cp.args, "rebase"); - - cp.in = xopen(am_path(state, "rewritten"), O_RDONLY); - cp.stdout_to_stderr = 1; - cp.trace2_hook_name = "post-rewrite"; - - ret = run_command(&cp); - - close(cp.in); - return ret; + return for_each_hook("post-rewrite", post_rewrite_rebase_hook, + (void *)am_path(state, "rewritten")); } /** diff --git a/sequencer.c b/sequencer.c index 546f281898..b899209f76 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1084,30 +1084,32 @@ int update_head_with_reflog(const struct commit *old_head, return ret; } -static int run_rewrite_hook(const struct object_id *oldoid, - const struct object_id *newoid) +struct rewrite_hook_data { + const struct object_id *oldoid; + const struct object_id *newoid; +}; + +static int do_run_rewrite_hook(const char *name, const char *path, void *p) { + struct rewrite_hook_data *data = p; struct child_process proc = CHILD_PROCESS_INIT; + struct strbuf sb = STRBUF_INIT; const char *argv[3]; int code; - struct strbuf sb = STRBUF_INIT; - - argv[0] = find_hook("post-rewrite"); - if (!argv[0]) - return 0; + argv[0] = path; argv[1] = "amend"; argv[2] = NULL; proc.argv = argv; proc.in = -1; proc.stdout_to_stderr = 1; - proc.trace2_hook_name = "post-rewrite"; + proc.trace2_hook_name = name; code = start_command(&proc); if (code) return code; - strbuf_addf(&sb, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid)); + strbuf_addf(&sb, "%s %s\n", oid_to_hex(data->oldoid), oid_to_hex(data->newoid)); sigchain_push(SIGPIPE, SIG_IGN); write_in_full(proc.in, sb.buf, sb.len); close(proc.in); @@ -1116,6 +1118,25 @@ static int run_rewrite_hook(const struct object_id *oldoid, return finish_command(&proc); } +static int run_rewrite_hook(const struct object_id *oldoid, + const struct object_id *newoid) +{ + struct rewrite_hook_data data = { oldoid, newoid }; + return for_each_hook("post-rewrite", do_run_rewrite_hook, &data); +} + +int post_rewrite_rebase_hook(const char *name, const char *path, void *input) +{ + struct child_process child = CHILD_PROCESS_INIT; + + child.in = open(input, O_RDONLY); + child.stdout_to_stderr = 1; + child.trace2_hook_name = "post-rewrite"; + argv_array_push(&child.args, path); + argv_array_push(&child.args, "rebase"); + return run_command(&child); +} + void commit_post_rewrite(struct repository *r, const struct commit *old_head, const struct object_id *new_head) @@ -1368,7 +1389,7 @@ static int try_to_commit(struct repository *r, goto out; } - if (find_hook("prepare-commit-msg")) { + if (find_hooks("prepare-commit-msg", NULL)) { res = run_prepare_commit_msg_hook(r, msg, hook_commit); if (res) goto out; @@ -3763,8 +3784,6 @@ static int pick_commits(struct repository *r, 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; @@ -3774,18 +3793,10 @@ static int pick_commits(struct repository *r, /* 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"; - argv_array_push(&hook.args, post_rewrite_hook); - argv_array_push(&hook.args, "rebase"); - /* we don't care if this hook failed */ - run_command(&hook); - } + /* we don't care if this hook failed */ + for_each_hook("post-rewrite", + post_rewrite_rebase_hook, + (void *)rebase_path_rewritten_list()); } apply_autostash(opts); diff --git a/sequencer.h b/sequencer.h index a515ee4457..710ef5c18c 100644 --- a/sequencer.h +++ b/sequencer.h @@ -154,6 +154,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla unsigned autosquash, struct todo_list *todo_list); int todo_list_rearrange_squash(struct todo_list *todo_list); +int post_rewrite_rebase_hook(const char *name, const char *path, void *input); + /* * Append a signoff to the commit message in "msgbuf". The ignore_footer * parameter specifies the number of bytes at the end of msgbuf that should diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh index 7344253bfb..f8ce32fe3b 100755 --- a/t/t5407-post-rewrite-hook.sh +++ b/t/t5407-post-rewrite-hook.sh @@ -5,6 +5,7 @@ test_description='Test the post-rewrite hook.' . ./test-lib.sh +. "$TEST_DIRECTORY/lib-hooks.sh" test_expect_success 'setup' ' test_commit A foo A && @@ -263,4 +264,18 @@ test_expect_success 'git rebase -i (exec)' ' verify_hook_input ' +cmd_rebase () { + git reset --hard D && + FAKE_LINES="1 fixup 2" git rebase -i B +} + +cmd_amend () { + git reset --hard D && + echo "D new message" > newmsg && + git commit -Fnewmsg --amend +} + +test_multiple_hooks --ignore-exit-status post-rewrite cmd_rebase +test_multiple_hooks --ignore-exit-status post-rewrite cmd_amend + test_done diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh index ba8bd1b514..287e13e29d 100755 --- a/t/t7505-prepare-commit-msg-hook.sh +++ b/t/t7505-prepare-commit-msg-hook.sh @@ -3,6 +3,7 @@ test_description='prepare-commit-msg hook' . ./test-lib.sh +. "$TEST_DIRECTORY/lib-hooks.sh" test_expect_success 'set up commits for rebasing' ' test_commit root && @@ -317,4 +318,12 @@ test_expect_success C_LOCALE_OUTPUT 'with failing hook (cherry-pick)' ' test $(grep -c prepare-commit-msg actual) = 1 ' +cherry_pick_command () { + git checkout -f master && + git checkout -B other b && + git cherry-pick rebase-1 +} + +test_multiple_hooks prepare-commit-msg cherry_pick_command + test_done
Add support for multiple post-rewrite hooks, both for "git commit --amend" and "git rebase". Unify the hook handling between the am-based and sequencer-based code paths. Additionally add support for multiple prepare-commit-msg hooks. Note that the prepare-commit-msg hook is not passed a set of hooks directly because these are discovered later when the code calls run_hooks_le. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- builtin/am.c | 20 +--------- sequencer.c | 59 ++++++++++++++++++------------ sequencer.h | 2 + t/t5407-post-rewrite-hook.sh | 15 ++++++++ t/t7505-prepare-commit-msg-hook.sh | 9 +++++ 5 files changed, 63 insertions(+), 42 deletions(-)