| Message ID | 38e190de-cbe4-4f75-acdb-fe566e541179@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fc87b2f7c18d0a097c80e4d9ef1f84da687636ab |
| Headers | show |
| Series | [1/2] pager: introduce wait_for_pager | expand |
Rubén Justo <rjusto@gmail.com> writes: > Make the print command trigger the pager when invoked using a capital > 'P', to make it easier for the user to review long hunks. > > Note that if the PAGER ends unexpectedly before we've been able to send > the payload, perhaps because the user is not interested in the whole > thing, we might receive a SIGPIPE, which would abruptly and unexpectedly > terminate the interactive session for the user. > > Therefore, we need to ignore a possible SIGPIPE signal. Add a test for > this, in addition to the test for normal operation. > > For the SIGPIPE test, we need to make sure that we completely fill the > operating system's buffer, otherwise we might not trigger the SIGPIPE > signal. The normal size of this buffer in different OSs varies from a > few KBs to 1MB. Use a payload large enough to guarantee that we exceed > this limit. Up to this point, it is fine. But ... > 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. ... unless the patch is about changing an existing GIT_PAGER=... test_terminal git add -p into ( GIT_PAGER=...; export GIT_PAGER test_terminal git add -p ) then all of the above is irrelevant noise that says "as the coding guidelines tell us not to use the non-portable 'VAR=VAL shell_func' construct, we don't." Always write your proposed log message to those who will read "git log -p" 6 months down the road. To them, the trouble we had while diagnosing the true cause of the breakage in the previous iteration do not exist. It is not part of the "log -p" output they see. > 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. And it does not matter if all CI platforms are updated. As long as our coding guidelines say not to use this construct, we don't. In any case, that is an appropriate thing to say in a commit that fixes use of such a construct, but not a commit that uses the right constuct from the get-go. I have to say that the [4/4] in the previous round, i.e., fc87b2f7 (add-patch: render hunks through the pager, 2024-07-25) in my tree, is better than this version. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > In any case, that is an appropriate thing to say in a commit that > fixes use of such a construct, but not a commit that uses the right > constuct from the get-go. > > I have to say that the [4/4] in the previous round, i.e., fc87b2f7 > (add-patch: render hunks through the pager, 2024-07-25) in my tree, > is better than this version. I do recall that you once had a version where the code violates the guidelines (and breaks dash) in one patch, and gets fixed in another patch in the same series. The above material would be a perfect fit in the proposed log message of the latter step. If we spent so much effort and digging to figure out exactly how it breaks with which shell, a separate patch to fix, primarily to document the fix, would have made sense. But the latest squashes the two and avoids making the mistake in the first place, eliminating the need for a documented fix. We generally prefer to do so to avoid breaking bisection (and the recommendation to keep the fix separate so that we can document it better was made as an exception), so squashing them into one is fine. But if we commit to that approach to pretend that there was no silly mistake, we should be consistent in pretending that is the case.
On Fri, Jul 26, 2024 at 06:40:49PM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > In any case, that is an appropriate thing to say in a commit that > > fixes use of such a construct, but not a commit that uses the right > > constuct from the get-go. > > > > I have to say that the [4/4] in the previous round, i.e., fc87b2f7 > > (add-patch: render hunks through the pager, 2024-07-25) in my tree, > > is better than this version. > > I do recall that you once had a version where the code violates the > guidelines (and breaks dash) in one patch, and gets fixed in another > patch in the same series. The above material would be a perfect fit > in the proposed log message of the latter step. If we spent so much > effort and digging to figure out exactly how it breaks with which > shell, a separate patch to fix, primarily to document the fix, would > have made sense. > > But the latest squashes the two and avoids making the mistake in the > first place, eliminating the need for a documented fix. We generally > prefer to do so to avoid breaking bisection (and the recommendation > to keep the fix separate so that we can document it better was made > as an exception), so squashing them into one is fine. But if we > commit to that approach to pretend that there was no silly mistake, > we should be consistent in pretending that is the case. > Fixing a problematic change with a new commit isn't the best idea if we have the opportunity to prevent the problem in the first place, as Phillip pointed out. Since rj/add-p-pager is still open, it's worthwhile to amend the problematic commit. Of course, we've now updated the documentation [*1*] and reinforced [*2*] the mechanisms to prevent this from happening again. However, I think adding a comment about the issue to the amended commit, which I think it has been suggested at some point, seems to me like a good addition. I do not believe that a future reading of the change will lead to confusion for this reason. The added comment does not document a fix, I think, but rather it is an explanation of what we're doing in the commit. Furthermore, we capture in the history, IMHO, notes of how things have happened, which is also why I intend to apply this series on 506f457e489b2097e2d4fc5ceffd6e242502b2bd, to only amend the last two commits. 1.- jc/doc-one-shot-export-with-shell-func 2.- es/shell-check-updates
diff --git a/add-patch.c b/add-patch.c index 6e176cd21a..f2c76b7d83 100644 --- a/add-patch.c +++ b/add-patch.c @@ -7,9 +7,11 @@ #include "environment.h" #include "gettext.h" #include "object-name.h" +#include "pager.h" #include "read-cache-ll.h" #include "repository.h" #include "strbuf.h" +#include "sigchain.h" #include "run-command.h" #include "strvec.h" #include "pathspec.h" @@ -1391,7 +1393,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n" "/ - search for a hunk matching the given regex\n" "s - split the current hunk into smaller hunks\n" "e - manually edit the current hunk\n" - "p - print the current hunk\n" + "p - print the current hunk, 'P' to use the pager\n" "? - print help\n"); static int patch_update_file(struct add_p_state *s, @@ -1402,7 +1404,7 @@ static int patch_update_file(struct add_p_state *s, struct hunk *hunk; char ch; struct child_process cp = CHILD_PROCESS_INIT; - int colored = !!s->colored.len, quit = 0; + int colored = !!s->colored.len, quit = 0, use_pager = 0; enum prompt_mode_type prompt_mode_type; enum { ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0, @@ -1452,9 +1454,18 @@ static int patch_update_file(struct add_p_state *s, strbuf_reset(&s->buf); if (file_diff->hunk_nr) { if (rendered_hunk_index != hunk_index) { + if (use_pager) { + setup_pager(); + sigchain_push(SIGPIPE, SIG_IGN); + } render_hunk(s, hunk, 0, colored, &s->buf); fputs(s->buf.buf, stdout); rendered_hunk_index = hunk_index; + if (use_pager) { + sigchain_pop(SIGPIPE); + wait_for_pager(); + use_pager = 0; + } } strbuf_reset(&s->buf); @@ -1675,8 +1686,9 @@ static int patch_update_file(struct add_p_state *s, hunk->use = USE_HUNK; goto soft_increment; } - } else if (s->answer.buf[0] == 'p') { + } else if (ch == 'p') { rendered_hunk_index = -1; + use_pager = (s->answer.buf[0] == 'P') ? 1 : 0; } else if (s->answer.buf[0] == '?') { const char *p = _(help_patch_remainder), *eol = p; diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 6daf3a6be0..1b8617e0c1 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -591,6 +591,38 @@ test_expect_success 'print again the hunk' ' test_cmp expect actual.trimmed ' +test_expect_success TTY 'print again the hunk (PAGER)' ' + test_when_finished "git reset" && + cat >expect <<-EOF && + <GREEN>+<RESET><GREEN>15<RESET> + 20<RESET> + <BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET> + PAGER 10<RESET> + PAGER <GREEN>+<RESET><GREEN>15<RESET> + PAGER 20<RESET> + <BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET> + EOF + test_write_lines s y g 1 P | + ( + GIT_PAGER="sed s/^/PAGER\ /" && + export GIT_PAGER && + test_terminal git add -p >actual + ) && + tail -n 7 <actual | test_decode_color >actual.trimmed && + test_cmp expect actual.trimmed +' + +test_expect_success TTY 'P handles SIGPIPE when writing to pager' ' + 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" && + export GIT_PAGER && + test_terminal git add -p + ) +' + test_expect_success 'split hunk "add -p (edit)"' ' # Split, say Edit and do nothing. Then: #
Make the print command trigger the pager when invoked using a capital 'P', to make it easier for the user to review long hunks. Note that if the PAGER ends unexpectedly before we've been able to send the payload, perhaps because the user is not interested in the whole thing, we might receive a SIGPIPE, which would abruptly and unexpectedly terminate the interactive session for the user. Therefore, we need to ignore a possible SIGPIPE signal. Add a test for this, in addition to the test for normal operation. For the SIGPIPE test, we need to make sure that we completely fill the operating system's buffer, otherwise we might not trigger the SIGPIPE signal. The normal size of this buffer in different OSs varies from a 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> --- add-patch.c | 18 +++++++++++++++--- t/t3701-add-interactive.sh | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-)