| Message ID | b7e24b08-40a1-4b18-89f6-e25ab96facaf@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | use the pager in 'add -p' | expand |
Rubén Justo <rjusto@gmail.com> writes: > Hopefully, we'll find a way to avoid sending ANSI codes, on demand, > without disabling it entirely with color.ui=never or any other global > option. To make this usable: > > (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | vim - > > However, the current functionality meets my current needs, so I'm happy > with it. Yup, if it really is needed we could do || or anything "unusual" to signal the unusual nature of the command. Or ">" command can send the output to specified file without coloring, and the user can do whatever they want to it. In any case, unlike "Let's not just do pager, but have a facility to pipe to anything and make the pager a default destination" that was a natural match to the originally proposed behaviour, these two are quite different and can be left totally outside the scope of the topic. > One final note; I preferred to model the help text this way: > > y - stage this hunk > n - do not stage this hunk > ... > g - select a hunk to go to > / - search for a hunk matching the given regex > ... > | - pipe the current hunk to the pager, or |<program> to use a program' > ? - print help That's fine. The 'g' and '/' commands take _mandatory_ arguments, but we do not even mention it in the help text. But we need to say something for this new thing, because it is _optional_ and if you do not give a program, it does not ask. A possibility is to phrase it like so: | - pipe the current hunk to the program (or "%s" by default) and fill %s with the program you'd use if not given, i.e. initially the value of the GIT_PAGER but updated to the last used program after the user uses "|<program>" form to specify one.
Junio C Hamano <gitster@pobox.com> writes: > The 'g' and '/' commands take _mandatory_ arguments, but we do not > even mention it in the help text. But we need to say something for > this new thing, because it is _optional_ and if you do not give a > program, it does not ask. By the way, although I personally do not have much sympathy to those who set it, in the presence of interactive.singleKey configuration variable, a command that takes optional argument may turn out to be a mistake, as the user cannot give the argument even if they wanted to, when the configuration variable is set to true. To help them, we'd probably need something like the following to allow them to optionally set their own program, like the following: 1. read the command, notice that it begins with '|'. 2. if it has <program> after it, call it <program> and jump to 5. 3. if it does not have <program> after it, but if single key operation is in effect, give the user a chance to give <program> by prompting. Call the answer to the prompt <program>. If it is not an empty string, jump to 5. 4. at this point, we have <program> that is empty as given by the user. Replace the <program> with the value we remembered from step 6. during the last use of the '|' command. 5. if <program> is all whitespace, replace it with an empty string. 6. remember the value of <program> (so that you can reuse it next time the user says '|' and without <program>), and call the "set-custom-pager" thing with <program> (this design assumes that "set-custom-pager" thing uses the GIT_PAGER when fed an empty string). 7. spawn the program set by "set-custom-pager", and feed our output to it. So the end-user observable behaviour would become * There is the "default" program, initially their pager, but after they use the '|' command, we remember the last program they used during the session and reuse it when they tell us to do so. * For singlekey folks, typing '|' will give them a prompt. They can give an empty string and spawn the "default" thing. They can give " " plus <RET> to reset the "default" to GIT_PAGER and use it. Or they can give <program> plus <RET> to use it and update the "default". * For the rest of us, typing "|" plus <RET> will spawn the "default" thing. Typing "|<program>" plus <RET> will use the <program> and update the "default". Typing "| " plus <RET> will reset the "default" to GIT_PAGER and use it. which is quite straight-forward and consistent between the two camps.
On Sun, Jun 02, 2024 at 09:36:35AM -0700, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > > Hopefully, we'll find a way to avoid sending ANSI codes, on demand, > > without disabling it entirely with color.ui=never or any other global > > option. To make this usable: > > > > (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | vim - > > > > However, the current functionality meets my current needs, so I'm happy > > with it. > > Yup, if it really is needed we could do || or anything "unusual" to > signal the unusual nature of the command. > > Or ">" command can send the output to specified file without > coloring, and the user can do whatever they want to it. Interesting; perhaps intuitive and in line with other 'isatty()' conditions we have. > > In any case, unlike "Let's not just do pager, but have a facility to > pipe to anything and make the pager a default destination" that was > a natural match to the originally proposed behaviour, these two are > quite different and can be left totally outside the scope of the > topic. > > > One final note; I preferred to model the help text this way: > > > > y - stage this hunk > > n - do not stage this hunk > > ... > > g - select a hunk to go to > > / - search for a hunk matching the given regex > > ... > > | - pipe the current hunk to the pager, or |<program> to use a program' > > ? - print help > > That's fine. > > The 'g' and '/' commands take _mandatory_ arguments, but we do not > even mention it in the help text. But we need to say something for > this new thing, because it is _optional_ and if you do not give a > program, it does not ask. > > A possibility is to phrase it like so: > > | - pipe the current hunk to the program (or "%s" by default) > > and fill %s with the program you'd use if not given, i.e. initially > the value of the GIT_PAGER but updated to the last used program > after the user uses "|<program>" form to specify one. And this one too; a nice way to allow reusing the previous value. Perhaps we'd better find a way to introduce some form of CTRL+P, or arrow-up... I dunno. Interesting ideas. But my preference is to queue this series as it is, if no major issue is pointed out. And leave for future series this topics or the others mentioned in the thread: the 'interactive.' setting suggested by Peff, or the '-P' switch, by Dragan. Let's see what others say. Thanks.
On Sun, Jun 02, 2024 at 10:11:49AM -0700, Junio C Hamano wrote: > By the way, although I personally do not have much sympathy to those > who set it, in the presence of interactive.singleKey configuration > variable, a command that takes optional argument may turn out to be > a mistake, as the user cannot give the argument even if they wanted > to, when the configuration variable is set to true. Well spotted. Perhaps we can do something like: diff --git a/add-patch.c b/add-patch.c index 5a586d1b9b..01525214f9 100644 --- a/add-patch.c +++ b/add-patch.c @@ -1685,8 +1685,17 @@ static int patch_update_file(struct add_p_state *s, } else if (s->answer.buf[0] == 'p') { rendered_hunk_index = -1; } else if (ch == '|') { + strbuf_remove(&s->answer, 0, 1); + if (s->s.use_single_key && !s->answer.len) { + printf("%s", _("program? ")); + fflush(stdout); + strbuf_getline(&s->answer, stdin); + strbuf_trim_trailing_newline(&s->answer); + } rendered_hunk_index = -1; - pager = s->answer.buf + 1; + pager = s->answer.buf; } else if (s->answer.buf[0] == '?') { const char *p = _(help_patch_remainder), *eol = p; I agree this needs to be addressed in this series. I'd prefer to leave the feature of reusing the command, to another series.
Hello Ruben, On 2024-06-02 17:38, Rubén Justo wrote: > This iteration, v3, introduces a new command: '|', suggested by Junio, > instead of the 'P' command proposed in the previous iteration. > > This allows us to use the pager: > > (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | > > But also to use other programs, like: > > (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | head > > Or: > > (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | grep term > > Hopefully, we'll find a way to avoid sending ANSI codes, on demand, > without disabling it entirely with color.ui=never or any other global > option. To make this usable: > > (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | vim - > > However, the current functionality meets my current needs, so I'm happy > with it. The way I see it, using "| <program>" should follow the de facto rules already established by the "--color=auto" command-line option in multiple utilities. Thus, when piping to a custom program, the escape codes that perform the coloring should be stripped. > This, a new 'interactive.pipeCommand' setting, or a new switch: 'add > -P', > are left for discussing in, hopefully, a future series. > > One final note; I preferred to model the help text this way: > > y - stage this hunk > n - do not stage this hunk > q - quit; do not stage this hunk or any of the remaining ones > a - stage this hunk and all later hunks in the file > d - do not stage this hunk or any of the later hunks in the file > j - leave this hunk undecided, see next undecided hunk > J - leave this hunk undecided, see next hunk > g - select a hunk to go to > / - search for a hunk matching the given regex > s - split the current hunk into smaller hunks > e - manually edit the current hunk > p - print the current hunk > | - pipe the current hunk to the pager, or |<program> to use a > program' > ? - print help I also like this form better, but I think wording could be improved. I'll think a bit more about it, maybe something like this: | - use pager to show the current hunk, or use |<program> to customize Also, what's the single quote doing after "use a program"? > Instead of: > > y - stage this hunk > n - do not stage this hunk > q - quit; do not stage this hunk or any of the remaining ones > a - stage this hunk and all later hunks in the file > d - do not stage this hunk or any of the later hunks in the file > j - leave this hunk undecided, see next undecided hunk > J - leave this hunk undecided, see next hunk > g - select a hunk to go to > / - search for a hunk matching the given regex > s - split the current hunk into smaller hunks > e - manually edit the current hunk > p - print the current hunk > |[program] - pipe the current hunk to a program, the pager if > none... > ? - print help > > Because I believe it reads better by maintaining a single character > before the dash. But I am not opposed to the latter.
Hello Junio, On 2024-06-02 18:36, Junio C Hamano wrote: > A possibility is to phrase it like so: > > | - pipe the current hunk to the program (or "%s" by default) > > and fill %s with the program you'd use if not given, i.e. initially > the value of the GIT_PAGER but updated to the last used program > after the user uses "|<program>" form to specify one. The value of GIT_PAGER (or the <program>) can be a rather long string, so it would have to be stripped down to the base command, but it would be rather error-prone and the printed information would become much less informative that way.
Dragan Simic <dsimic@manjaro.org> writes: > Hello Junio, > > On 2024-06-02 18:36, Junio C Hamano wrote: >> A possibility is to phrase it like so: >> | - pipe the current hunk to the program (or "%s" by default) >> and fill %s with the program you'd use if not given, i.e. initially >> the value of the GIT_PAGER but updated to the last used program >> after the user uses "|<program>" form to specify one. > > The value of GIT_PAGER (or the <program>) can be a rather long string, > so it would have to be stripped down to the base command, but it would > be rather error-prone and the printed information would become much less > informative that way. I'd probably just say $GIT_PAGER instead of its expansion if we were go that route.
On 2024-06-03 11:03, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: >> On 2024-06-02 18:36, Junio C Hamano wrote: >>> A possibility is to phrase it like so: >>> | - pipe the current hunk to the program (or "%s" by default) >>> and fill %s with the program you'd use if not given, i.e. initially >>> the value of the GIT_PAGER but updated to the last used program >>> after the user uses "|<program>" form to specify one. >> >> The value of GIT_PAGER (or the <program>) can be a rather long string, >> so it would have to be stripped down to the base command, but it would >> be rather error-prone and the printed information would become much >> less >> informative that way. > > I'd probably just say $GIT_PAGER instead of its expansion if we were > go that route. Makes sense to me. More precisely, the environment should be checked to see is it "$GIT_PAGER" or "$PAGER" that needs to be printed literally as part of the help message.
Dragan Simic <dsimic@manjaro.org> writes: >> I'd probably just say $GIT_PAGER instead of its expansion if we were >> go that route. > > Makes sense to me. More precisely, the environment should be checked > to see is it "$GIT_PAGER" or "$PAGER" that needs to be printed literally > as part of the help message. I was sure somebody will split a hair like that. At that point we are better off mentioning 'git var' X-<. Or just 'Your Pager'.
Dragan Simic <dsimic@manjaro.org> writes: > Thus, when piping to a custom program, the escape codes that > perform the coloring should be stripped. I tend to agree that if we do not give a way to toggle between "with" and "without" color when piping to a program, it is safer to make the default uncolored. The user's configured pager is expected to deal with colors just fine (or the user has globally configured colors to be off). As we are capable of telling if the user is asking to spawn the default pager (by not giving a custom command or by clearing the previous custom command given in the same session) or a custom one, it should be easily doable to give colored version to the configured pager and uncolored version to a custom/one-shot command. Unlike the existing support for (e)dit command, we do not read back from what the command does using the hunk and present it again to the user, it should be a relatively easy and safe thing to do.
On Sun, Jun 02, 2024 at 07:36:37PM +0200, Dragan Simic wrote: > The way I see it, using "| <program>" should follow the de facto rules > already established by the "--color=auto" command-line option in multiple > utilities. Thus, when piping to a custom program, the escape codes that > perform the coloring should be stripped. Interesting. However, I'd like to find a way to keep the escape codes when using programs like: '|head'; perhaps with the '>' command, suggested by Junio. At any rate, I feel we can leave that, perhaps corner-case scenario, for a future series. As this series is mainly about the 'pager' machinery. > > > This, a new 'interactive.pipeCommand' setting, or a new switch: 'add > > -P', > > are left for discussing in, hopefully, a future series. > > > > One final note; I preferred to model the help text this way: > > > > y - stage this hunk > > n - do not stage this hunk > > q - quit; do not stage this hunk or any of the remaining ones > > a - stage this hunk and all later hunks in the file > > d - do not stage this hunk or any of the later hunks in the file > > j - leave this hunk undecided, see next undecided hunk > > J - leave this hunk undecided, see next hunk > > g - select a hunk to go to > > / - search for a hunk matching the given regex > > s - split the current hunk into smaller hunks > > e - manually edit the current hunk > > p - print the current hunk > > | - pipe the current hunk to the pager, or |<program> to use a > > program' > > ? - print help > > I also like this form better, but I think wording could be improved. > I'll think a bit more about it, maybe something like this: > > | - use pager to show the current hunk, or use |<program> to customize Certainly! It is indeed a sensible idea to improve the wording, avoiding the word "pipe" :-). Thank you. > > Also, what's the single quote doing after "use a program"? Just a typo. Sorry. > > > Instead of: > > > > y - stage this hunk > > n - do not stage this hunk > > q - quit; do not stage this hunk or any of the remaining ones > > a - stage this hunk and all later hunks in the file > > d - do not stage this hunk or any of the later hunks in the file > > j - leave this hunk undecided, see next undecided hunk > > J - leave this hunk undecided, see next hunk > > g - select a hunk to go to > > / - search for a hunk matching the given regex > > s - split the current hunk into smaller hunks > > e - manually edit the current hunk > > p - print the current hunk > > |[program] - pipe the current hunk to a program, the pager if > > none... > > ? - print help > > > > Because I believe it reads better by maintaining a single character > > before the dash. But I am not opposed to the latter.
On Sun, Jun 02, 2024 at 05:38:42PM +0200, Rubén Justo wrote: > However, the current functionality meets my current needs, so I'm happy > with it. > > This, a new 'interactive.pipeCommand' setting, or a new switch: 'add -P', > are left for discussing in, hopefully, a future series. Earlier I suggested that I'd set such a config variable to something like: diff-highlight | less -FX But after playing with your patch, I realized that: - there's no need to pipe through diff-highlight; it already happened as part of interactive.diffFilter! - since it's triggered manually now, there's no need to add in -FX So I am perfectly happy for you to stop where you did. Possibly interactive.pipeCommand could be useful in a more general sense, but we can wait until somebody encounters that itch. -Peff
Jeff King <peff@peff.net> writes: > diff-highlight | less -FX > > But after playing with your patch, I realized that: > > - there's no need to pipe through diff-highlight; it already happened > as part of interactive.diffFilter! ;-) > - since it's triggered manually now, there's no need to add in -FX I do not know about -X, but yeah, -F is of dubious value in this particular context. You explicitly told us that you want to page, somehow knowing that the hunk needs paging. > So I am perfectly happy for you to stop where you did. Possibly > interactive.pipeCommand could be useful in a more general sense, but we > can wait until somebody encounters that itch. It makes it sound like if somebody has a use case already we shouldn't stop here ;-) The default that colors the output is something we might later regret. Those who want colored output can always use the interactive.diffFilter configuration, but I am not sure if going the other direction to strip coloring is just as easy. But other than that, I think we are at an OK place to stop. Thanks.
On 2024-06-03 17:28, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: > >>> I'd probably just say $GIT_PAGER instead of its expansion if we were >>> go that route. >> >> Makes sense to me. More precisely, the environment should be checked >> to see is it "$GIT_PAGER" or "$PAGER" that needs to be printed >> literally >> as part of the help message. > > I was sure somebody will split a hair like that. At that point we > are better off mentioning 'git var' X-<. Or just 'Your Pager'. I agree that it can be seen as splitting hairs, but the general approach of Git is accuracy, so using "$GIT_PAGER" regardless of it being used or not would be simply against the general approach. Though, I agree that using "pager" or maybe even "your pager" would be simpler, yet a very good option.
On 2024-06-03 18:01, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: > >> Thus, when piping to a custom program, the escape codes that >> perform the coloring should be stripped. > > I tend to agree that if we do not give a way to toggle between > "with" and "without" color when piping to a program, it is safer to > make the default uncolored. Good point. The default behavior or "|xyz" should be to strip the coloring escape sequences, because we don't know is "xyz" capable of handling those escape sequences properly, and to keep the coloring with "||xyz" or whatever we come up with for our equivalent "--color=always", so to speak. > The user's configured pager is expected to deal with colors just > fine (or the user has globally configured colors to be off). As we > are capable of telling if the user is asking to spawn the default > pager (by not giving a custom command or by clearing the previous > custom command given in the same session) or a custom one, it should > be easily doable to give colored version to the configured pager and > uncolored version to a custom/one-shot command. Unlike the existing > support for (e)dit command, we do not read back from what the > command does using the hunk and present it again to the user, it > should be a relatively easy and safe thing to do. Exactly.
On 2024-06-04 19:41, Dragan Simic wrote: > On 2024-06-03 18:01, Junio C Hamano wrote: >> Dragan Simic <dsimic@manjaro.org> writes: >> >>> Thus, when piping to a custom program, the escape codes that >>> perform the coloring should be stripped. >> >> I tend to agree that if we do not give a way to toggle between >> "with" and "without" color when piping to a program, it is safer to >> make the default uncolored. > > Good point. The default behavior or "|xyz" should be to strip > the coloring escape sequences, because we don't know is "xyz" > capable of handling those escape sequences properly, and to keep > the coloring with "||xyz" or whatever we come up with for our > equivalent "--color=always", so to speak. Oops, sorry... s/equivalent/equivalent of/ >> The user's configured pager is expected to deal with colors just >> fine (or the user has globally configured colors to be off). As we >> are capable of telling if the user is asking to spawn the default >> pager (by not giving a custom command or by clearing the previous >> custom command given in the same session) or a custom one, it should >> be easily doable to give colored version to the configured pager and >> uncolored version to a custom/one-shot command. Unlike the existing >> support for (e)dit command, we do not read back from what the >> command does using the hunk and present it again to the user, it >> should be a relatively easy and safe thing to do. > > Exactly.
On 2024-06-03 22:19, Rubén Justo wrote: > On Sun, Jun 02, 2024 at 07:36:37PM +0200, Dragan Simic wrote: > >> The way I see it, using "| <program>" should follow the de facto rules >> already established by the "--color=auto" command-line option in >> multiple >> utilities. Thus, when piping to a custom program, the escape codes >> that >> perform the coloring should be stripped. > > Interesting. However, I'd like to find a way to keep the escape codes > when using programs like: '|head'; perhaps with the '>' command, > suggested by Junio. > > At any rate, I feel we can leave that, perhaps corner-case scenario, > for > a future series. As this series is mainly about the 'pager' machinery. I'd suggest that "| <program>" is made to work as "--color=auto" in the current series, i.e. with the coloring escape sequences stripped, which is the safe approach because we don't know is "<program>" capable of handling those escape sequences, while implementing support for "--color=always" would be left for a follow-up series. Using ">" for that future command makes sense to me, because it suggests an output redirection in raw form. Of course, the plain "|" should still leave the coloring escape sequences intact, because the configured pager (or the default pager, configured by Git internally) is expected to handle them properly. >> > This, a new 'interactive.pipeCommand' setting, or a new switch: 'add >> > -P', >> > are left for discussing in, hopefully, a future series. >> > >> > One final note; I preferred to model the help text this way: >> > >> > y - stage this hunk >> > n - do not stage this hunk >> > q - quit; do not stage this hunk or any of the remaining ones >> > a - stage this hunk and all later hunks in the file >> > d - do not stage this hunk or any of the later hunks in the file >> > j - leave this hunk undecided, see next undecided hunk >> > J - leave this hunk undecided, see next hunk >> > g - select a hunk to go to >> > / - search for a hunk matching the given regex >> > s - split the current hunk into smaller hunks >> > e - manually edit the current hunk >> > p - print the current hunk >> > | - pipe the current hunk to the pager, or |<program> to use a >> > program' >> > ? - print help >> >> I also like this form better, but I think wording could be improved. >> I'll think a bit more about it, maybe something like this: >> >> | - use pager to show the current hunk, or use |<program> to >> customize > > Certainly! It is indeed a sensible idea to improve the wording, > avoiding > the word "pipe" :-). Thank you. I'm glad that you like it. :) >> Also, what's the single quote doing after "use a program"? > > Just a typo. Sorry. Ah, I see. It looked a bit strange. :) >> > Instead of: >> > >> > y - stage this hunk >> > n - do not stage this hunk >> > q - quit; do not stage this hunk or any of the remaining ones >> > a - stage this hunk and all later hunks in the file >> > d - do not stage this hunk or any of the later hunks in the file >> > j - leave this hunk undecided, see next undecided hunk >> > J - leave this hunk undecided, see next hunk >> > g - select a hunk to go to >> > / - search for a hunk matching the given regex >> > s - split the current hunk into smaller hunks >> > e - manually edit the current hunk >> > p - print the current hunk >> > |[program] - pipe the current hunk to a program, the pager if >> > none... >> > ? - print help >> > >> > Because I believe it reads better by maintaining a single character >> > before the dash. But I am not opposed to the latter.
On Tue, Jun 04, 2024 at 08:32:04AM -0700, Junio C Hamano wrote: > The default that colors the output is something we might later > regret. Those who want colored output can always use the > interactive.diffFilter configuration, but I am not sure if going > the other direction to strip coloring is just as easy. But other > than that, I think we are at an OK place to stop. I don't think diffFilter is a great solution there. In my experience, that is augmenting the existing coloring done by "diff --color" itself (and of course many people do not use a separate filter in the first place, and just see the normal color output). So there's not an easy way to add the color back to a stripped version. The interactive-patch code is literally holding the two color/non-color variants in memory, splitting the hunks based on line counts, and then showing you one and applying the other. Even if you wanted to run "git diff --color" yourself in the "|" command, it would be a lot of work to pick out the hunk of interest. Given that the main use case for "|" is for human viewing through a pager, I think the colorful, filtered version meant for users is the best default. And then the "bare" version can come from an alternate command or a knob. Just to note some prior art, mutt's "<pipe-message>" faces a similar problem. You might want the raw message (if you're going to poke at headers, MIME parts, etc yourself) or you may want a decoded one (if you just care about body text and don't want to deal with base64, qp, etc, yourself). They provide a stateful config knob, but then you end up with horrible macros that toggle the knob, like: :set pipe_decode=yes<enter><pipe-message>my-script<enter>:set pipe_decode=no<enter> I think having two separate commands for the two modes would be less confusing. -Peff
On 05/06/2024 10:09, Jeff King wrote: > On Tue, Jun 04, 2024 at 08:32:04AM -0700, Junio C Hamano wrote: > > Given that the main use case for "|" is for human viewing through a > pager, I think the colorful, filtered version meant for users is the > best default. And then the "bare" version can come from an alternate > command or a knob. I think that's a very good point. It is hard to see what "|" can be used for other than viewing the hunk as (a) git does not read the output so it cannot be used to filter or edit the hunk that is applied and (b) we pass an isolated hunk so the post-image offset in the hunk header is likely to be wrong and there is no indication as to which file it comes from so the program being run cannot apply the hunk itself. Having the escape codes does make it harder to filter the hunk. For example to just look at the post-image as one needs to do something like grep '^[^-+ @]*[+ @]' instead of just using '^[+ @]' as the pattern but the bonus is that the output is colored. > Just to note some prior art, mutt's "<pipe-message>" faces a similar > problem. You might want the raw message (if you're going to poke at > headers, MIME parts, etc yourself) or you may want a decoded one (if you > just care about body text and don't want to deal with base64, qp, etc, > yourself). They provide a stateful config knob, but then you end up with > horrible macros that toggle the knob, like: > > :set pipe_decode=yes<enter><pipe-message>my-script<enter>:set pipe_decode=no<enter> > > I think having two separate commands for the two modes would be less > confusing. That does sound simpler Best Wishes Phillip
Jeff King <peff@peff.net> writes: > Just to note some prior art, mutt's "<pipe-message>" faces a similar > problem. You might want the raw message (if you're going to poke at > headers, MIME parts, etc yourself) or you may want a decoded one (if you > just care about body text and don't want to deal with base64, qp, etc, > yourself). They provide a stateful config knob, but then you end up with > horrible macros that toggle the knob, like: > > :set pipe_decode=yes<enter><pipe-message>my-script<enter>:set pipe_decode=no<enter> > > I think having two separate commands for the two modes would be less > confusing. Yup, I have been suffering from its equivalent in Gnus/Emacs every day, where decoded version of multipart/signed messages look different enough from valid RFC2822 messages and break application with "git am", and there of course is "feed the undecoded original to pipe" option. But the need for both is pretty much known, and the real issue is that what the default should be. The default destination of the pipe being a pager, making the colored output the default is consistent with that, I would think. Thanks.
On 2024-06-05 15:21, Phillip Wood wrote: > On 05/06/2024 10:09, Jeff King wrote: >> On Tue, Jun 04, 2024 at 08:32:04AM -0700, Junio C Hamano wrote: >> >> Given that the main use case for "|" is for human viewing through a >> pager, I think the colorful, filtered version meant for users is the >> best default. And then the "bare" version can come from an alternate >> command or a knob. > > I think that's a very good point. It is hard to see what "|" can be > used for other than viewing the hunk as (a) git does not read the > output so it cannot be used to filter or edit the hunk that is applied > and (b) we pass an isolated hunk so the post-image offset in the hunk > header is likely to be wrong and there is no indication as to which > file it comes from so the program being run cannot apply the hunk > itself. Having the escape codes does make it harder to filter the > hunk. For example to just look at the post-image as one needs to do > something like > > grep '^[^-+ @]*[+ @]' > > instead of just using '^[+ @]' as the pattern but the bonus is that > the output is colored. Agreed, but as I already explained, [1] only when using the bare "|" command. When "|xyz" is used instead, the version of the hunk with no coloring escape sequences should be piped to xyz. [1] https://lore.kernel.org/git/844704794168f9fcb85c75014c84cde0@manjaro.org/
On Sat, Jun 08, 2024 at 07:54:34AM +0200, Dragan Simic wrote: > When "|xyz" is used instead, the version of the hunk with no coloring > escape sequences should be piped to xyz. That is a sane and conservative approach, and I'm not opposed. However, giving the colorful version though a custom pager is a good thing to have, I think, i.e: allowing a simple "head" without losing the coloring. Let's recap a bit. Initially, this series aimed to enable sending chunks to the pager during "add -p" sessions. To reduce the blast radius of spawning a pager for each chunk, we introduced a new command "P". Junio suggested opening up the command to allow specifying a custom pager, in the form of "P<program>". The "P" command started to resemble a lot to the common pipe operator. Thus, we shifted to "|<program>". Some concerns were raised about controlling when to send coloring escape sequences. Several ideas were discussed to address this, including introducing a new command ">", a modifier for "|": "||", and others. Alternatively, we could leave it up to the user to filter as needed. Or, simply, do not send escape codes at all. So, looking back at the ideas discussed in the thread, perhaps a reasonable next step might be to reintroduce the 'P<program>' command and let '|<program>' be the way to send raw, uncolored, chunks. This approach makes sense to me. I'll wait a bit before sending a reroll to gather feedback, though. Thanks.
Hello Ruben, On 2024-06-09 09:44, Rubén Justo wrote: > Some concerns were raised about controlling when to send coloring > escape > sequences. Several ideas were discussed to address this, including > introducing a new command ">", a modifier for "|": "||", and others. > Alternatively, we could leave it up to the user to filter as needed. > Or, simply, do not send escape codes at all. > > So, looking back at the ideas discussed in the thread, perhaps a > reasonable next step might be to reintroduce the 'P<program>' command > and let '|<program>' be the way to send raw, uncolored, chunks. Actually, it would be better to re-introduce the "P" option, without any parameters, which would display the current hunk through the already configured pager, and let "|<program>" be the new option that pipes hunks _without_ coloring escape sequences to "<program>". As a follow-up, after the dust settles, we could add "><program>" as another new option that pipes hunks _with_ the coloring escape sequences to "<program>". That would be a rather clean approach, which would also reserve the "weird-looking" commands with additional parameters (i.e. "|<program>" and "><program>") for the more advanced operations, while the "normal-looking" command (i.e. "P") would be assigned to the, presumably, most commonly used new hunk operation.
Hi Dragan On 08/06/2024 06:54, Dragan Simic wrote: > On 2024-06-05 15:21, Phillip Wood wrote: >> On 05/06/2024 10:09, Jeff King wrote: >>> On Tue, Jun 04, 2024 at 08:32:04AM -0700, Junio C Hamano wrote: >>> >>> Given that the main use case for "|" is for human viewing through a >>> pager, I think the colorful, filtered version meant for users is the >>> best default. And then the "bare" version can come from an alternate >>> command or a knob. >> >> I think that's a very good point. It is hard to see what "|" can be >> used for other than viewing the hunk as (a) git does not read the >> output so it cannot be used to filter or edit the hunk that is applied >> and (b) we pass an isolated hunk so the post-image offset in the hunk >> header is likely to be wrong and there is no indication as to which >> file it comes from so the program being run cannot apply the hunk >> itself. Having the escape codes does make it harder to filter the >> hunk. For example to just look at the post-image as one needs to do >> something like >> >> grep '^[^-+ @]*[+ @]' >> >> instead of just using '^[+ @]' as the pattern but the bonus is that >> the output is colored. > > Agreed, but as I already explained, [1] only when using the bare "|" > command. When "|xyz" is used instead, the version of the hunk with > no coloring escape sequences should be piped to xyz. Having read the message you referenced I'm struggling to understand the use-case for stripping escape codes - what do you want to do with the hunk that means you want to remove the color? Best Wishes Phillip > > [1] > https://lore.kernel.org/git/844704794168f9fcb85c75014c84cde0@manjaro.org/
Hello Phillip, On 2024-06-09 16:29, phillip.wood123@gmail.com wrote: > On 08/06/2024 06:54, Dragan Simic wrote: >> On 2024-06-05 15:21, Phillip Wood wrote: >>> On 05/06/2024 10:09, Jeff King wrote: >>>> On Tue, Jun 04, 2024 at 08:32:04AM -0700, Junio C Hamano wrote: >>>> >>>> Given that the main use case for "|" is for human viewing through a >>>> pager, I think the colorful, filtered version meant for users is the >>>> best default. And then the "bare" version can come from an alternate >>>> command or a knob. >>> >>> I think that's a very good point. It is hard to see what "|" can be >>> used for other than viewing the hunk as (a) git does not read the >>> output so it cannot be used to filter or edit the hunk that is >>> applied >>> and (b) we pass an isolated hunk so the post-image offset in the hunk >>> header is likely to be wrong and there is no indication as to which >>> file it comes from so the program being run cannot apply the hunk >>> itself. Having the escape codes does make it harder to filter the >>> hunk. For example to just look at the post-image as one needs to do >>> something like >>> >>> grep '^[^-+ @]*[+ @]' >>> >>> instead of just using '^[+ @]' as the pattern but the bonus is that >>> the output is colored. >> >> Agreed, but as I already explained, [1] only when using the bare "|" >> command. When "|xyz" is used instead, the version of the hunk with >> no coloring escape sequences should be piped to xyz. > > Having read the message you referenced I'm struggling to understand > the use-case for stripping escape codes - what do you want to do with > the hunk that means you want to remove the color? Let me recap, please. Basically, when an output of some command is piped into another command, e.g. by running "grep -r abc . | grep def", the command that produces the piped output doesn't put the coloring escape codes into the produced output, because it's unknown can the command that receives it handle those escape codes properly. That's become some kind of de facto standard embodied into the "--color=auto" command-line option for various utilities. In the example above, one can have "grep -n --color=auto" defined as their alias for "grep", which is what I use, and the "grep -r abc" produces the output with no coloring escape sequences, which gets piped into "grep def" that does produce coloring escape codes, because its output goes to the terminal emulator, which is expected to handle those escape codes properly. In our use case, Git becomes what produces a hunk as the output that gets piped into some program "xyz" by receiving "|xyz" as the command while "git add -p" is executed, but it isn't known can "xyz" handle the coloring escape sequences properly, so they should not be included in the piped output, i.e. in the produced hunk. As discussed later, [2] we could introduce ">xyz" as another command for "git add -p", so receiving ">xyz" from the user would pipe the hunk to "xyz" with the coloring escape sequences included, because the user already knows that "xyz" can handle them. Separating this into two commands is a safe approach. I hope this makes the whole thing more clear. [1] https://lore.kernel.org/git/844704794168f9fcb85c75014c84cde0@manjaro.org/ [2] https://lore.kernel.org/git/7937845d7cb7ae0179c4922ed154c5c7@manjaro.org/
Hi Dragan On 09/06/2024 18:20, Dragan Simic wrote: > On 2024-06-09 16:29, phillip.wood123@gmail.com wrote: > >> Having read the message you referenced I'm struggling to understand >> the use-case for stripping escape codes - what do you want to do with >> the hunk that means you want to remove the color? > > Let me recap, please. > [...] > I hope this makes the whole thing more clear. It is very clear _how_ you think it should work and I agree that makes sense in the context of a generic shell pipeline. What's not clear to me is _why_ that is useful in the context of displaying hunks in "git add -p". The purpose of "git add -p" is to allow the user to interactively stage individual hunks. The "|" command allows the user to display the hunk in a way that helps them decide whether to stage that particular hunk. Are you able to give a specific example of a command that would help you decide whether to stage a particular hunk where you would not want to keep the escape codes? Best Wishes Phillip
Hello Phillip, On 2024-06-10 10:27, Phillip Wood wrote: > On 09/06/2024 18:20, Dragan Simic wrote: >> On 2024-06-09 16:29, phillip.wood123@gmail.com wrote: >> >>> Having read the message you referenced I'm struggling to understand >>> the use-case for stripping escape codes - what do you want to do with >>> the hunk that means you want to remove the color? >> >> Let me recap, please. >> [...] >> I hope this makes the whole thing more clear. > > It is very clear _how_ you think it should work and I agree that makes > sense in the context of a generic shell pipeline. What's not clear to > me is _why_ that is useful in the context of displaying hunks in "git > add -p". The purpose of "git add -p" is to allow the user to > interactively stage individual hunks. The "|" command allows the user > to display the hunk in a way that helps them decide whether to stage > that particular hunk. Are you able to give a specific example of a > command that would help you decide whether to stage a particular hunk > where you would not want to keep the escape codes? Well, it isn't about not _wanting_ to keep the coloring escape sequences, but about the _need_ to play it safe, so to speak. In other words, we can't know that a random "xyz", as in "|xyz", can actually handle the coloring escape sequences, so we need to be on the safe side and protect the users from being hit by garbled outputs. That's why it would be the best to have "P", "|xyz" and ">xyz" as the different "git add -p" options (piped to the configured pager with preserved escape sequences, piped to "xyz" with stripped escape sequences, and piped to "xyz" with preserved escape sequences, respectively), as I proposed earlier.
Hi Rubén On 09/06/2024 08:44, Rubén Justo wrote: > On Sat, Jun 08, 2024 at 07:54:34AM +0200, Dragan Simic wrote: > >> When "|xyz" is used instead, the version of the hunk with no coloring >> escape sequences should be piped to xyz. > > That is a sane and conservative approach, and I'm not opposed. However, > giving the colorful version though a custom pager is a good thing to > have, I think, i.e: allowing a simple "head" without losing the > coloring. > > Let's recap a bit. > > Initially, this series aimed to enable sending chunks to the pager > during "add -p" sessions. > > To reduce the blast radius of spawning a pager for each chunk, we > introduced a new command "P". > > Junio suggested opening up the command to allow specifying a custom > pager, in the form of "P<program>". > > The "P" command started to resemble a lot to the common pipe operator. > Thus, we shifted to "|<program>". > > Some concerns were raised about controlling when to send coloring escape > sequences. I'm still not really convinced that the escape sequences are a problem. As Peff has pointed out [1] this new command exists primarily to display the current hunk. I've asked for concrete examples of programs that it would be useful to run from "git add -p" where the escape codes are a problem [2,3]. Sadly the replies talked in generic terms about an imaginary program without any reference to displaying or processing a hunk from "git add -p". Without a clear use case for stripping the escape codes I think we should add a single command that pipes the colored output to a user specified program. We can make it clear in the documentation that the input to the user's command will contain escape sequences unless they pass "-c color.diff=false" when starting "git add -p". If it becomes clear that there is a use for the plain output we can add that at a later stage. Best Wishes Phillip [1] https://lore.kernel.org/git/20240605090935.GF2345232@coredump.intra.peff.net [2] https://lore.kernel.org/git/a2a59f5e-fd55-41d3-8472-b99256e1f428@gmail.com> [3] https://lore.kernel.org/git/1ae0715d-df76-4019-995e-f00f3506f2ac@gmail.com > Several ideas were discussed to address this, including > introducing a new command ">", a modifier for "|": "||", and others. > Alternatively, we could leave it up to the user to filter as needed. > Or, simply, do not send escape codes at all. > > So, looking back at the ideas discussed in the thread, perhaps a > reasonable next step might be to reintroduce the 'P<program>' command > and let '|<program>' be the way to send raw, uncolored, chunks. > > This approach makes sense to me. I'll wait a bit before sending a > reroll to gather feedback, though. > > Thanks.
Phillip Wood <phillip.wood123@gmail.com> writes: > ... We can make it > clear in the documentation that the input to the user's command will > contain escape sequences unless they pass "-c color.diff=false" when > starting "git add -p". If it becomes clear that there is a use for the > plain output we can add that at a later stage. It is a reasonable stance to take, I would think. I do not _mind_ a non-colored mode existing once we identify a use case that needs it. Those who want colored output already are known (i.e. users of color capable pagers with customization). Once we give output that are colored only, those who want plain output can make complain with concrete use cases they have. Thanks.
On Sun, Jun 09, 2024 at 09:57:20AM +0200, Dragan Simic wrote: > Hello Ruben, > > On 2024-06-09 09:44, Rubén Justo wrote: > > Some concerns were raised about controlling when to send coloring escape > > sequences. Several ideas were discussed to address this, including > > introducing a new command ">", a modifier for "|": "||", and others. > > Alternatively, we could leave it up to the user to filter as needed. > > Or, simply, do not send escape codes at all. > > > > So, looking back at the ideas discussed in the thread, perhaps a > > reasonable next step might be to reintroduce the 'P<program>' command > > and let '|<program>' be the way to send raw, uncolored, chunks. > > Actually, it would be better to re-introduce the "P" option, without > any parameters, which would display the current hunk through the > already configured pager I'm sorry, but why limit the "P" command now? I understand the caution expressed in another message of this thread about playing it safe, but I think the user won't be surprised if we respect here the "color.diff" setting, just like we do with "p", and ... > and let "|<program>" be the new option > that pipes hunks _without_ coloring escape sequences to "<program>". ... we'll offer the command "|" to allow the user to process the raw chunk.
On Mon, Jun 10, 2024 at 03:09:48PM +0100, Phillip Wood wrote:
> I'm still not really convinced that the escape sequences are a problem.
A concern about the escape sequences was mentioned in this message:
https://lore.kernel.org/git/b7e24b08-40a1-4b18-89f6-e25ab96facaf@gmail.com/
It arose when exploring the possibility of using the new command "|"
with tools that do not support escape sequences, e.g.: "| vim -", or
"| clip.exe" to send the hunk to the clipboard, on Windows.
Hello Phillip, On 2024-06-10 16:09, Phillip Wood wrote: > On 09/06/2024 08:44, Rubén Justo wrote: >> On Sat, Jun 08, 2024 at 07:54:34AM +0200, Dragan Simic wrote: >> >>> When "|xyz" is used instead, the version of the hunk with no coloring >>> escape sequences should be piped to xyz. >> >> That is a sane and conservative approach, and I'm not opposed. >> However, >> giving the colorful version though a custom pager is a good thing to >> have, I think, i.e: allowing a simple "head" without losing the >> coloring. >> >> Let's recap a bit. >> >> Initially, this series aimed to enable sending chunks to the pager >> during "add -p" sessions. >> >> To reduce the blast radius of spawning a pager for each chunk, we >> introduced a new command "P". >> >> Junio suggested opening up the command to allow specifying a custom >> pager, in the form of "P<program>". >> >> The "P" command started to resemble a lot to the common pipe operator. >> Thus, we shifted to "|<program>". >> >> Some concerns were raised about controlling when to send coloring >> escape >> sequences. > > I'm still not really convinced that the escape sequences are a > problem. As Peff has pointed out [1] this new command exists primarily > to display the current hunk. I've asked for concrete examples of > programs that it would be useful to run from "git add -p" where the > escape codes are a problem [2,3]. Sadly the replies talked in generic > terms about an imaginary program without any reference to displaying > or processing a hunk from "git add -p". Without a clear use case for > stripping the escape codes I think we should add a single command that > pipes the colored output to a user specified program. We can make it > clear in the documentation that the input to the user's command will > contain escape sequences unless they pass "-c color.diff=false" when > starting "git add -p". If it becomes clear that there is a use for the > plain output we can add that at a later stage. Regarding examples, just have a look at less(1), [4] which is probably the most commonly used pager. It requires "-R" to be specified as one of its command-line parameters for the coloring escape sequences to be processed properly. The presence of "-R" is taken as granted these days, but it hasn't always been the case. If you want to see an example of the garbled output, as a result of the coloring escape sequences not being processed correctly, just "cd" into your favorite Git repository and run the following commands (assuming you're using bash, if not, please adjust the first command): export GIT_PAGER='less' git log --patch I hope that will make more clear why I'm "advocating" for three separate new commands for "git add -p", i.e. "P", "|xyz" and ">xyz". > [1] > https://lore.kernel.org/git/20240605090935.GF2345232@coredump.intra.peff.net > [2] > https://lore.kernel.org/git/a2a59f5e-fd55-41d3-8472-b99256e1f428@gmail.com> > [3] > https://lore.kernel.org/git/1ae0715d-df76-4019-995e-f00f3506f2ac@gmail.com [4] https://man.archlinux.org/man/less.1.en
Rubén Justo <rjusto@gmail.com> writes: > On Mon, Jun 10, 2024 at 03:09:48PM +0100, Phillip Wood wrote: > >> I'm still not really convinced that the escape sequences are a problem. > > A concern about the escape sequences was mentioned in this message: > https://lore.kernel.org/git/b7e24b08-40a1-4b18-89f6-e25ab96facaf@gmail.com/ > > It arose when exploring the possibility of using the new command "|" > with tools that do not support escape sequences, e.g.: "| vim -", or > "| clip.exe" to send the hunk to the clipboard, on Windows. Let's not bikeshed this for too long. Pipe "|" stuff that is suitable for pager in the context of Git (i.e. if color.ui does not forbid coloring with "no", their usual pager is capable of color, so we send colored output to the pipe), and if somebody wants uncolored output later, they can use "P" to pipe or ">" to file or whatever other enhancement they want, but let's leave them out of this round.
Dragan Simic <dsimic@manjaro.org> writes: > If you want to see an example of the garbled output, as a result of the > coloring escape sequences not being processed correctly, just "cd" into > your favorite Git repository and run the following commands (assuming > you're using bash, if not, please adjust the first command): > > export GIT_PAGER='less' > git log --patch > > I hope that will make more clear why I'm "advocating" for three separate > new commands for "git add -p", i.e. "P", "|xyz" and ">xyz". We are talking about folks who use a pager in the context of Git, and to them, your example is totally made-up and irrelevant one. Either they use a color-enabled pager (like "less -R") or they have configured color.ui to disable coloring. So I do not see this quite as a safety issue.
Hello all, On 2024-06-10 21:09, Rubén Justo wrote: > On Sun, Jun 09, 2024 at 09:57:20AM +0200, Dragan Simic wrote: >> On 2024-06-09 09:44, Rubén Justo wrote: >> > Some concerns were raised about controlling when to send coloring escape >> > sequences. Several ideas were discussed to address this, including >> > introducing a new command ">", a modifier for "|": "||", and others. >> > Alternatively, we could leave it up to the user to filter as needed. >> > Or, simply, do not send escape codes at all. >> > >> > So, looking back at the ideas discussed in the thread, perhaps a >> > reasonable next step might be to reintroduce the 'P<program>' command >> > and let '|<program>' be the way to send raw, uncolored, chunks. >> >> Actually, it would be better to re-introduce the "P" option, without >> any parameters, which would display the current hunk through the >> already configured pager > > I'm sorry, but why limit the "P" command now? I'll explain below, and provide an alternate approach. > I understand the caution expressed in another message of this thread > about playing it safe, but I think the user won't be surprised if we > respect here the "color.diff" setting, just like we do with "p", and > ... Let me provide a quotation from git-config(1): color.diff Whether to use ANSI escape sequences to add color to patches. If this is set to always, git-diff(1), git-log(1), and git-show(1) will use color for all patches. If it is set to true or auto, those commands will only use color when output is to the terminal. If unset, then the value of color.ui is used (auto by default). The key takeaway there is "when output is to the terminal", but when you think about it, the description is a bit vague, because the output to the terminal is also piped through the default pager when running git-log(1), for example. Perhaps the more accurate wording could be "when the known destination of the output is the terminal", but that might also be a bit confusing. Thus, if we want to honor the "color.diff" setting, which I think we should, then we can do everything with only one new command for "git add -p", which would be "P<program>". It would be seen as just another "channel" for the "output to the terminal" category, and it would honor "color.diff" just like git-log(1) does, for example, both when received as "P" and as "Pxyz" from the user. As a result, there would be no way to pipe a hunk to "xyz" with the coloring escape sequences removed, unless "color.diff" is set to false. That would be the most consistent approach, because "P<program>" would be just another "channel" to reaching the terminal with the output. Also, we'd need to update the documentation for "color.diff" to include a description of the new "git add -p" command. >> and let "|<program>" be the new option >> that pipes hunks _without_ coloring escape sequences to "<program>". > > ... we'll offer the command "|" to allow the user to process the raw > chunk. Sorry, but now I have to ask why do we want that? If the goal is to open the hunk in an editor, there's already a command in "git add -p" for that purpose.
On 2024-06-10 21:14, Rubén Justo wrote: > On Mon, Jun 10, 2024 at 03:09:48PM +0100, Phillip Wood wrote: > >> I'm still not really convinced that the escape sequences are a >> problem. > > A concern about the escape sequences was mentioned in this message: > https://lore.kernel.org/git/b7e24b08-40a1-4b18-89f6-e25ab96facaf@gmail.com/ > > It arose when exploring the possibility of using the new command "|" > with tools that do not support escape sequences, e.g.: "| vim -", or > "| clip.exe" to send the hunk to the clipboard, on Windows. Hmm, I asked [1] why do we need "|xyz" and escape-sequence-free hunks, and the answer is above. Another example would be "|xclip" on Linux. With that in mind, let's have "P" and "Pxyz" that both honor "color.diff", as already proposed, [1] and "|xyz" that strips the coloring escape sequences. [1] https://lore.kernel.org/git/3268c498c9ee60803384db6db2d0e94b@manjaro.org/
On 2024-06-10 22:08, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: > >> If you want to see an example of the garbled output, as a result of >> the >> coloring escape sequences not being processed correctly, just "cd" >> into >> your favorite Git repository and run the following commands (assuming >> you're using bash, if not, please adjust the first command): >> >> export GIT_PAGER='less' >> git log --patch >> >> I hope that will make more clear why I'm "advocating" for three >> separate >> new commands for "git add -p", i.e. "P", "|xyz" and ">xyz". > > We are talking about folks who use a pager in the context of Git, > and to them, your example is totally made-up and irrelevant one. Ah, that wasn't my intention. I just wanted to provide an example that shows what a garbled output looks like, because I was under impression that Phillip asked for such an example. > Either they use a color-enabled pager (like "less -R") or they have > configured color.ui to disable coloring. So I do not see this quite > as a safety issue. Agreed, but only if we end up honoring "color.ui" and "color.diff" in the new "git add -p" command(s).
The main goal in this series is to allow using the pager when displaying hunks during "add -p" sessions, to make easier for users to review hunks longer than one screen height. This iteration, v3, introduces a new command: '|', suggested by Junio, instead of the 'P' command proposed in the previous iteration. This allows us to use the pager: (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | But also to use other programs, like: (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | head Or: (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | grep term Hopefully, we'll find a way to avoid sending ANSI codes, on demand, without disabling it entirely with color.ui=never or any other global option. To make this usable: (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | vim - However, the current functionality meets my current needs, so I'm happy with it. This, a new 'interactive.pipeCommand' setting, or a new switch: 'add -P', are left for discussing in, hopefully, a future series. One final note; I preferred to model the help text this way: y - stage this hunk n - do not stage this hunk q - quit; do not stage this hunk or any of the remaining ones a - stage this hunk and all later hunks in the file d - do not stage this hunk or any of the later hunks in the file j - leave this hunk undecided, see next undecided hunk J - leave this hunk undecided, see next hunk g - select a hunk to go to / - search for a hunk matching the given regex s - split the current hunk into smaller hunks e - manually edit the current hunk p - print the current hunk | - pipe the current hunk to the pager, or |<program> to use a program' ? - print help Instead of: y - stage this hunk n - do not stage this hunk q - quit; do not stage this hunk or any of the remaining ones a - stage this hunk and all later hunks in the file d - do not stage this hunk or any of the later hunks in the file j - leave this hunk undecided, see next undecided hunk J - leave this hunk undecided, see next hunk g - select a hunk to go to / - search for a hunk matching the given regex s - split the current hunk into smaller hunks e - manually edit the current hunk p - print the current hunk |[program] - pipe the current hunk to a program, the pager if none... ? - print help Because I believe it reads better by maintaining a single character before the dash. But I am not opposed to the latter. The series is now based on jc/add-patch-enforce-single-letter-input, which has been recently merged to master. Thanks. Rubén Justo (6): add-patch: test for 'p' command pager: do not close fd 2 unnecessarily pager: introduce wait_for_pager pager: introduce setup_custom_pager test-terminal: introduce --no-stdin-pty add-patch: introduce the command '|' add-patch.c | 17 ++++++++-- pager.c | 55 ++++++++++++++++++++++++------- pager.h | 7 +++- t/t3701-add-interactive.sh | 67 +++++++++++++++++++++++++++++--------- t/test-terminal.perl | 32 ++++++++++-------- 5 files changed, 135 insertions(+), 43 deletions(-)