Re: [PATCH v2 0/2] add-p P fixups

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 23, 2024, 00:54 UTC
Message-ID: <xmqqa5i953nu.fsf@gitster.g>
In-Reply-To: <7c9ec43d-f52f-49b7-b1f3-fe3c85554006@gmail.com>

Rubén Justo <rjusto@gmail.com> writes:

>     +    Note that POSIX is not specific about this behavior:

This is misleading. It _specifically_ says that the behaviour is unspecified. Here "unspecified" is a term of art with a much less vague meaning. With respect to the points described as unspecified, compliant implementations of shell can behave differently, and scripts (like our tests) that assume one behaviour cannot complain if a POSIX compliant shell implements the behaviour differently and "breaks" them.

So "POSIX says the behaviour is unspecified" would be more appropriate.

Show 16 quoted lines

>     +    http://www.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01
>     +
>          One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash
>          0.5.10.2-6, so we failed the test t3701:51;  the "git add -p" being
>          tested did not get our custom GIT_PAGER, which broke the test.
> 2:  f45455f1ff ! 2:  b87c3d96e4 pager: make wait_for_pager a no-op for "cat"
>     @@ Commit message
>          "cat" [*2*], then we return from `setup_pager()` silently without doing
>          anything, allowing the output to go directly to the normal stdout.
>      
>     -    Let's make the call to `wait_for_pager()` for these cases, or any other
>     -    future optimizations that may occur, also exit silently without doing
>     -    anything.
>     +    If `setup_pager()` avoids forking a pager, then when the client calls
>     +    the corresponding `wait_for_pager()`, we might fail trying to terminate
>     +    a process that wasn't started.

It may try to stop one, but because we didn't start one to begin with, there is nothing to stop. Then is there any problem and why?

In other words, I was hoping that we can clearly say what the externally visible breakage was.

Show 7 quoted lines

>     +    One solution to avoid this problem could be to make the caller aware
>     +    that `setup_pager()` did nothing, so it could avoid calling
>     +    `wait_for_pager()`.
>     +
>     +    However, let's avoid shifting that responsibility to the caller and
>     +    instead treat the call to `wait_for_pager()` as a no-op when we know we
>     +    haven't forked a pager.

These two paragraphs are good.

Thanks.