| Message ID | patch-4.5-7a55c95f60f-20230123T170551Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hook API: support stdin, convert post-rewrite | expand |
Hi Ævar On 23/01/2023 17:15, Ævar Arnfjörð Bjarmason wrote: > From: Emily Shaffer <emilyshaffer@google.com> > > Change the invocation of the "post-rewrite" hook added in > 795160457db (sequencer (rebase -i): run the post-rewrite hook, if > needed, 2017-01-02) to use the new hook API. > > This leaves the more complex "post-rewrite" invocation added in > a87a6f3c98e (commit: move post-rewrite code to libgit, 2017-11-17) > here in sequencer.c unconverted. That'll be done in a subsequent > commit. As a reader I'd find it more helpful to explain why the conversion isn't done here rather than leaving be to run "git show" to figure it out. If you re-roll perhaps we could replace the commit citation with something like sequencer.c also contains an invocation of the "post-rewrite" hook in run_rewrite_hook() that is not converted as the hook API does not allow us to pass the hook input as a string yet. Best Wishes Phillip > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > sequencer.c | 18 ++++-------------- > 1 file changed, 4 insertions(+), 14 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 3e4a1972897..d8d59d05dd4 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -4834,8 +4834,7 @@ 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"); > + struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT; > > child.in = open(rebase_path_rewritten_list(), O_RDONLY); > child.git_cmd = 1; > @@ -4845,18 +4844,9 @@ 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"; > - strvec_push(&hook.args, post_rewrite_hook); > - strvec_push(&hook.args, "rebase"); > - /* we don't care if this hook failed */ > - run_command(&hook); > - } > + hook_opt.path_to_stdin = rebase_path_rewritten_list(); > + strvec_push(&hook_opt.args, "rebase"); > + run_hooks_opt("post-rewrite", &hook_opt); > } > apply_autostash(rebase_path_autostash()); >
Hi Ævar On 24/01/2023 14:46, Phillip Wood wrote: > Hi Ævar > > On 23/01/2023 17:15, Ævar Arnfjörð Bjarmason wrote: >> From: Emily Shaffer <emilyshaffer@google.com> >> >> Change the invocation of the "post-rewrite" hook added in >> 795160457db (sequencer (rebase -i): run the post-rewrite hook, if >> needed, 2017-01-02) to use the new hook API. >> >> This leaves the more complex "post-rewrite" invocation added in >> a87a6f3c98e (commit: move post-rewrite code to libgit, 2017-11-17) >> here in sequencer.c unconverted. That'll be done in a subsequent >> commit. > > As a reader I'd find it more helpful to explain why the conversion isn't > done here rather than leaving be to run "git show" to figure it out. If > you re-roll perhaps we could replace the commit citation with something > like > > sequencer.c also contains an invocation of the "post-rewrite" hook in > run_rewrite_hook() that is not converted as the hook API does not allow > us to pass the hook input as a string yet. Sorry, I forgot to say in my previous reply that I like the code change here - it is a nice simplification for callers. builtin/am.c has a similar function to the one that is converted here. Best Wishes Phillip > Best Wishes > > Phillip > >> Signed-off-by: Emily Shaffer <emilyshaffer@google.com> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> sequencer.c | 18 ++++-------------- >> 1 file changed, 4 insertions(+), 14 deletions(-) >> >> diff --git a/sequencer.c b/sequencer.c >> index 3e4a1972897..d8d59d05dd4 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -4834,8 +4834,7 @@ 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"); >> + struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT; >> child.in = open(rebase_path_rewritten_list(), O_RDONLY); >> child.git_cmd = 1; >> @@ -4845,18 +4844,9 @@ 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"; >> - strvec_push(&hook.args, post_rewrite_hook); >> - strvec_push(&hook.args, "rebase"); >> - /* we don't care if this hook failed */ >> - run_command(&hook); >> - } >> + hook_opt.path_to_stdin = rebase_path_rewritten_list(); >> + strvec_push(&hook_opt.args, "rebase"); >> + run_hooks_opt("post-rewrite", &hook_opt); >> } >> apply_autostash(rebase_path_autostash());
diff --git a/sequencer.c b/sequencer.c index 3e4a1972897..d8d59d05dd4 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4834,8 +4834,7 @@ 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"); + struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT; child.in = open(rebase_path_rewritten_list(), O_RDONLY); child.git_cmd = 1; @@ -4845,18 +4844,9 @@ 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"; - strvec_push(&hook.args, post_rewrite_hook); - strvec_push(&hook.args, "rebase"); - /* we don't care if this hook failed */ - run_command(&hook); - } + hook_opt.path_to_stdin = rebase_path_rewritten_list(); + strvec_push(&hook_opt.args, "rebase"); + run_hooks_opt("post-rewrite", &hook_opt); } apply_autostash(rebase_path_autostash());