From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Fri, 26 Sep 2025 12:29:37 GMT
Subject: Re: [PATCH 03/10] hook: convert 'post-rewrite' hook in sequencer.c to hook.h
Message-ID: <87bjmxo0r2.fsf@collabora.com>
In-Reply-To: <xmqq8qi2ffv6.fsf@gitster.g>

On Thu, 25 Sep 2025, Junio C Hamano <gitster@pobox.com> wrote:
> Adrian Ratiu <adrian.ratiu@collabora.com> writes: 
> 
>> From: Emily Shaffer <emilyshaffer@google.com> 
>> 
>> By using 'hook.h' for 'post-rewrite', we simplify hook 
>> invocations by 
> 
> This "By using 'hook.h" is somewhat a strange thing to say. 
> <hook.h> has been in use by the file (evidenced by the fact that 
> there is no new "#include <hook.h>" in the patch).  I haven't 
> carefully read other steps in this series, but from my quick 
> skimming of them, I got an impression that this comment may 
> apply equally to other steps as well. 
> 
> What the commit does is not "use hook.h"; it is to replace a 
> custom run-command call with a call to run_hooks_opt().  The 
> shared API service function may happen to be declared in 
> <hook.h>, that that is secondary piece of information. 
> 
>> not needing to put together our own 'struct child_process'. 
> 
> Imperative?  I think just dropping "we" would be sufficient. 
> 
>> The signal handling that's being removed by this commit now 
>> takes place in run-command.h:run_processes_parallel(), so it is 
>> OK to remove them here. 
> 
> Phrase it more positively, instead of "it is OK" (which sounds 
> like it is also OK to leave it there).  Perhaps say something 
> like: 
> 
>     Another benefit we gain from using run_hook_opt() instead of 
>     a custom start_command()/finish_command() invocations is 
>     that the hook API handles with sigpipe itself, so we no 
>     longer need to toggle signals ourselves. 
> 
> or something like that, perhaps. 

Ack, will reword in v2. I actually inherited this message and thought
about rewording it as well :). Some others I already reworded in v1.