{"thread":{"id":"61825","subject":"[PATCH v2 0/2] add-p P fixups","startedAt":"2024-07-23T00:40:01Z","lastAt":"2024-07-29T18:45:55Z","messageCount":29,"participants":["Rubén Justo","Junio C Hamano","Phillip Wood","phillip.wood123@gmail.com"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"499111","messageId":"7c9ec43d-f52f-49b7-b1f3-fe3c85554006@gmail.com","threadId":"61825","inReplyTo":null,"subject":"[PATCH v2 0/2] add-p P fixups","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-23T00:39:58Z","receivedAt":"2024-07-23T00:40:01Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Rubén Justo (1):\n t3701: avoid one-shot export for shell functions\n pager: make wait_for_pager a no-op for \"cat\"\n\n pager.c | 3 +++\n t/t3701-add-interactive.sh | 6 +++++-\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\nRange-diff against v1:\n1: c3b8ebbae7 ! 1: 15fbf82fff t3701: avoid one-shot export for shell functions\n @@ Commit message\n \n VAR=VAL command args\n \n - it's a common way to define one-shot variables within the scope of\n + is a common way to set and export one-shot variables within the scope of\n executing a \"command\".\n \n However, when \"command\" is a function which in turn executes the\n @@ Commit message\n $ A=1 f\n A=\n \n + Note that POSIX is not specific about this behavior:\n +\n + http://www.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01\n +\n One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash\n 0.5.10.2-6, so we failed the test t3701:51; the \"git add -p\" being\n tested did not get our custom GIT_PAGER, which broke the test.\n2: f45455f1ff ! 2: b87c3d96e4 pager: make wait_for_pager a no-op for \"cat\"\n @@ Commit message\n \"cat\" [*2*], then we return from `setup_pager()` silently without doing\n anything, allowing the output to go directly to the normal stdout.\n \n - Let's make the call to `wait_for_pager()` for these cases, or any other\n - future optimizations that may occur, also exit silently without doing\n - anything.\n + If `setup_pager()` avoids forking a pager, then when the client calls\n + the corresponding `wait_for_pager()`, we might fail trying to terminate\n + a process that wasn't started.\n +\n + One solution to avoid this problem could be to make the caller aware\n + that `setup_pager()` did nothing, so it could avoid calling\n + `wait_for_pager()`.\n +\n + However, let's avoid shifting that responsibility to the caller and\n + instead treat the call to `wait_for_pager()` as a no-op when we know we\n + haven't forked a pager.\n \n 1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,\n 2006-04-16)\n-- \n2.45.1\n"},{"id":"499112","messageId":"af444f9c-7f40-4afa-98dc-0b503642b58c@gmail.com","threadId":"61825","inReplyTo":"7c9ec43d-f52f-49b7-b1f3-fe3c85554006@gmail.com","subject":"[PATCH v2 1/2] t3701: avoid one-shot export for shell functions","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-23T00:42:10Z","receivedAt":"2024-07-23T00:42:12Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"The common construct:\n\n VAR=VAL command args\n\nis a common way to set and export one-shot variables within the scope of\nexecuting a \"command\".\n\nHowever, when \"command\" is a function which in turn executes the\n\"command\", the behavior varies depending on the shell:\n\n ** Bash 5.2.21 **\n\n $ f () { bash -c 'echo A=$A'; }\n $ A=1 f\n A=1\n\n ** dash 0.5.12-9 **\n\n $ f () { bash -c 'echo A=$A'; }\n $ A=1 f\n A=1\n\n ** dash 0.5.10.2-6 **\n\n $ f () { bash -c 'echo A=$A'; }\n $ A=1 f\n A=\n\nNote that POSIX is not specific about this behavior:\n\nhttp://www.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01\n\nOne of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash\n0.5.10.2-6, so we failed the test t3701:51; the \"git add -p\" being\ntested did not get our custom GIT_PAGER, which broke the test.\n\nWork it around by explicitly exporting the variable in a subshell.\n\nSigned-off-by: Rubén Justo \n---\n t/t3701-add-interactive.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex c60589cb94..1b8617e0c1 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -616,7 +616,11 @@ test_expect_success TTY 'P handles SIGPIPE when writing to pager' '\n \ttest_when_finished \"rm -f huge_file; git reset\" &&\n \tprintf \"\\n%2500000s\" Y >huge_file &&\n \tgit add -N huge_file &&\n-\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n+\ttest_write_lines P q | (\n+\t\tGIT_PAGER=\"head -n 1\" &&\n+\t\texport GIT_PAGER &&\n+\t\ttest_terminal git add -p\n+\t)\n '\n \n test_expect_success 'split hunk \"add -p (edit)\"' '\n-- \n2.45.1\n"},{"id":"499113","messageId":"a2d6169f-61ee-4a3f-9f8a-f791229c25a2@gmail.com","threadId":"61825","inReplyTo":"7c9ec43d-f52f-49b7-b1f3-fe3c85554006@gmail.com","subject":"[PATCH v2 2/2] pager: make wait_for_pager a no-op for \"cat\"","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-23T00:42:56Z","receivedAt":"2024-07-23T00:42:58Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"If we find that the configured pager is an empty string [*1*] or simply\n\"cat\" [*2*], then we return from `setup_pager()` silently without doing\nanything, allowing the output to go directly to the normal stdout.\n\nIf `setup_pager()` avoids forking a pager, then when the client calls\nthe corresponding `wait_for_pager()`, we might fail trying to terminate\na process that wasn't started.\n\nOne solution to avoid this problem could be to make the caller aware\nthat `setup_pager()` did nothing, so it could avoid calling\n`wait_for_pager()`.\n\nHowever, let's avoid shifting that responsibility to the caller and\ninstead treat the call to `wait_for_pager()` as a no-op when we know we\nhaven't forked a pager.\n\n 1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,\n 2006-04-16)\n\n 2.- caef71a535 (Do not fork PAGER=cat, 2006-04-16)\n\nSigned-off-by: Rubén Justo \n---\n pager.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/pager.c b/pager.c\nindex bea4345f6f..896f40fcd2 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -46,6 +46,9 @@ static void wait_for_pager_atexit(void)\n \n void wait_for_pager(void)\n {\n+\tif (old_fd1 == -1)\n+\t\treturn;\n+\n \tfinish_pager();\n \tsigchain_pop_common();\n \tunsetenv(\"GIT_PAGER_IN_USE\");\n-- \n2.45.1\n"},{"id":"499114","messageId":"xmqqa5i953nu.fsf@gitster.g","threadId":"61825","inReplyTo":"7c9ec43d-f52f-49b7-b1f3-fe3c85554006@gmail.com","subject":"Re: [PATCH v2 0/2] add-p P fixups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-23T00:54:13Z","receivedAt":"2024-07-23T00:54:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> + Note that POSIX is not specific about this behavior:\n\nThis is misleading. It _specifically_ says that the behaviour is\nunspecified. Here \"unspecified\" is a term of art with a much less\nvague meaning. With respect to the points described as unspecified,\ncompliant implementations of shell can behave differently, and\nscripts (like our tests) that assume one behaviour cannot complain\nif a POSIX compliant shell implements the behaviour differently and\n\"breaks\" them.\n\nSo \"POSIX says the behaviour is unspecified\" would be more\nappropriate.\n\n> + http://www.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01\n> +\n> One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash\n> 0.5.10.2-6, so we failed the test t3701:51; the \"git add -p\" being\n> tested did not get our custom GIT_PAGER, which broke the test.\n> 2: f45455f1ff ! 2: b87c3d96e4 pager: make wait_for_pager a no-op for \"cat\"\n> @@ Commit message\n> \"cat\" [*2*], then we return from `setup_pager()` silently without doing\n> anything, allowing the output to go directly to the normal stdout.\n> \n> - Let's make the call to `wait_for_pager()` for these cases, or any other\n> - future optimizations that may occur, also exit silently without doing\n> - anything.\n> + If `setup_pager()` avoids forking a pager, then when the client calls\n> + the corresponding `wait_for_pager()`, we might fail trying to terminate\n> + a process that wasn't started.\n\nIt may try to stop one, but because we didn't start one to begin\nwith, there is nothing to stop. Then is there any problem and why?\n\nIn other words, I was hoping that we can clearly say what the\nexternally visible breakage was.\n\n> + One solution to avoid this problem could be to make the caller aware\n> + that `setup_pager()` did nothing, so it could avoid calling\n> + `wait_for_pager()`.\n> +\n> + However, let's avoid shifting that responsibility to the caller and\n> + instead treat the call to `wait_for_pager()` as a no-op when we know we\n> + haven't forked a pager.\n\nThese two paragraphs are good.\n\nThanks.\n"},{"id":"499140","messageId":"62af789f-ca19-4f11-9339-a97400f7e70c@gmail.com","threadId":"61825","inReplyTo":"7c9ec43d-f52f-49b7-b1f3-fe3c85554006@gmail.com","subject":"Re: [PATCH v2 0/2] add-p P fixups","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-23T09:15:03Z","receivedAt":"2024-07-23T09:15:12Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nThese changes themselves look sensible. As rj/add-p-pager is only in \nseen I assume you'll re-roll with these squashed in once everyone is happy?\n\nBest Wishes\n\nPhillip\n\nOn 23/07/2024 01:39, Rubén Justo wrote:\n> Rubén Justo (1):\n> t3701: avoid one-shot export for shell functions\n> pager: make wait_for_pager a no-op for \"cat\"\n> \n> pager.c | 3 +++\n> t/t3701-add-interactive.sh | 6 +++++-\n> 2 files changed, 8 insertions(+), 1 deletion(-)\n> \n> Range-diff against v1:\n> 1: c3b8ebbae7 ! 1: 15fbf82fff t3701: avoid one-shot export for shell functions\n> @@ Commit message\n> \n> VAR=VAL command args\n> \n> - it's a common way to define one-shot variables within the scope of\n> + is a common way to set and export one-shot variables within the scope of\n> executing a \"command\".\n> \n> However, when \"command\" is a function which in turn executes the\n> @@ Commit message\n> $ A=1 f\n> A=\n> \n> + Note that POSIX is not specific about this behavior:\n> +\n> + http://www.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01\n> +\n> One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash\n> 0.5.10.2-6, so we failed the test t3701:51; the \"git add -p\" being\n> tested did not get our custom GIT_PAGER, which broke the test.\n> 2: f45455f1ff ! 2: b87c3d96e4 pager: make wait_for_pager a no-op for \"cat\"\n> @@ Commit message\n> \"cat\" [*2*], then we return from `setup_pager()` silently without doing\n> anything, allowing the output to go directly to the normal stdout.\n> \n> - Let's make the call to `wait_for_pager()` for these cases, or any other\n> - future optimizations that may occur, also exit silently without doing\n> - anything.\n> + If `setup_pager()` avoids forking a pager, then when the client calls\n> + the corresponding `wait_for_pager()`, we might fail trying to terminate\n> + a process that wasn't started.\n> +\n> + One solution to avoid this problem could be to make the caller aware\n> + that `setup_pager()` did nothing, so it could avoid calling\n> + `wait_for_pager()`.\n> +\n> + However, let's avoid shifting that responsibility to the caller and\n> + instead treat the call to `wait_for_pager()` as a no-op when we know we\n> + haven't forked a pager.\n> \n> 1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,\n> 2006-04-16)\n"},{"id":"499179","messageId":"xmqqed7k2gq0.fsf@gitster.g","threadId":"61825","inReplyTo":"62af789f-ca19-4f11-9339-a97400f7e70c@gmail.com","subject":"Re: [PATCH v2 0/2] add-p P fixups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-23T16:52:39Z","receivedAt":"2024-07-23T16:52:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Hi Rubén\n>\n> These changes themselves look sensible. As rj/add-p-pager is only in\n> seen I assume you'll re-roll with these squashed in once everyone is\n> happy?\n\nAh, good point. Throwing around \"oops, here is a fixup\" makes the\ndevelopment harder to follow for those watching from the sidelines.\nI try to keep what is in my tree on 'seen' fairly complete, but\nat some point we need to \"sync up\".\n\nThanks.\n"},{"id":"499203","messageId":"2333cb14-f020-451c-ad14-3f30edd152ec@gmail.com","threadId":"61825","inReplyTo":"62af789f-ca19-4f11-9339-a97400f7e70c@gmail.com","subject":"Re: [PATCH v2 0/2] add-p P fixups","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-23T22:08:05Z","receivedAt":"2024-07-23T22:08:08Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Tue, Jul 23, 2024 at 10:15:03AM +0100, Phillip Wood wrote:\n> Hi Rubén\n> \n> These changes themselves look sensible.\n\nGlad to hear that.\n\n> As rj/add-p-pager is only in seen I\n> assume you'll re-roll with these squashed in once everyone is happy?\n\nJunio has already integrated these changes into the branch he has in his\ntree, including a small change to the message to adjust it to his\ncomments, which I think is good. \n\nI hope that what we already have in Junio's tree is the final iteration\nof this long series and that we can let it settle before making further\nchanges. \n\n> \n> Best Wishes\n> \n> Phillip\n> \n> On 23/07/2024 01:39, Rubén Justo wrote:\n> > Rubén Justo (1):\n> > t3701: avoid one-shot export for shell functions\n> > pager: make wait_for_pager a no-op for \"cat\"\n> > \n> > pager.c | 3 +++\n> > t/t3701-add-interactive.sh | 6 +++++-\n> > 2 files changed, 8 insertions(+), 1 deletion(-)\n> > \n> > Range-diff against v1:\n> > 1: c3b8ebbae7 ! 1: 15fbf82fff t3701: avoid one-shot export for shell functions\n> > @@ Commit message\n> > VAR=VAL command args\n> > - it's a common way to define one-shot variables within the scope of\n> > + is a common way to set and export one-shot variables within the scope of\n> > executing a \"command\".\n> > However, when \"command\" is a function which in turn executes the\n> > @@ Commit message\n> > $ A=1 f\n> > A=\n> > + Note that POSIX is not specific about this behavior:\n> > +\n> > + http://www.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01\n> > +\n> > One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash\n> > 0.5.10.2-6, so we failed the test t3701:51; the \"git add -p\" being\n> > tested did not get our custom GIT_PAGER, which broke the test.\n> > 2: f45455f1ff ! 2: b87c3d96e4 pager: make wait_for_pager a no-op for \"cat\"\n> > @@ Commit message\n> > \"cat\" [*2*], then we return from `setup_pager()` silently without doing\n> > anything, allowing the output to go directly to the normal stdout.\n> > - Let's make the call to `wait_for_pager()` for these cases, or any other\n> > - future optimizations that may occur, also exit silently without doing\n> > - anything.\n> > + If `setup_pager()` avoids forking a pager, then when the client calls\n> > + the corresponding `wait_for_pager()`, we might fail trying to terminate\n> > + a process that wasn't started.\n> > +\n> > + One solution to avoid this problem could be to make the caller aware\n> > + that `setup_pager()` did nothing, so it could avoid calling\n> > + `wait_for_pager()`.\n> > +\n> > + However, let's avoid shifting that responsibility to the caller and\n> > + instead treat the call to `wait_for_pager()` as a no-op when we know we\n> > + haven't forked a pager.\n> > 1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,\n> > 2006-04-16)\n"},{"id":"499272","messageId":"5735bee3-0532-4894-b717-12a0bdcb9e84@gmail.com","threadId":"61825","inReplyTo":"2333cb14-f020-451c-ad14-3f30edd152ec@gmail.com","subject":"Re: [PATCH v2 0/2] add-p P fixups","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-24T15:21:53Z","receivedAt":"2024-07-24T15:22:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 23/07/2024 23:08, Rubén Justo wrote:\n> On Tue, Jul 23, 2024 at 10:15:03AM +0100, Phillip Wood wrote:\n> \n>> As rj/add-p-pager is only in seen I\n>> assume you'll re-roll with these squashed in once everyone is happy?\n> \n> Junio has already integrated these changes into the branch he has in his\n> tree, including a small change to the message to adjust it to his\n> comments, which I think is good.\n> \n> I hope that what we already have in Junio's tree is the final iteration\n> of this long series and that we can let it settle before making further\n> changes.\n\nThe resulting tree is good, but the history is not bisectable. You \nshould squash the fixups locally, updating the message of the fixed up \ncommit as needed and submit the result as the final version.\n\nBest Wishes\n\nPhillip\n\n>>\n>> Best Wishes\n>>\n>> Phillip\n>>\n>> On 23/07/2024 01:39, Rubén Justo wrote:\n>>> Rubén Justo (1):\n>>> t3701: avoid one-shot export for shell functions\n>>> pager: make wait_for_pager a no-op for \"cat\"\n>>>\n>>> pager.c | 3 +++\n>>> t/t3701-add-interactive.sh | 6 +++++-\n>>> 2 files changed, 8 insertions(+), 1 deletion(-)\n>>>\n>>> Range-diff against v1:\n>>> 1: c3b8ebbae7 ! 1: 15fbf82fff t3701: avoid one-shot export for shell functions\n>>> @@ Commit message\n>>> VAR=VAL command args\n>>> - it's a common way to define one-shot variables within the scope of\n>>> + is a common way to set and export one-shot variables within the scope of\n>>> executing a \"command\".\n>>> However, when \"command\" is a function which in turn executes the\n>>> @@ Commit message\n>>> $ A=1 f\n>>> A=\n>>> + Note that POSIX is not specific about this behavior:\n>>> +\n>>> + http://www.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01\n>>> +\n>>> One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash\n>>> 0.5.10.2-6, so we failed the test t3701:51; the \"git add -p\" being\n>>> tested did not get our custom GIT_PAGER, which broke the test.\n>>> 2: f45455f1ff ! 2: b87c3d96e4 pager: make wait_for_pager a no-op for \"cat\"\n>>> @@ Commit message\n>>> \"cat\" [*2*], then we return from `setup_pager()` silently without doing\n>>> anything, allowing the output to go directly to the normal stdout.\n>>> - Let's make the call to `wait_for_pager()` for these cases, or any other\n>>> - future optimizations that may occur, also exit silently without doing\n>>> - anything.\n>>> + If `setup_pager()` avoids forking a pager, then when the client calls\n>>> + the corresponding `wait_for_pager()`, we might fail trying to terminate\n>>> + a process that wasn't started.\n>>> +\n>>> + One solution to avoid this problem could be to make the caller aware\n>>> + that `setup_pager()` did nothing, so it could avoid calling\n>>> + `wait_for_pager()`.\n>>> +\n>>> + However, let's avoid shifting that responsibility to the caller and\n>>> + instead treat the call to `wait_for_pager()` as a no-op when we know we\n>>> + haven't forked a pager.\n>>> 1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,\n>>> 2006-04-16)\n"},{"id":"499277","messageId":"a25c37e2-fcfd-4a4c-890b-a85039ccef12@gmail.com","threadId":"61825","inReplyTo":"5735bee3-0532-4894-b717-12a0bdcb9e84@gmail.com","subject":"Re: [PATCH v2 0/2] add-p P fixups","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-24T16:12:18Z","receivedAt":"2024-07-24T16:12:21Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Wed, Jul 24, 2024 at 04:21:53PM +0100, phillip.wood123@gmail.com wrote:\n\n> > I hope that what we already have in Junio's tree is the final iteration\n> > of this long series and that we can let it settle before making further\n> > changes.\n> \n> The resulting tree is good, but the history is not bisectable. You should\n> squash the fixups locally, updating the message of the fixed up commit as\n> needed and submit the result as the final version.\n\nThat was my initial thought [*1*] when the problem with \"dash\n0.5.10.2-6\" appeared. \n\nJunio proposed [*2*] documenting the changes to address it as a separate\npatch, and I think it makes sense and it is valuable to capture the\nsituation this way in the history.\n\nRegarding the bisectability, I don't understand what stops from being\nbisectable. Except in a scenario with a shell like \"dash 0.5.10.2-6\"\nthere won't be any problem. And in one with it, which should be\nuncommon, the situation is well explained.\n\nSo, I dunno.\n\n 1.- 2b57479c-29c8-4a6e-b7b0-1309395cfbd9@gmail.com\n\n 2.- xmqq7cdd9l0m.fsf@gitster.g\n"},{"id":"499323","messageId":"97902c27-63c9-4537-8ebe-853ef0cb1d3b@gmail.com","threadId":"61825","inReplyTo":"a25c37e2-fcfd-4a4c-890b-a85039ccef12@gmail.com","subject":"Re: [PATCH v2 0/2] add-p P fixups","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-25T09:45:04Z","receivedAt":"2024-07-25T09:45:08Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 24/07/2024 17:12, Rubén Justo wrote:\n> On Wed, Jul 24, 2024 at 04:21:53PM +0100, phillip.wood123@gmail.com wrote:\n> \n> That was my initial thought [*1*] when the problem with \"dash\n> 0.5.10.2-6\" appeared.\n> \n> Junio proposed [*2*] documenting the changes to address it as a separate\n> patch, and I think it makes sense and it is valuable to capture the\n> situation this way in the history.\n\nWe normally avoid merging commits that are known to break our ci. We can \nadd a comment about the dash problem to the commit message when this \nfixup is squashed. Also the problem is now documented in \nDocumentation/CodingGuidelines which is more likely to be read by other \ncontributors.\n\n> Regarding the bisectability, I don't understand what stops from being\n> bisectable. Except in a scenario with a shell like \"dash 0.5.10.2-6\"\n> there won't be any problem.\n\nBut we know that shell is in use in a popular Linux distribution so it \nis a problem for those users.\n\nBest Wishes\n\nPhillip\n\n> And in one with it, which should be\n> uncommon, the situation is well explained.\n> \n> So, I dunno.\n> \n> 1.- 2b57479c-29c8-4a6e-b7b0-1309395cfbd9@gmail.com\n> \n> 2.- xmqq7cdd9l0m.fsf@gitster.g\n"},{"id":"499332","messageId":"88286ad9-eab7-4461-a407-898737faa6a1@gmail.com","threadId":"61825","inReplyTo":"97902c27-63c9-4537-8ebe-853ef0cb1d3b@gmail.com","subject":"Re: [PATCH v2 0/2] add-p P fixups","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-25T12:16:31Z","receivedAt":"2024-07-25T12:16:34Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Thu, Jul 25, 2024 at 10:45:04AM +0100, Phillip Wood wrote:\n> Hi Rubén\n> \n> On 24/07/2024 17:12, Rubén Justo wrote:\n> > On Wed, Jul 24, 2024 at 04:21:53PM +0100, phillip.wood123@gmail.com wrote:\n> > \n> > That was my initial thought [*1*] when the problem with \"dash\n> > 0.5.10.2-6\" appeared.\n> > \n> > Junio proposed [*2*] documenting the changes to address it as a separate\n> > patch, and I think it makes sense and it is valuable to capture the\n> > situation this way in the history.\n> \n> We normally avoid merging commits that are known to break our ci.\n\nFair point. I'll reroll and let Junio decide, or ignore ;).\n\n> We can add\n> a comment about the dash problem to the commit message when this fixup is\n> squashed. Also the problem is now documented in\n> Documentation/CodingGuidelines which is more likely to be read by other\n> contributors.\n> \n> > Regarding the bisectability, I don't understand what stops from being\n> > bisectable. Except in a scenario with a shell like \"dash 0.5.10.2-6\"\n> > there won't be any problem.\n> \n> But we know that shell is in use in a popular Linux distribution so it is a\n> problem for those users.\n\nIt's not an excuse; I just wanted to point out that Ubuntu 20.04 was\nupdated with a version of \"dash\" that doesn't have the issue we're\nseeing in our CI for quite some time now. The version I mentioned in\nmessage [1/2] of this thread: dash 0.5.12-9. \n\n> \n> Best Wishes\n> \n> Phillip\n> \n> > And in one with it, which should be\n> > uncommon, the situation is well explained.\n> > \n> > So, I dunno.\n> > \n> > 1.- 2b57479c-29c8-4a6e-b7b0-1309395cfbd9@gmail.com\n> > \n> > 2.- xmqq7cdd9l0m.fsf@gitster.g\n\nThank you for trying to make Git and its history better.\n"},{"id":"499336","messageId":"76936fb1-446d-455f-b4e7-6e24dda3c17d@gmail.com","threadId":"61825","inReplyTo":"88286ad9-eab7-4461-a407-898737faa6a1@gmail.com","subject":"[PATCH 0/4] squash fixups in rj/add-p-pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-25T13:42:07Z","receivedAt":"2024-07-25T13:42:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Here is the series that squashes the fixups in rj/add-p-pager.\n\nI don't have a strong preference for this over what's already in\nrj/add-p-pager, but let's go through the changes Phillip has suggested,\neven if it's just to archive them in the list. \n\nThanks.\n\nRubén Justo (4):\n add-patch: test for 'p' command\n pager: do not close fd 2 unnecessarily\n pager: introduce wait_for_pager\n add-patch: render hunks through the pager\n\n add-patch.c | 18 +++++++++++---\n pager.c | 48 ++++++++++++++++++++++++++++++++++----\n pager.h | 1 +\n t/t3701-add-interactive.sh | 48 ++++++++++++++++++++++++++++++++++++++\n 4 files changed, 107 insertions(+), 8 deletions(-)\n\nRange-diff:\n1: 5fdfd2f3bd = 1: 9f358c6d69 add-patch: test for 'p' command\n2: 506f457e48 = 2: f45a7ca9b2 pager: do not close fd 2 unnecessarily\n3: b29c59e3d2 ! 3: 9d7a50e531 pager: introduce wait_for_pager\n @@ Commit message\n In the interactive commands (i.e.: add -p) we want to use the pager for\n some output, while maintaining the interaction with the user.\n \n - Modify the pager machinery so that we can use setup_pager and, once\n + Modify the pager machinery so that we can use `setup_pager()` and, once\n we've finished sending the desired output for the pager, wait for the\n - pager termination using a new function wait_for_pager. Make this\n + pager termination using a new function `wait_for_pager()`. Make this\n function reset the pager machinery before returning.\n \n + One specific point to note is that we avoid forking the pager in\n + `setup_pager()` if the configured pager is an empty string [*1*] or\n + simply \"cat\" [*2*]. In these cases, `setup_pager()` does nothing and\n + therefore `wait_for_pager()` should not be called.\n +\n + We could modify `setup_pager()` to return an indication of these\n + situations, so we could avoid calling `wait_for_pager()`.\n +\n + However, let's avoid transferring that responsibility to the caller and\n + instead treat the call to `wait_for_pager()` as a no-op when we know we\n + haven't forked the pager.\n +\n + 1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,\n + 2006-04-16)\n +\n + 2.- caef71a535 (Do not fork PAGER=cat, 2006-04-16)\n +\n Signed-off-by: Rubén Justo \n - Signed-off-by: Junio C Hamano \n \n ## pager.c ##\n @@ pager.c: int pager_use_color = 1;\n @@ pager.c: static void wait_for_pager_atexit(void)\n +\n +void wait_for_pager(void)\n +{\n ++\tif (old_fd1 == -1)\n ++\t\treturn;\n ++\n +\tfinish_pager();\n +\tsigchain_pop_common();\n +\tunsetenv(\"GIT_PAGER_IN_USE\");\n4: 6bc52a5543 ! 4: 6f4990c0d4 add-patch: render hunks through the pager\n @@ Commit message\n this limit.\n \n Signed-off-by: Rubén Justo \n - Signed-off-by: Junio C Hamano \n \n ## add-patch.c ##\n @@\n @@ t/t3701-add-interactive.sh: test_expect_success 'print again the hunk' '\n +\ttest_when_finished \"rm -f huge_file; git reset\" &&\n +\tprintf \"\\n%2500000s\" Y >huge_file &&\n +\tgit add -N huge_file &&\n -+\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n ++\ttest_write_lines P q | (\n ++\t\tGIT_PAGER=\"head -n 1\" &&\n ++\t\texport GIT_PAGER &&\n ++\t\ttest_terminal git add -p\n ++\t)\n +'\n +\n test_expect_success 'split hunk \"add -p (edit)\"' '\n5: b7637a9f21 < -: ---------- t3701: avoid one-shot export for shell functions\n6: 4b53ff8c0e < -: ---------- pager: make wait_for_pager a no-op for \"cat\"\n\nbase-commit: a7dae3bdc8b516d36f630b12bb01e853a667e0d9\n-- \n2.46.0.rc0.4.g6f4990c0d4\n"},{"id":"499337","messageId":"57073ffc-65ab-48eb-9517-964a6f5141fd@gmail.com","threadId":"61825","inReplyTo":"76936fb1-446d-455f-b4e7-6e24dda3c17d@gmail.com","subject":"[PATCH 1/4] add-patch: test for 'p' command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-25T13:44:16Z","receivedAt":"2024-07-25T13:44:19Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Add a test for the 'p' command, which was introduced in 66c14ab592\n(add-patch: introduce 'p' in interactive-patch, 2024-03-29).\n\nSigned-off-by: Rubén Justo \nSigned-off-by: Junio C Hamano \n---\n t/t3701-add-interactive.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 5d78868ac1..6daf3a6be0 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -575,6 +575,22 @@ test_expect_success 'navigate to hunk via regex / pattern' '\n \ttest_cmp expect actual.trimmed\n '\n \n+test_expect_success 'print again the hunk' '\n+\ttest_when_finished \"git reset\" &&\n+\ttr _ \" \" >expect <<-EOF &&\n+\t+15\n+\t 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@\n+\t 10\n+\t+15\n+\t 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\tEOF\n+\ttest_write_lines s y g 1 p | git add -p >actual &&\n+\ttail -n 7 actual.trimmed &&\n+\ttest_cmp expect actual.trimmed\n+'\n+\n test_expect_success 'split hunk \"add -p (edit)\"' '\n \t# Split, say Edit and do nothing. Then:\n \t#\n-- \n2.46.0.rc0.4.g6f4990c0d4\n"},{"id":"499338","messageId":"3fcb30a2-8a97-4b34-aa3d-b19ee54dc97e@gmail.com","threadId":"61825","inReplyTo":"76936fb1-446d-455f-b4e7-6e24dda3c17d@gmail.com","subject":"[PATCH 2/4] pager: do not close fd 2 unnecessarily","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-25T13:44:27Z","receivedAt":"2024-07-25T13:44:30Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"We send errors to the pager since 61b80509e3 (sending errors to stdout\nunder $PAGER, 2008-02-16).\n\nIn a8335024c2 (pager: do not dup2 stderr if it is already redirected,\n2008-12-15) an exception was introduced to avoid redirecting stderr if\nit is not connected to a terminal.\n\nIn such exceptional cases, the close(STDERR_FILENO) we're doing in\nclose_pager_fds, is unnecessary.\n\nFurthermore, in a subsequent commit we're going to introduce changes\nthat will involve using close_pager_fds multiple times.\n\nWith this in mind, controlling when we want to close stderr, become\nsensible.\n\nLet's close(STDERR_FILENO) only when necessary, and pave the way for the\nupcoming changes.\n\nSigned-off-by: Rubén Justo \nSigned-off-by: Junio C Hamano \n---\n pager.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex be6f4ee59f..251adfc2ad 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -14,6 +14,7 @@ int pager_use_color = 1;\n \n static struct child_process pager_process;\n static char *pager_program;\n+static int close_fd2;\n \n /* Is the value coming back from term_columns() just a guess? */\n static int term_columns_guessed;\n@@ -23,7 +24,8 @@ static void close_pager_fds(void)\n {\n \t/* signal EOF to pager */\n \tclose(1);\n-\tclose(2);\n+\tif (close_fd2)\n+\t\tclose(2);\n }\n \n static void wait_for_pager_atexit(void)\n@@ -141,8 +143,10 @@ void setup_pager(void)\n \n \t/* original process continues, but writes to the pipe */\n \tdup2(pager_process.in, 1);\n-\tif (isatty(2))\n+\tif (isatty(2)) {\n+\t\tclose_fd2 = 1;\n \t\tdup2(pager_process.in, 2);\n+\t}\n \tclose(pager_process.in);\n \n \t/* this makes sure that the parent terminates after the pager */\n-- \n2.46.0.rc0.4.g6f4990c0d4\n"},{"id":"499339","messageId":"934b8247-a5e7-4919-8ccb-08ceb23c03ff@gmail.com","threadId":"61825","inReplyTo":"76936fb1-446d-455f-b4e7-6e24dda3c17d@gmail.com","subject":"[PATCH 3/4] pager: introduce wait_for_pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-25T13:44:39Z","receivedAt":"2024-07-25T13:44:42Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,\n2006-02-28) we have the machinery to send our output to a pager.\n\nThat machinery, once set up, does not allow us to regain the original\nstdio streams.\n\nIn the interactive commands (i.e.: add -p) we want to use the pager for\nsome output, while maintaining the interaction with the user.\n\nModify the pager machinery so that we can use `setup_pager()` and, once\nwe've finished sending the desired output for the pager, wait for the\npager termination using a new function `wait_for_pager()`. Make this\nfunction reset the pager machinery before returning.\n\nOne specific point to note is that we avoid forking the pager in\n`setup_pager()` if the configured pager is an empty string [*1*] or\nsimply \"cat\" [*2*]. In these cases, `setup_pager()` does nothing and\ntherefore `wait_for_pager()` should not be called.\n\nWe could modify `setup_pager()` to return an indication of these\nsituations, so we could avoid calling `wait_for_pager()`.\n\nHowever, let's avoid transferring that responsibility to the caller and\ninstead treat the call to `wait_for_pager()` as a no-op when we know we\nhaven't forked the pager.\n\n 1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,\n 2006-04-16)\n\n 2.- caef71a535 (Do not fork PAGER=cat, 2006-04-16)\n\nSigned-off-by: Rubén Justo \n---\n pager.c | 46 ++++++++++++++++++++++++++++++++++++++++------\n pager.h | 1 +\n 2 files changed, 41 insertions(+), 6 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 251adfc2ad..896f40fcd2 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -14,7 +14,7 @@ int pager_use_color = 1;\n \n static struct child_process pager_process;\n static char *pager_program;\n-static int close_fd2;\n+static int old_fd1 = -1, old_fd2 = -1;\n \n /* Is the value coming back from term_columns() just a guess? */\n static int term_columns_guessed;\n@@ -24,11 +24,11 @@ static void close_pager_fds(void)\n {\n \t/* signal EOF to pager */\n \tclose(1);\n-\tif (close_fd2)\n+\tif (old_fd2 != -1)\n \t\tclose(2);\n }\n \n-static void wait_for_pager_atexit(void)\n+static void finish_pager(void)\n {\n \tfflush(stdout);\n \tfflush(stderr);\n@@ -36,8 +36,37 @@ static void wait_for_pager_atexit(void)\n \tfinish_command(&pager_process);\n }\n \n+static void wait_for_pager_atexit(void)\n+{\n+\tif (old_fd1 == -1)\n+\t\treturn;\n+\n+\tfinish_pager();\n+}\n+\n+void wait_for_pager(void)\n+{\n+\tif (old_fd1 == -1)\n+\t\treturn;\n+\n+\tfinish_pager();\n+\tsigchain_pop_common();\n+\tunsetenv(\"GIT_PAGER_IN_USE\");\n+\tdup2(old_fd1, 1);\n+\tclose(old_fd1);\n+\told_fd1 = -1;\n+\tif (old_fd2 != -1) {\n+\t\tdup2(old_fd2, 2);\n+\t\tclose(old_fd2);\n+\t\told_fd2 = -1;\n+\t}\n+}\n+\n static void wait_for_pager_signal(int signo)\n {\n+\tif (old_fd1 == -1)\n+\t\treturn;\n+\n \tclose_pager_fds();\n \tfinish_command_in_signal(&pager_process);\n \tsigchain_pop(signo);\n@@ -113,6 +142,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)\n \n void setup_pager(void)\n {\n+\tstatic int once = 0;\n \tconst char *pager = git_pager(isatty(1));\n \n \tif (!pager)\n@@ -142,16 +172,20 @@ void setup_pager(void)\n \t\tdie(\"unable to execute pager '%s'\", pager);\n \n \t/* original process continues, but writes to the pipe */\n+\told_fd1 = dup(1);\n \tdup2(pager_process.in, 1);\n \tif (isatty(2)) {\n-\t\tclose_fd2 = 1;\n+\t\told_fd2 = dup(2);\n \t\tdup2(pager_process.in, 2);\n \t}\n \tclose(pager_process.in);\n \n-\t/* this makes sure that the parent terminates after the pager */\n \tsigchain_push_common(wait_for_pager_signal);\n-\tatexit(wait_for_pager_atexit);\n+\n+\tif (!once) {\n+\t\tonce++;\n+\t\tatexit(wait_for_pager_atexit);\n+\t}\n }\n \n int pager_in_use(void)\ndiff --git a/pager.h b/pager.h\nindex b77433026d..103ecac476 100644\n--- a/pager.h\n+++ b/pager.h\n@@ -5,6 +5,7 @@ struct child_process;\n \n const char *git_pager(int stdout_is_tty);\n void setup_pager(void);\n+void wait_for_pager(void);\n int pager_in_use(void);\n int term_columns(void);\n void term_clear_line(void);\n-- \n2.46.0.rc0.4.g6f4990c0d4\n"},{"id":"499340","messageId":"0c0cff56-d44b-4a95-809e-afdd219539aa@gmail.com","threadId":"61825","inReplyTo":"76936fb1-446d-455f-b4e7-6e24dda3c17d@gmail.com","subject":"[PATCH 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-25T13:44:52Z","receivedAt":"2024-07-25T13:44:55Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Make the print command trigger the pager when invoked using a capital\n'P', to make it easier for the user to review long hunks.\n\nNote that if the PAGER ends unexpectedly before we've been able to send\nthe payload, perhaps because the user is not interested in the whole\nthing, we might receive a SIGPIPE, which would abruptly and unexpectedly\nterminate the interactive session for the user.\n\nTherefore, we need to ignore a possible SIGPIPE signal. Add a test for\nthis, in addition to the test for normal operation.\n\nFor the SIGPIPE test, we need to make sure that we completely fill the\noperating system's buffer, otherwise we might not trigger the SIGPIPE\nsignal. The normal size of this buffer in different OSs varies from a\nfew KBs to 1MB. Use a payload large enough to guarantee that we exceed\nthis limit.\n\nSigned-off-by: Rubén Justo \n---\n add-patch.c | 18 +++++++++++++++---\n t/t3701-add-interactive.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 47 insertions(+), 3 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 6e176cd21a..f2c76b7d83 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -7,9 +7,11 @@\n #include \"environment.h\"\n #include \"gettext.h\"\n #include \"object-name.h\"\n+#include \"pager.h\"\n #include \"read-cache-ll.h\"\n #include \"repository.h\"\n #include \"strbuf.h\"\n+#include \"sigchain.h\"\n #include \"run-command.h\"\n #include \"strvec.h\"\n #include \"pathspec.h\"\n@@ -1391,7 +1393,7 @@ N_(\"j - leave this hunk undecided, see next undecided hunk\\n\"\n \"/ - search for a hunk matching the given regex\\n\"\n \"s - split the current hunk into smaller hunks\\n\"\n \"e - manually edit the current hunk\\n\"\n- \"p - print the current hunk\\n\"\n+ \"p - print the current hunk, 'P' to use the pager\\n\"\n \"? - print help\\n\");\n \n static int patch_update_file(struct add_p_state *s,\n@@ -1402,7 +1404,7 @@ static int patch_update_file(struct add_p_state *s,\n \tstruct hunk *hunk;\n \tchar ch;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tint colored = !!s->colored.len, quit = 0;\n+\tint colored = !!s->colored.len, quit = 0, use_pager = 0;\n \tenum prompt_mode_type prompt_mode_type;\n \tenum {\n \t\tALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,\n@@ -1452,9 +1454,18 @@ static int patch_update_file(struct add_p_state *s,\n \t\tstrbuf_reset(&s->buf);\n \t\tif (file_diff->hunk_nr) {\n \t\t\tif (rendered_hunk_index != hunk_index) {\n+\t\t\t\tif (use_pager) {\n+\t\t\t\t\tsetup_pager();\n+\t\t\t\t\tsigchain_push(SIGPIPE, SIG_IGN);\n+\t\t\t\t}\n \t\t\t\trender_hunk(s, hunk, 0, colored, &s->buf);\n \t\t\t\tfputs(s->buf.buf, stdout);\n \t\t\t\trendered_hunk_index = hunk_index;\n+\t\t\t\tif (use_pager) {\n+\t\t\t\t\tsigchain_pop(SIGPIPE);\n+\t\t\t\t\twait_for_pager();\n+\t\t\t\t\tuse_pager = 0;\n+\t\t\t\t}\n \t\t\t}\n \n \t\t\tstrbuf_reset(&s->buf);\n@@ -1675,8 +1686,9 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\thunk->use = USE_HUNK;\n \t\t\t\tgoto soft_increment;\n \t\t\t}\n-\t\t} else if (s->answer.buf[0] == 'p') {\n+\t\t} else if (ch == 'p') {\n \t\t\trendered_hunk_index = -1;\n+\t\t\tuse_pager = (s->answer.buf[0] == 'P') ? 1 : 0;\n \t\t} else if (s->answer.buf[0] == '?') {\n \t\t\tconst char *p = _(help_patch_remainder), *eol = p;\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 6daf3a6be0..1b8617e0c1 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -591,6 +591,38 @@ test_expect_success 'print again the hunk' '\n \ttest_cmp expect actual.trimmed\n '\n \n+test_expect_success TTY 'print again the hunk (PAGER)' '\n+\ttest_when_finished \"git reset\" &&\n+\tcat >expect <<-EOF &&\n+\t+15\n+\t 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? PAGER @@ -1,2 +1,3 @@\n+\tPAGER 10\n+\tPAGER +15\n+\tPAGER 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? \n+\tEOF\n+\ttest_write_lines s y g 1 P |\n+\t(\n+\t\tGIT_PAGER=\"sed s/^/PAGER\\ /\" &&\n+\t\texport GIT_PAGER &&\n+\t\ttest_terminal git add -p >actual\n+\t) &&\n+\ttail -n 7 actual.trimmed &&\n+\ttest_cmp expect actual.trimmed\n+'\n+\n+test_expect_success TTY 'P handles SIGPIPE when writing to pager' '\n+\ttest_when_finished \"rm -f huge_file; git reset\" &&\n+\tprintf \"\\n%2500000s\" Y >huge_file &&\n+\tgit add -N huge_file &&\n+\ttest_write_lines P q | (\n+\t\tGIT_PAGER=\"head -n 1\" &&\n+\t\texport GIT_PAGER &&\n+\t\ttest_terminal git add -p\n+\t)\n+'\n+\n test_expect_success 'split hunk \"add -p (edit)\"' '\n \t# Split, say Edit and do nothing. Then:\n \t#\n-- \n2.46.0.rc0.4.g6f4990c0d4\n"},{"id":"499348","messageId":"xmqqcyn1lcjo.fsf@gitster.g","threadId":"61825","inReplyTo":"97902c27-63c9-4537-8ebe-853ef0cb1d3b@gmail.com","subject":"Re: [PATCH v2 0/2] add-p P fixups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-25T15:24:43Z","receivedAt":"2024-07-25T15:24:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> ... We\n> can add a comment about the dash problem to the commit message when\n> this fixup is squashed. Also the problem is now documented in\n> Documentation/CodingGuidelines which is more likely to be read by\n> other contributors.\n\nThat is a good thing to take into consideration, indeed.\n\n"},{"id":"499361","messageId":"24e83a0f-b0c8-4cd5-b321-1d7702b844ce@gmail.com","threadId":"61825","inReplyTo":"xmqqcyn1lcjo.fsf@gitster.g","subject":"Re* [PATCH v2 0/2] add-p P fixups","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-25T16:41:17Z","receivedAt":"2024-07-25T16:41:20Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 7/25/24 5:24 PM, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> ... We\n>> can add a comment about the dash problem to the commit message when\n>> this fixup is squashed. Also the problem is now documented in\n>> Documentation/CodingGuidelines which is more likely to be read by\n>> other contributors.\n> \n> That is a good thing to take into consideration, indeed.\n\nRubén Justo (2):\n pager: introduce wait_for_pager\n add-patch: render hunks through the pager\n\n add-patch.c | 18 ++++++++++++---\n pager.c | 46 +++++++++++++++++++++++++++++++++-----\n pager.h | 1 +\n t/t3701-add-interactive.sh | 32 ++++++++++++++++++++++++++\n 4 files changed, 88 insertions(+), 9 deletions(-)\n\nRange-diff:\n1: b29c59e3d2 ! 1: 8a6116ef86 pager: introduce wait_for_pager\n @@ Commit message\n In the interactive commands (i.e.: add -p) we want to use the pager for\n some output, while maintaining the interaction with the user.\n \n - Modify the pager machinery so that we can use setup_pager and, once\n + Modify the pager machinery so that we can use `setup_pager()` and, once\n we've finished sending the desired output for the pager, wait for the\n - pager termination using a new function wait_for_pager. Make this\n + pager termination using a new function `wait_for_pager()`. Make this\n function reset the pager machinery before returning.\n \n + One specific point to note is that we avoid forking the pager in\n + `setup_pager()` if the configured pager is an empty string [*1*] or\n + simply \"cat\" [*2*]. In these cases, `setup_pager()` does nothing and\n + therefore `wait_for_pager()` should not be called.\n +\n + We could modify `setup_pager()` to return an indication of these\n + situations, so we could avoid calling `wait_for_pager()`.\n +\n + However, let's avoid transferring that responsibility to the caller and\n + instead treat the call to `wait_for_pager()` as a no-op when we know we\n + haven't forked the pager.\n +\n + 1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,\n + 2006-04-16)\n +\n + 2.- caef71a535 (Do not fork PAGER=cat, 2006-04-16)\n +\n Signed-off-by: Rubén Justo \n - Signed-off-by: Junio C Hamano \n \n ## pager.c ##\n @@ pager.c: int pager_use_color = 1;\n @@ pager.c: static void wait_for_pager_atexit(void)\n +\n +void wait_for_pager(void)\n +{\n ++\tif (old_fd1 == -1)\n ++\t\treturn;\n ++\n +\tfinish_pager();\n +\tsigchain_pop_common();\n +\tunsetenv(\"GIT_PAGER_IN_USE\");\n2: 6bc52a5543 ! 2: 980187854a add-patch: render hunks through the pager\n @@ Commit message\n few KBs to 1MB. Use a payload large enough to guarantee that we exceed\n this limit.\n \n + For the tests, avoid the common construct to set and export one-shot\n + variables within the scope of a command:\n +\n + VAR=VAL command args\n +\n + It happens that when \"command\" is a shell function that in turn executes\n + a \"command\", the behavior with \"VAR\" varies depending on the shell:\n +\n + ** Bash 5.2.21 **\n +\n + $ f () { bash -c 'echo A=$A'; }\n + $ A=1 f\n + A=1\n +\n + ** dash 0.5.12-9 **\n +\n + $ f () { bash -c 'echo A=$A'; }\n + $ A=1 f\n + A=1\n +\n + ** dash 0.5.10.2-6 **\n +\n + $ f () { bash -c 'echo A=$A'; }\n + $ A=1 f\n + A=\n +\n + POSIX explicitly says the effect of this construct is unspecified.\n +\n + One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash\n + 0.5.10.2-6, so avoid using the construct and use a subshell instead.\n +\n Signed-off-by: Rubén Justo \n - Signed-off-by: Junio C Hamano \n \n ## add-patch.c ##\n @@\n @@ t/t3701-add-interactive.sh: test_expect_success 'print again the hunk' '\n +\ttest_when_finished \"rm -f huge_file; git reset\" &&\n +\tprintf \"\\n%2500000s\" Y >huge_file &&\n +\tgit add -N huge_file &&\n -+\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n ++\ttest_write_lines P q | (\n ++\t\tGIT_PAGER=\"head -n 1\" &&\n ++\t\texport GIT_PAGER &&\n ++\t\ttest_terminal git add -p\n ++\t)\n +'\n +\n test_expect_success 'split hunk \"add -p (edit)\"' '\n3: b7637a9f21 < -: ---------- t3701: avoid one-shot export for shell functions\n4: 4b53ff8c0e < -: ---------- pager: make wait_for_pager a no-op for \"cat\"\n\nbase-commit: 506f457e489b2097e2d4fc5ceffd6e242502b2bd\n-- \n2.46.0.rc0.4.g6f4990c0d4\n"},{"id":"499362","messageId":"2ec2a894-a173-469c-8211-ac5f49a82a27@gmail.com","threadId":"61825","inReplyTo":"24e83a0f-b0c8-4cd5-b321-1d7702b844ce@gmail.com","subject":"[PATCH 1/2] pager: introduce wait_for_pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-25T16:43:43Z","receivedAt":"2024-07-25T16:43:45Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,\n2006-02-28) we have the machinery to send our output to a pager.\n\nThat machinery, once set up, does not allow us to regain the original\nstdio streams.\n\nIn the interactive commands (i.e.: add -p) we want to use the pager for\nsome output, while maintaining the interaction with the user.\n\nModify the pager machinery so that we can use `setup_pager()` and, once\nwe've finished sending the desired output for the pager, wait for the\npager termination using a new function `wait_for_pager()`. Make this\nfunction reset the pager machinery before returning.\n\nOne specific point to note is that we avoid forking the pager in\n`setup_pager()` if the configured pager is an empty string [*1*] or\nsimply \"cat\" [*2*]. In these cases, `setup_pager()` does nothing and\ntherefore `wait_for_pager()` should not be called.\n\nWe could modify `setup_pager()` to return an indication of these\nsituations, so we could avoid calling `wait_for_pager()`.\n\nHowever, let's avoid transferring that responsibility to the caller and\ninstead treat the call to `wait_for_pager()` as a no-op when we know we\nhaven't forked the pager.\n\n 1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,\n 2006-04-16)\n\n 2.- caef71a535 (Do not fork PAGER=cat, 2006-04-16)\n\nSigned-off-by: Rubén Justo \n---\n pager.c | 46 ++++++++++++++++++++++++++++++++++++++++------\n pager.h | 1 +\n 2 files changed, 41 insertions(+), 6 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 251adfc2ad..896f40fcd2 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -14,7 +14,7 @@ int pager_use_color = 1;\n \n static struct child_process pager_process;\n static char *pager_program;\n-static int close_fd2;\n+static int old_fd1 = -1, old_fd2 = -1;\n \n /* Is the value coming back from term_columns() just a guess? */\n static int term_columns_guessed;\n@@ -24,11 +24,11 @@ static void close_pager_fds(void)\n {\n \t/* signal EOF to pager */\n \tclose(1);\n-\tif (close_fd2)\n+\tif (old_fd2 != -1)\n \t\tclose(2);\n }\n \n-static void wait_for_pager_atexit(void)\n+static void finish_pager(void)\n {\n \tfflush(stdout);\n \tfflush(stderr);\n@@ -36,8 +36,37 @@ static void wait_for_pager_atexit(void)\n \tfinish_command(&pager_process);\n }\n \n+static void wait_for_pager_atexit(void)\n+{\n+\tif (old_fd1 == -1)\n+\t\treturn;\n+\n+\tfinish_pager();\n+}\n+\n+void wait_for_pager(void)\n+{\n+\tif (old_fd1 == -1)\n+\t\treturn;\n+\n+\tfinish_pager();\n+\tsigchain_pop_common();\n+\tunsetenv(\"GIT_PAGER_IN_USE\");\n+\tdup2(old_fd1, 1);\n+\tclose(old_fd1);\n+\told_fd1 = -1;\n+\tif (old_fd2 != -1) {\n+\t\tdup2(old_fd2, 2);\n+\t\tclose(old_fd2);\n+\t\told_fd2 = -1;\n+\t}\n+}\n+\n static void wait_for_pager_signal(int signo)\n {\n+\tif (old_fd1 == -1)\n+\t\treturn;\n+\n \tclose_pager_fds();\n \tfinish_command_in_signal(&pager_process);\n \tsigchain_pop(signo);\n@@ -113,6 +142,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)\n \n void setup_pager(void)\n {\n+\tstatic int once = 0;\n \tconst char *pager = git_pager(isatty(1));\n \n \tif (!pager)\n@@ -142,16 +172,20 @@ void setup_pager(void)\n \t\tdie(\"unable to execute pager '%s'\", pager);\n \n \t/* original process continues, but writes to the pipe */\n+\told_fd1 = dup(1);\n \tdup2(pager_process.in, 1);\n \tif (isatty(2)) {\n-\t\tclose_fd2 = 1;\n+\t\told_fd2 = dup(2);\n \t\tdup2(pager_process.in, 2);\n \t}\n \tclose(pager_process.in);\n \n-\t/* this makes sure that the parent terminates after the pager */\n \tsigchain_push_common(wait_for_pager_signal);\n-\tatexit(wait_for_pager_atexit);\n+\n+\tif (!once) {\n+\t\tonce++;\n+\t\tatexit(wait_for_pager_atexit);\n+\t}\n }\n \n int pager_in_use(void)\ndiff --git a/pager.h b/pager.h\nindex b77433026d..103ecac476 100644\n--- a/pager.h\n+++ b/pager.h\n@@ -5,6 +5,7 @@ struct child_process;\n \n const char *git_pager(int stdout_is_tty);\n void setup_pager(void);\n+void wait_for_pager(void);\n int pager_in_use(void);\n int term_columns(void);\n void term_clear_line(void);\n-- \n2.46.0.rc0.4.g6f4990c0d4\n"},{"id":"499363","messageId":"38e190de-cbe4-4f75-acdb-fe566e541179@gmail.com","threadId":"61825","inReplyTo":"24e83a0f-b0c8-4cd5-b321-1d7702b844ce@gmail.com","subject":"[PATCH 2/2] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-25T16:43:45Z","receivedAt":"2024-07-25T16:43:48Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Make the print command trigger the pager when invoked using a capital\n'P', to make it easier for the user to review long hunks.\n\nNote that if the PAGER ends unexpectedly before we've been able to send\nthe payload, perhaps because the user is not interested in the whole\nthing, we might receive a SIGPIPE, which would abruptly and unexpectedly\nterminate the interactive session for the user.\n\nTherefore, we need to ignore a possible SIGPIPE signal. Add a test for\nthis, in addition to the test for normal operation.\n\nFor the SIGPIPE test, we need to make sure that we completely fill the\noperating system's buffer, otherwise we might not trigger the SIGPIPE\nsignal. The normal size of this buffer in different OSs varies from a\nfew KBs to 1MB. Use a payload large enough to guarantee that we exceed\nthis limit.\n\nFor the tests, avoid the common construct to set and export one-shot\nvariables within the scope of a command:\n\n VAR=VAL command args\n\nIt happens that when \"command\" is a shell function that in turn executes\na \"command\", the behavior with \"VAR\" varies depending on the shell:\n\n ** Bash 5.2.21 **\n\n $ f () { bash -c 'echo A=$A'; }\n $ A=1 f\n A=1\n\n ** dash 0.5.12-9 **\n\n $ f () { bash -c 'echo A=$A'; }\n $ A=1 f\n A=1\n\n ** dash 0.5.10.2-6 **\n\n $ f () { bash -c 'echo A=$A'; }\n $ A=1 f\n A=\n\nPOSIX explicitly says the effect of this construct is unspecified.\n\nOne of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash\n0.5.10.2-6, so avoid using the construct and use a subshell instead.\n\nSigned-off-by: Rubén Justo \n---\n add-patch.c | 18 +++++++++++++++---\n t/t3701-add-interactive.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 47 insertions(+), 3 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 6e176cd21a..f2c76b7d83 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -7,9 +7,11 @@\n #include \"environment.h\"\n #include \"gettext.h\"\n #include \"object-name.h\"\n+#include \"pager.h\"\n #include \"read-cache-ll.h\"\n #include \"repository.h\"\n #include \"strbuf.h\"\n+#include \"sigchain.h\"\n #include \"run-command.h\"\n #include \"strvec.h\"\n #include \"pathspec.h\"\n@@ -1391,7 +1393,7 @@ N_(\"j - leave this hunk undecided, see next undecided hunk\\n\"\n \"/ - search for a hunk matching the given regex\\n\"\n \"s - split the current hunk into smaller hunks\\n\"\n \"e - manually edit the current hunk\\n\"\n- \"p - print the current hunk\\n\"\n+ \"p - print the current hunk, 'P' to use the pager\\n\"\n \"? - print help\\n\");\n \n static int patch_update_file(struct add_p_state *s,\n@@ -1402,7 +1404,7 @@ static int patch_update_file(struct add_p_state *s,\n \tstruct hunk *hunk;\n \tchar ch;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tint colored = !!s->colored.len, quit = 0;\n+\tint colored = !!s->colored.len, quit = 0, use_pager = 0;\n \tenum prompt_mode_type prompt_mode_type;\n \tenum {\n \t\tALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,\n@@ -1452,9 +1454,18 @@ static int patch_update_file(struct add_p_state *s,\n \t\tstrbuf_reset(&s->buf);\n \t\tif (file_diff->hunk_nr) {\n \t\t\tif (rendered_hunk_index != hunk_index) {\n+\t\t\t\tif (use_pager) {\n+\t\t\t\t\tsetup_pager();\n+\t\t\t\t\tsigchain_push(SIGPIPE, SIG_IGN);\n+\t\t\t\t}\n \t\t\t\trender_hunk(s, hunk, 0, colored, &s->buf);\n \t\t\t\tfputs(s->buf.buf, stdout);\n \t\t\t\trendered_hunk_index = hunk_index;\n+\t\t\t\tif (use_pager) {\n+\t\t\t\t\tsigchain_pop(SIGPIPE);\n+\t\t\t\t\twait_for_pager();\n+\t\t\t\t\tuse_pager = 0;\n+\t\t\t\t}\n \t\t\t}\n \n \t\t\tstrbuf_reset(&s->buf);\n@@ -1675,8 +1686,9 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\thunk->use = USE_HUNK;\n \t\t\t\tgoto soft_increment;\n \t\t\t}\n-\t\t} else if (s->answer.buf[0] == 'p') {\n+\t\t} else if (ch == 'p') {\n \t\t\trendered_hunk_index = -1;\n+\t\t\tuse_pager = (s->answer.buf[0] == 'P') ? 1 : 0;\n \t\t} else if (s->answer.buf[0] == '?') {\n \t\t\tconst char *p = _(help_patch_remainder), *eol = p;\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 6daf3a6be0..1b8617e0c1 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -591,6 +591,38 @@ test_expect_success 'print again the hunk' '\n \ttest_cmp expect actual.trimmed\n '\n \n+test_expect_success TTY 'print again the hunk (PAGER)' '\n+\ttest_when_finished \"git reset\" &&\n+\tcat >expect <<-EOF &&\n+\t+15\n+\t 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? PAGER @@ -1,2 +1,3 @@\n+\tPAGER 10\n+\tPAGER +15\n+\tPAGER 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? \n+\tEOF\n+\ttest_write_lines s y g 1 P |\n+\t(\n+\t\tGIT_PAGER=\"sed s/^/PAGER\\ /\" &&\n+\t\texport GIT_PAGER &&\n+\t\ttest_terminal git add -p >actual\n+\t) &&\n+\ttail -n 7 actual.trimmed &&\n+\ttest_cmp expect actual.trimmed\n+'\n+\n+test_expect_success TTY 'P handles SIGPIPE when writing to pager' '\n+\ttest_when_finished \"rm -f huge_file; git reset\" &&\n+\tprintf \"\\n%2500000s\" Y >huge_file &&\n+\tgit add -N huge_file &&\n+\ttest_write_lines P q | (\n+\t\tGIT_PAGER=\"head -n 1\" &&\n+\t\texport GIT_PAGER &&\n+\t\ttest_terminal git add -p\n+\t)\n+'\n+\n test_expect_success 'split hunk \"add -p (edit)\"' '\n \t# Split, say Edit and do nothing. Then:\n \t#\n-- \n2.46.0.rc0.4.g6f4990c0d4\n"},{"id":"499460","messageId":"xmqqsevwui31.fsf@gitster.g","threadId":"61825","inReplyTo":"24e83a0f-b0c8-4cd5-b321-1d7702b844ce@gmail.com","subject":"Re: Re* [PATCH v2 0/2] add-p P fixups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-26T18:24:50Z","receivedAt":"2024-07-26T18:24:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> On 7/25/24 5:24 PM, Junio C Hamano wrote:\n>> Phillip Wood writes:\n>> \n>>> ... We\n>>> can add a comment about the dash problem to the commit message when\n>>> this fixup is squashed. Also the problem is now documented in\n>>> Documentation/CodingGuidelines which is more likely to be read by\n>>> other contributors.\n>> \n>> That is a good thing to take into consideration, indeed.\n>\n> Rubén Justo (2):\n> pager: introduce wait_for_pager\n> add-patch: render hunks through the pager\n\nHmph, what happened to the first two out of the four patch series?\nRetracted? Or you just didn't bother sending the whole thing?\n\n\n"},{"id":"499461","messageId":"xmqq8qxouhjv.fsf@gitster.g","threadId":"61825","inReplyTo":"38e190de-cbe4-4f75-acdb-fe566e541179@gmail.com","subject":"Re: [PATCH 2/2] add-patch: render hunks through the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-26T18:36:20Z","receivedAt":"2024-07-26T18:36:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> Make the print command trigger the pager when invoked using a capital\n> 'P', to make it easier for the user to review long hunks.\n>\n> Note that if the PAGER ends unexpectedly before we've been able to send\n> the payload, perhaps because the user is not interested in the whole\n> thing, we might receive a SIGPIPE, which would abruptly and unexpectedly\n> terminate the interactive session for the user.\n>\n> Therefore, we need to ignore a possible SIGPIPE signal. Add a test for\n> this, in addition to the test for normal operation.\n>\n> For the SIGPIPE test, we need to make sure that we completely fill the\n> operating system's buffer, otherwise we might not trigger the SIGPIPE\n> signal. The normal size of this buffer in different OSs varies from a\n> few KBs to 1MB. Use a payload large enough to guarantee that we exceed\n> this limit.\n\nUp to this point, it is fine.\n\nBut ...\n\n> For the tests, avoid the common construct to set and export one-shot\n> variables within the scope of a command:\n>\n> VAR=VAL command args\n>\n> It happens that when \"command\" is a shell function that in turn executes\n> a \"command\", the behavior with \"VAR\" varies depending on the shell:\n>\n> ** Bash 5.2.21 **\n>\n> $ f () { bash -c 'echo A=$A'; }\n> $ A=1 f\n> A=1\n>\n> ** dash 0.5.12-9 **\n>\n> $ f () { bash -c 'echo A=$A'; }\n> $ A=1 f\n> A=1\n>\n> ** dash 0.5.10.2-6 **\n>\n> $ f () { bash -c 'echo A=$A'; }\n> $ A=1 f\n> A=\n>\n> POSIX explicitly says the effect of this construct is unspecified.\n\n... unless the patch is about changing an existing\n\n\tGIT_PAGER=... test_terminal git add -p\n\ninto\n\n\t(\n\t\tGIT_PAGER=...; export GIT_PAGER\n\t\ttest_terminal git add -p\n\t)\n\nthen all of the above is irrelevant noise that says \"as the coding\nguidelines tell us not to use the non-portable 'VAR=VAL shell_func'\nconstruct, we don't.\"\n\nAlways write your proposed log message to those who will read \"git\nlog -p\" 6 months down the road. To them, the trouble we had while\ndiagnosing the true cause of the breakage in the previous iteration\ndo not exist. It is not part of the \"log -p\" output they see.\n\n> One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash\n> 0.5.10.2-6, so avoid using the construct and use a subshell instead.\n\nAnd it does not matter if all CI platforms are updated. As long as\nour coding guidelines say not to use this construct, we don't.\n\nIn any case, that is an appropriate thing to say in a commit that\nfixes use of such a construct, but not a commit that uses the right\nconstuct from the get-go.\n\nI have to say that the [4/4] in the previous round, i.e., fc87b2f7\n(add-patch: render hunks through the pager, 2024-07-25) in my tree,\nis better than this version.\n\nThanks.\n"},{"id":"499464","messageId":"1dc4cb5d-966a-402f-a880-42280750b949@gmail.com","threadId":"61825","inReplyTo":"xmqqsevwui31.fsf@gitster.g","subject":"Re: Re* [PATCH v2 0/2] add-p P fixups","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-26T19:22:15Z","receivedAt":"2024-07-26T19:22:18Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Jul 26, 2024 at 11:24:50AM -0700, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n> > On 7/25/24 5:24 PM, Junio C Hamano wrote:\n> >> Phillip Wood writes:\n> >> \n> >>> ... We\n> >>> can add a comment about the dash problem to the commit message when\n> >>> this fixup is squashed. Also the problem is now documented in\n> >>> Documentation/CodingGuidelines which is more likely to be read by\n> >>> other contributors.\n> >> \n> >> That is a good thing to take into consideration, indeed.\n> >\n> > Rubén Justo (2):\n> > pager: introduce wait_for_pager\n> > add-patch: render hunks through the pager\n> \n> Hmph, what happened to the first two out of the four patch series?\n> Retracted? Or you just didn't bother sending the whole thing?\n> \n\nNo, I just wanted to modify only the two commits in rj/add-p-pager that\nwere affected by the fixups. \n\nI thought it wasn't necessary to modify the first two, which remain\ncorrect, and I didn't want to bring them up again. Additionally,\nkeeping the dates of the first two different from the two modified here\ncould be interesting.\n\nThat's the reason. I didn't want to cause any distraction or add any\ninconvenience, if that has been the case.\n"},{"id":"499468","messageId":"xmqqle1oszn1.fsf@gitster.g","threadId":"61825","inReplyTo":"1dc4cb5d-966a-402f-a880-42280750b949@gmail.com","subject":"Re: Re* [PATCH v2 0/2] add-p P fixups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-26T19:48:34Z","receivedAt":"2024-07-26T19:48:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> I thought it wasn't necessary to modify the first two, which remain\n> correct, and I didn't want to bring them up again. Additionally,\n> keeping the dates of the first two different from the two modified here\n> could be interesting.\n\nThen at least you should have said so. From the receiving end, that\nand retracting the first two would look the same, so there needs to\nbe some clue to let the receiver tell which one is the case.\n\nThanks.\n"},{"id":"499470","messageId":"9f4c596b-cd6c-4f0c-bed4-dd6febb5e697@gmail.com","threadId":"61825","inReplyTo":"xmqqle1oszn1.fsf@gitster.g","subject":"Re: Re* [PATCH v2 0/2] add-p P fixups","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-26T20:16:33Z","receivedAt":"2024-07-26T20:16:36Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Jul 26, 2024 at 12:48:34PM -0700, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n> > I thought it wasn't necessary to modify the first two, which remain\n> > correct, and I didn't want to bring them up again. Additionally,\n> > keeping the dates of the first two different from the two modified here\n> > could be interesting.\n> \n> Then at least you should have said so. From the receiving end, that\n> and retracting the first two would look the same, so there needs to\n> be some clue to let the receiver tell which one is the case.\n\nThat was my intention with:\n\nbase-commit: 506f457e489b2097e2d4fc5ceffd6e242502b2bd\n\nBut you're right, I should have made my intentions clearer.\n"},{"id":"499475","messageId":"xmqq1q3ftxwe.fsf@gitster.g","threadId":"61825","inReplyTo":"xmqq8qxouhjv.fsf@gitster.g","subject":"Re: [PATCH 2/2] add-patch: render hunks through the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-27T01:40:49Z","receivedAt":"2024-07-27T01:40:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> In any case, that is an appropriate thing to say in a commit that\n> fixes use of such a construct, but not a commit that uses the right\n> constuct from the get-go.\n>\n> I have to say that the [4/4] in the previous round, i.e., fc87b2f7\n> (add-patch: render hunks through the pager, 2024-07-25) in my tree,\n> is better than this version.\n\nI do recall that you once had a version where the code violates the\nguidelines (and breaks dash) in one patch, and gets fixed in another\npatch in the same series. The above material would be a perfect fit\nin the proposed log message of the latter step. If we spent so much\neffort and digging to figure out exactly how it breaks with which\nshell, a separate patch to fix, primarily to document the fix, would\nhave made sense.\n\nBut the latest squashes the two and avoids making the mistake in the\nfirst place, eliminating the need for a documented fix. We generally\nprefer to do so to avoid breaking bisection (and the recommendation\nto keep the fix separate so that we can document it better was made\nas an exception), so squashing them into one is fine. But if we\ncommit to that approach to pretend that there was no silly mistake,\nwe should be consistent in pretending that is the case.\n\n"},{"id":"499483","messageId":"82560916-c539-4ae3-a378-391a1da7d80a@gmail.com","threadId":"61825","inReplyTo":"xmqq1q3ftxwe.fsf@gitster.g","subject":"Re: [PATCH 2/2] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-27T14:33:11Z","receivedAt":"2024-07-27T14:33:15Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Jul 26, 2024 at 06:40:49PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > In any case, that is an appropriate thing to say in a commit that\n> > fixes use of such a construct, but not a commit that uses the right\n> > constuct from the get-go.\n> >\n> > I have to say that the [4/4] in the previous round, i.e., fc87b2f7\n> > (add-patch: render hunks through the pager, 2024-07-25) in my tree,\n> > is better than this version.\n> \n> I do recall that you once had a version where the code violates the\n> guidelines (and breaks dash) in one patch, and gets fixed in another\n> patch in the same series. The above material would be a perfect fit\n> in the proposed log message of the latter step. If we spent so much\n> effort and digging to figure out exactly how it breaks with which\n> shell, a separate patch to fix, primarily to document the fix, would\n> have made sense.\n> \n> But the latest squashes the two and avoids making the mistake in the\n> first place, eliminating the need for a documented fix. We generally\n> prefer to do so to avoid breaking bisection (and the recommendation\n> to keep the fix separate so that we can document it better was made\n> as an exception), so squashing them into one is fine. But if we\n> commit to that approach to pretend that there was no silly mistake,\n> we should be consistent in pretending that is the case.\n> \n\nFixing a problematic change with a new commit isn't the best idea if\nwe have the opportunity to prevent the problem in the first place, as\nPhillip pointed out. Since rj/add-p-pager is still open, it's\nworthwhile to amend the problematic commit.\n\nOf course, we've now updated the documentation [*1*] and reinforced\n[*2*] the mechanisms to prevent this from happening again.\n\nHowever, I think adding a comment about the issue to the amended\ncommit, which I think it has been suggested at some point, seems to me\nlike a good addition. I do not believe that a future reading of the\nchange will lead to confusion for this reason. The added comment does\nnot document a fix, I think, but rather it is an explanation of what\nwe're doing in the commit.\n\nFurthermore, we capture in the history, IMHO, notes of how things have\nhappened, which is also why I intend to apply this series on\n506f457e489b2097e2d4fc5ceffd6e242502b2bd, to only amend the last two\ncommits.\n\n 1.- jc/doc-one-shot-export-with-shell-func\n\n 2.- es/shell-check-updates\n"},{"id":"499498","messageId":"9e9bbff4-e2e1-4867-8f17-ebc366c7bec5@gmail.com","threadId":"61825","inReplyTo":"9f4c596b-cd6c-4f0c-bed4-dd6febb5e697@gmail.com","subject":"Re: Re* [PATCH v2 0/2] add-p P fixups","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-28T09:11:27Z","receivedAt":"2024-07-28T09:11:30Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Sat, Jul 27, 2024 at 04:33:11PM +0200, Rubén Justo wrote:\n\n> Fixing a problematic change with a new commit isn't the best idea if\n> we have the opportunity to prevent the problem in the first place, as\n> Phillip pointed out. Since rj/add-p-pager is still open, it's\n> worthwhile to amend the problematic commit.\n> \n> Of course, we've now updated the documentation [*1*] and reinforced\n> [*2*] the mechanisms to prevent this from happening again.\n> \n> However, I think adding a comment about the issue to the amended\n> commit, which I think it has been suggested at some point, seems to me\n> like a good addition. I do not believe that a future reading of the\n> change will lead to confusion for this reason. The added comment does\n> not document a fix, I think, but rather it is an explanation of what\n> we're doing in the commit.\n> \n> Furthermore, we capture in the history, IMHO, notes of how things have\n> happened, which is also why I intend to apply this series on\n> 506f457e489b2097e2d4fc5ceffd6e242502b2bd, to only amend the last two\n> commits.\n>\n> 1.- jc/doc-one-shot-export-with-shell-func\n> \n> 2.- es/shell-check-updates\n\nAfter re-reading the series today, I still believe the change in the\nmessage for [2/2] or rebasing on 506f457e48, add value to the series,\nbut I also see that it's not a significant improvement. Besides that\nminor detail, IMHO, I think we have consensus on the changes.\n\nI'm not going to send a new iteration, not because I'm against\nchanging the message, but because I think we are entering, if we\naren't already, the realm of bikeshedding. \n\nOnce the changes settle, I'll send a new series to address the new\n\"|[cmd]\" command.\n\nFor reference, this was the first message about the 'P' command:\n1d0cb55c-5f32-419a-b593-d5f0969a51fd@gmail.com. After all, I was only\ninterested in reviewing hunks longer that one screen height ;)\n\nThanks, all.\n"},{"id":"499527","messageId":"xmqqed7chw9r.fsf@gitster.g","threadId":"61825","inReplyTo":"9e9bbff4-e2e1-4867-8f17-ebc366c7bec5@gmail.com","subject":"Re: Re* [PATCH v2 0/2] add-p P fixups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-29T18:45:52Z","receivedAt":"2024-07-29T18:45:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> After re-reading the series today, I still believe the change in the\n> message for [2/2] or rebasing on 506f457e48, add value to the series,\n> but I also see that it's not a significant improvement. Besides that\n> minor detail, IMHO, I think we have consensus on the changes.\n\nI think we have established that the \"Don't attempt a one-shot\nexport with shell functions---it would not work\" is not all that\nimportant to stress on *in* *the* *context* *of* *this* *series*.\n\nAfter all, that is why the latest shape of the series is not to do\nthe \"keep the already known to be bad commit, followed by a fix-up\nto illustrate exactly why the first one breaks\" pattern, which is\ndesigned to help future developers when the breakage is rather\nsubtle and we all miss in our initial reviews, but is rather unusual\nfor a topic that hasn't hit 'next' yet. Instead we just correct our\nmistakes and pretend as if we just got straight to the right\nsolution.\n\nSo, let's just use what we already have queued, without details that\nare irrelevant for the final shape of the history that did not have\nsuch a screw-up in the code. The \"Don't attempt a one-shot export\nwith shell functions\" message, as you said, is already captured in a\nmore relevant place to help developers. We could expand that part\nin the documentation patch, or even a new documentation patch, but\nthis topic, and especially its test part, is about ensuring that the\npager support added here will correctly deals with a stuck pipe, and\nno developers who hit this commit either by browsing \"git log\" or by\nfinding it in \"git blame\" would be seeking an advice on \"one-shot\nexport\", as that is not a mistake this series _did_ _not_ commit, at\nleast to them.\n\n"}]}