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

[Junio C Hamano <gitster@pobox.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.

>     +    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.

>     +    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.