From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 25 Sep 2025 20:15:41 GMT
Subject: Re: [PATCH 03/10] hook: convert 'post-rewrite' hook in sequencer.c to hook.h
Message-ID: <xmqq8qi2ffv6.fsf@gitster.g>
In-Reply-To: <20250925125352.1728840-4-adrian.ratiu@collabora.com>

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.

Thanks.