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

["Rubén Justo" <rjusto@gmail.com>]

On 7/25/24 5:24 PM, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> ... We
>> can add a comment about the dash problem to the commit message when
>> this fixup is squashed. Also the problem is now documented in
>> Documentation/CodingGuidelines which is more likely to be read by
>> other contributors.
> 
> That is a good thing to take into consideration, indeed.

Rubén Justo (2):
  pager: introduce wait_for_pager
  add-patch: render hunks through the pager

 add-patch.c                | 18 ++++++++++++---
 pager.c                    | 46 +++++++++++++++++++++++++++++++++-----
 pager.h                    |  1 +
 t/t3701-add-interactive.sh | 32 ++++++++++++++++++++++++++
 4 files changed, 88 insertions(+), 9 deletions(-)

Range-diff:
1:  b29c59e3d2 ! 1:  8a6116ef86 pager: introduce wait_for_pager
    @@ Commit message
         In the interactive commands (i.e.: add -p) we want to use the pager for
         some output, while maintaining the interaction with the user.
     
    -    Modify the pager machinery so that we can use setup_pager and, once
    +    Modify the pager machinery so that we can use `setup_pager()` and, once
         we've finished sending the desired output for the pager, wait for the
    -    pager termination using a new function wait_for_pager.   Make this
    +    pager termination using a new function `wait_for_pager()`.  Make this
         function reset the pager machinery before returning.
     
    +    One specific point to note is that we avoid forking the pager in
    +    `setup_pager()` if the configured pager is an empty string [*1*] or
    +    simply "cat" [*2*].  In these cases, `setup_pager()` does nothing and
    +    therefore `wait_for_pager()` should not be called.
    +
    +    We could modify `setup_pager()` to return an indication of these
    +    situations, so we could avoid calling `wait_for_pager()`.
    +
    +    However, let's avoid transferring 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 the pager.
    +
    +       1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,
    +                       2006-04-16)
    +
    +       2.- caef71a535 (Do not fork PAGER=cat, 2006-04-16)
    +
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## pager.c ##
     @@ pager.c: int pager_use_color = 1;
    @@ pager.c: static void wait_for_pager_atexit(void)
     +
     +void wait_for_pager(void)
     +{
    ++	if (old_fd1 == -1)
    ++		return;
    ++
     +	finish_pager();
     +	sigchain_pop_common();
     +	unsetenv("GIT_PAGER_IN_USE");
2:  6bc52a5543 ! 2:  980187854a add-patch: render hunks through the pager
    @@ Commit message
         few KBs to 1MB.  Use a payload large enough to guarantee that we exceed
         this limit.
     
    +    For the tests, avoid the common construct to set and export one-shot
    +    variables within the scope of a command:
    +
    +        VAR=VAL command args
    +
    +    It happens that when "command" is a shell function that in turn executes
    +    a "command", the behavior with "VAR" varies depending on the shell:
    +
    +     ** Bash 5.2.21 **
    +
    +        $ f () { bash -c 'echo A=$A'; }
    +        $ A=1 f
    +        A=1
    +
    +     ** dash 0.5.12-9 **
    +
    +        $ f () { bash -c 'echo A=$A'; }
    +        $ A=1 f
    +        A=1
    +
    +     ** dash 0.5.10.2-6 **
    +
    +        $ f () { bash -c 'echo A=$A'; }
    +        $ A=1 f
    +        A=
    +
    +    POSIX explicitly says the effect of this construct is unspecified.
    +
    +    One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash
    +    0.5.10.2-6, so avoid using the construct and use a subshell instead.
    +
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## add-patch.c ##
     @@
    @@ t/t3701-add-interactive.sh: test_expect_success 'print again the hunk' '
     +	test_when_finished "rm -f huge_file; git reset" &&
     +	printf "\n%2500000s" Y >huge_file &&
     +	git add -N huge_file &&
    -+	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
    ++	test_write_lines P q | (
    ++		GIT_PAGER="head -n 1" &&
    ++		export GIT_PAGER &&
    ++		test_terminal git add -p
    ++	)
     +'
     +
      test_expect_success 'split hunk "add -p (edit)"' '
3:  b7637a9f21 < -:  ---------- t3701: avoid one-shot export for shell functions
4:  4b53ff8c0e < -:  ---------- pager: make wait_for_pager a no-op for "cat"

base-commit: 506f457e489b2097e2d4fc5ceffd6e242502b2bd
-- 
2.46.0.rc0.4.g6f4990c0d4