Re: [PATCH 4/5] sequencer: use the new hook API for the simpler "post-rewrite" call

[Phillip Wood <phillip.wood123@gmail.com>]

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());