Re: [PATCH 03/10] hook: convert 'post-rewrite' hook in sequencer.c to hook.h

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 25, 2025, 20:15 UTC
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.