On 7/25/24 5:24 PM, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@xxxxxxxxx> 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@xxxxxxxxx>
- Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
## 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@xxxxxxxxx>
- Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
## 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
