[PATCH 50/78] config.txt: move pager.* to a separate file

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/config.txt   | 9 +
 Documentation/config/pager.txt | 8 
 2 files changed, 9 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/config/pager.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4e839db64c..a7b72d2722 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -379,14 +379,7 @@ include::config/notes.txt[]
 
 include::config/pack.txt[]
 
-pager.::
-   If the value is boolean, turns on or off pagination of the
-   output of a particular Git subcommand when writing to a tty.
-   Otherwise, turns on pagination for the subcommand using the
-   pager specified by the value of `pager.`.  If `--paginate`
-   or `--no-pager` is specified on the command line, it takes
-   precedence over this option.  To disable pagination for all
-   commands, set `core.pager` or `GIT_PAGER` to `cat`.
+include::config/pager.txt[]
 
 pretty.::
Alias for a --pretty= format string, as specified in
diff --git a/Documentation/config/pager.txt b/Documentation/config/pager.txt
new file mode 100644
index 00..d3731cf66c
--- /dev/null
+++ b/Documentation/config/pager.txt
@@ -0,0 +1,8 @@
+pager.::
+   If the value is boolean, turns on or off pagination of the
+   output of a particular Git subcommand when writing to a tty.
+   Otherwise, turns on pagination for the subcommand using the
+   pager specified by the value of `pager.`.  If `--paginate`
+   or `--no-pager` is specified on the command line, it takes
+   precedence over this option.  To disable pagination for all
+   commands, set `core.pager` or `GIT_PAGER` to `cat`.
-- 
2.19.1.647.g708186aaf9

[PATCH 40/59] config.txt: move pager.* to a separate file

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/config.txt   | 9 +
 Documentation/pager-config.txt | 8 
 2 files changed, 9 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/pager-config.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 45b184b247..656ae3158c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -630,14 +630,7 @@ include::notes-config.txt[]
 
 include::pack-config.txt[]
 
-pager.::
-   If the value is boolean, turns on or off pagination of the
-   output of a particular Git subcommand when writing to a tty.
-   Otherwise, turns on pagination for the subcommand using the
-   pager specified by the value of `pager.`.  If `--paginate`
-   or `--no-pager` is specified on the command line, it takes
-   precedence over this option.  To disable pagination for all
-   commands, set `core.pager` or `GIT_PAGER` to `cat`.
+include::pager-config.txt[]
 
 pretty.::
Alias for a --pretty= format string, as specified in
diff --git a/Documentation/pager-config.txt b/Documentation/pager-config.txt
new file mode 100644
index 00..d3731cf66c
--- /dev/null
+++ b/Documentation/pager-config.txt
@@ -0,0 +1,8 @@
+pager.::
+   If the value is boolean, turns on or off pagination of the
+   output of a particular Git subcommand when writing to a tty.
+   Otherwise, turns on pagination for the subcommand using the
+   pager specified by the value of `pager.`.  If `--paginate`
+   or `--no-pager` is specified on the command line, it takes
+   precedence over this option.  To disable pagination for all
+   commands, set `core.pager` or `GIT_PAGER` to `cat`.
-- 
2.19.1.647.g708186aaf9

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Sat, Jun 02, 2018 at 06:46:31AM +0200, Duy Nguyen wrote:

> > if (used_deprecated_reflog_option) {
> > -   warning("the '-l' alias for '--create-reflog' is 
> > deprecated;");
> > -   warning("it will be removed in a future version of Git");
> > +   if (list) {
> > +   warning("the '-l' option is an alias for 
> > '--create-reflog' and");
> > +   warning("has no effect in list mode. This option 
> > will soon be");
> > +   warning("removed and you should omit it (or use 
> > '--list' instead).");
> > +   } else {
> > +   warning("the '-l' alias for '--create-reflog' is 
> > deprecated;");
> > +   warning("it will be removed in a future version of 
> > Git");
> > +   }
> 
> This is already in 'next', but could you do a follow up patch to mark
> these strings for translation? While at there, concatenating them into
> full sentences would also help translators.

Already being discussed elsewhere on the thread; we're hoping for a
warning() that will auto-prefix each one.

That said, I think I'm going to re-roll this in the direction discussed
elsewhere in the thread (skipping the warning for list-mode).

-Peff

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Duy Nguyen]

On Fri, May 25, 2018 at 4:40 AM, Jeff King  wrote:
> -- >8 --
> Subject: [PATCH] branch: customize "-l" warning in list mode
>
> People mistakenly use "git branch -l", thinking that it
> triggers list mode. It doesn't, but the lack of non-option
> arguments in that command does (and the "-l" becomes a
> silent noop).
>
> Since afc968e579 (branch: deprecate "-l" option, 2018-03-26)
> we've warned that "-l" is going away. But the warning text
> is primarily aimed at people who _meant_ to use "-l", as in
> "git branch -l foo". People who mistakenly said "git branch
> -l" may be left puzzled.
>
> Let's make it clear that:
>
>   1. No, "-l" didn't do what they thought here.
>
>   2. It's going away, and what they should do instead.
>
> Signed-off-by: Jeff King 
> ---
>  builtin/branch.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 55bfacd843..b0b33dab94 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -701,8 +701,14 @@ int cmd_branch(int argc, const char **argv, const char 
> *prefix)
> setup_auto_pager("branch", 1);
>
> if (used_deprecated_reflog_option) {
> -   warning("the '-l' alias for '--create-reflog' is 
> deprecated;");
> -   warning("it will be removed in a future version of Git");
> +   if (list) {
> +   warning("the '-l' option is an alias for 
> '--create-reflog' and");
> +   warning("has no effect in list mode. This option will 
> soon be");
> +   warning("removed and you should omit it (or use 
> '--list' instead).");
> +   } else {
> +   warning("the '-l' alias for '--create-reflog' is 
> deprecated;");
> +   warning("it will be removed in a future version of 
> Git");
> +   }

This is already in 'next', but could you do a follow up patch to mark
these strings for translation? While at there, concatenating them into
full sentences would also help translators.

> }
>
> if (delete) {
> --
> 2.17.0.1391.g6fdbf40724
>



-- 
Duy

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Jeff King  writes:

> So I think you're proposing:
>
>   - step 0: warn about "-l" when it actually gets used, and otherwise
> continue ignoring
>
>   - step 1: turn "-l" into "--list"
>
>   - step 2: there is no step 2
>
> ... So I
> guess the right rule is to warn when we are not in list-mode, and
> otherwise quietly accept it.
>
> That does mean that anybody who misses the deprecation warning may be
> surprised when "branch -l foo" starts listing instead of creating a
> branch with a reflog (whereas in the current 3-step plan, we have a
> period in the middle where that's a hard error). That may be OK, though,
> and is a natural consequence of getting to the end step sooner (even
> with a 3-step plan, anybody who skips the versions in the middle _could_
> be surprised).

Thanks for a concise and readably summary ;-)

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Kaartic.Sivaraam]

On Wednesday 30 May 2018 08:22 AM, Junio C Hamano wrote:

Jeff King  writes:


Right, what I meant by "gentler" is that we continue to perform the same
behavior as the old version, alongside the warning. It's arguable here
because running "git branch -l" has _always_ been wrong. It's just wrong
in a way that happens to do what the user wants. ;)
...

Anyways, if you think it mustn't turn into an error now and only in the
next stage, a suggestion follows in another thread.


I don't think "mustn't", but I have a slight preference for what I
posted, as I think it is a little friendlier during the transition (at
the risk of somebody missing the warning, but then step 2 turns it into
a hard error anyway, so they'll certainly find out then).


Well, we could keep treating '-l' given in contexts where we have
silently ignored the option and did "list" instead as before during
the transition, until the very end where it becomes an explicit
"list" command, no?  Then there is no need to even warn against '-l'
that is ignored because we are listing in the earliest step.  The
only usage that requires a warning then becomes '-l' used for its
original meaning to create a reflog, right?  That sounds gentler to
me.



That sounds interesting. I guess that would avoid the confusion I was 
speaking of from the beginning of this thread as the warning message 
would not be shown at all for "git branch -l". Of course, it would then 
confuse people who discover that "git branch -l" lists and "git branch 
-l $prefix" creates a branch with name "$prefix" (if it's valid) instead 
of listing branch names with prefix "$prefix". So, it might be worth 
considering.


BTW, I suspect this would make the deprecation of "-l" a little unsual 
as the no. of people who see the deprecation warning would be less as 
the warning is shown only for "git branch -l $branch" and I also suspect 
the no. of users of that command would be very less as previously 
pointed by someone elsewhere.


--
Sivaraam

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Wed, May 30, 2018 at 11:52:19AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Right, what I meant by "gentler" is that we continue to perform the same
> > behavior as the old version, alongside the warning. It's arguable here
> > because running "git branch -l" has _always_ been wrong. It's just wrong
> > in a way that happens to do what the user wants. ;)
> > ...
> >> Anyways, if you think it mustn't turn into an error now and only in the
> >> next stage, a suggestion follows in another thread.
> >
> > I don't think "mustn't", but I have a slight preference for what I
> > posted, as I think it is a little friendlier during the transition (at
> > the risk of somebody missing the warning, but then step 2 turns it into
> > a hard error anyway, so they'll certainly find out then).
> 
> Well, we could keep treating '-l' given in contexts where we have
> silently ignored the option and did "list" instead as before during
> the transition, until the very end where it becomes an explicit
> "list" command, no?  Then there is no need to even warn against '-l'
> that is ignored because we are listing in the earliest step.  The
> only usage that requires a warning then becomes '-l' used for its
> original meaning to create a reflog, right?  That sounds gentler to
> me.

So I think you're proposing:

  - step 0: warn about "-l" when it actually gets used, and otherwise
continue ignoring

  - step 1: turn "-l" into "--list"

  - step 2: there is no step 2

I can't think of any reason that would work, and it lets us reclaim it
for "--list" sooner. I guess "when it gets used" is maybe not the right
criterion. We'd warn on:

  git branch -l foo

But we wouldn't on:

  git branch -d -l foo

That's clearly nonsense, but we're probably better off complaining. So I
guess the right rule is to warn when we are not in list-mode, and
otherwise quietly accept it.

That does mean that anybody who misses the deprecation warning may be
surprised when "branch -l foo" starts listing instead of creating a
branch with a reflog (whereas in the current 3-step plan, we have a
period in the middle where that's a hard error). That may be OK, though,
and is a natural consequence of getting to the end step sooner (even
with a 3-step plan, anybody who skips the versions in the middle _could_
be surprised).

-Peff

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Wed, May 30, 2018 at 11:48:32AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> >> -  if (list) {
> >> -  warning("the '-l' option is an alias for 
> >> '--create-reflog' and");
> >> -  warning("has no effect in list mode. This option will 
> >> soon be");
> >> -  warning("removed and you should omit it (or use 
> >> '--list' instead).");
> >> -  } else {
> >> -  warning("the '-l' alias for '--create-reflog' is 
> >> deprecated;");
> >> -  warning("it will be removed in a future version of 
> >> Git");
> >> -  }
> >> -  }
> >> -
> >
> > Oh, and did we want to mark these for translation on the step 0 branch?
> > Obviously that would impact this hunk.
> 
> I was hoping that we can settle the "multi-line message translation
> that can potentially result in different number of lines" issue
> before we have to mark the above for translation ;-)

Yeah, right after saying that I realized it would create horrible
translation-lego. I agree we should punt for now.

-Peff

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Jeff King  writes:

> Right, what I meant by "gentler" is that we continue to perform the same
> behavior as the old version, alongside the warning. It's arguable here
> because running "git branch -l" has _always_ been wrong. It's just wrong
> in a way that happens to do what the user wants. ;)
> ...
>> Anyways, if you think it mustn't turn into an error now and only in the
>> next stage, a suggestion follows in another thread.
>
> I don't think "mustn't", but I have a slight preference for what I
> posted, as I think it is a little friendlier during the transition (at
> the risk of somebody missing the warning, but then step 2 turns it into
> a hard error anyway, so they'll certainly find out then).

Well, we could keep treating '-l' given in contexts where we have
silently ignored the option and did "list" instead as before during
the transition, until the very end where it becomes an explicit
"list" command, no?  Then there is no need to even warn against '-l'
that is ignored because we are listing in the earliest step.  The
only usage that requires a warning then becomes '-l' used for its
original meaning to create a reflog, right?  That sounds gentler to
me.

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Jeff King  writes:

>> -if (list) {
>> -warning("the '-l' option is an alias for 
>> '--create-reflog' and");
>> -warning("has no effect in list mode. This option will 
>> soon be");
>> -warning("removed and you should omit it (or use 
>> '--list' instead).");
>> -} else {
>> -warning("the '-l' alias for '--create-reflog' is 
>> deprecated;");
>> -warning("it will be removed in a future version of 
>> Git");
>> -}
>> -}
>> -
>
> Oh, and did we want to mark these for translation on the step 0 branch?
> Obviously that would impact this hunk.

I was hoping that we can settle the "multi-line message translation
that can potentially result in different number of lines" issue
before we have to mark the above for translation ;-)

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Tue, May 29, 2018 at 05:20:29PM -0400, Jeff King wrote:

> Thanks. There's one bit missing here, because it did not cause a textual
> conflict during the rebase (but it's now dead code). Patch below (to be
> squashed to the tip of jk/branch-l-1-removal).
> [...]
> - if (used_deprecated_reflog_option) {
> - if (list) {
> - warning("the '-l' option is an alias for 
> '--create-reflog' and");
> - warning("has no effect in list mode. This option will 
> soon be");
> - warning("removed and you should omit it (or use 
> '--list' instead).");
> - } else {
> - warning("the '-l' alias for '--create-reflog' is 
> deprecated;");
> - warning("it will be removed in a future version of 
> Git");
> - }
> - }
> -

Oh, and did we want to mark these for translation on the step 0 branch?
Obviously that would impact this hunk.

-Peff

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Sat, May 26, 2018 at 11:32:35AM +0900, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > Yup, thanks for being extra explicit.  I do imagine there are quite
> > a few of us who would be puzzled without this update (but with the
> > previous one to unhide it from behind the pager).
> 
> With these two patches queued on top of jk/branch-l-0-deprecation,
> the follow-up patches jk/branch-l-1-removal that makes 'branch -l'
> to fail and then jk/branch-l-2-reincarnation that makes 'branch -l'
> a synonym to 'branch --list' needs rebasing.  Both are trivial and
> hopefully I did them correctly.
> -- >8 --
> From: Jeff King 
> Date: Mon, 26 Mar 2018 03:29:22 -0400
> Subject: [PATCH] branch: drop deprecated "-l" option
> 
> We marked the "-l" option as deprecated back in  here>. Now that sufficient time has passed, let's follow
> through and get rid of it.

Thanks. There's one bit missing here, because it did not cause a textual
conflict during the rebase (but it's now dead code). Patch below (to be
squashed to the tip of jk/branch-l-1-removal).

We may also want to fill in . I think it's afc968e579
(branch: deprecate "-l" option, 2018-03-26), which should be stable at
this point (it's already merged to 'next').

diff --git a/builtin/branch.c b/builtin/branch.c
index 01b35b3c3d..f7cd333587 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -34,7 +34,6 @@ static const char * const builtin_branch_usage[] = {
NULL
 };
 
-static int used_deprecated_reflog_option;
 static const char *head;
 static struct object_id head_oid;
 
@@ -686,17 +685,6 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
if (list)
setup_auto_pager("branch", 1);
 
-   if (used_deprecated_reflog_option) {
-   if (list) {
-   warning("the '-l' option is an alias for 
'--create-reflog' and");
-   warning("has no effect in list mode. This option will 
soon be");
-   warning("removed and you should omit it (or use 
'--list' instead).");
-   } else {
-   warning("the '-l' alias for '--create-reflog' is 
deprecated;");
-   warning("it will be removed in a future version of 
Git");
-   }
-   }
-
if (delete) {
if (!argc)
die(_("branch name required"));

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Sun, May 27, 2018 at 12:15:40AM +0530, Kaartic Sivaraam wrote:

> > Hmm, actually, I suppose the true value of the warning is to help people
> > doing "git branch -l foo", and it would still work there. The "more
> > extreme" from your suggested patch would only affect "branch -l".
> > 
> > Still, I think I prefer the gentler version that we get by keeping it as
> > a warning even in the latter case.
> >
> 
> I never wanted to suppress the warning message in the latter case. I
> just wanted to avoid listing the branches. Actually the patch I sent in
> one the previous threads[1] that avoids listing the branches has the
> following behaviour,
> 
> $ git branch -l
> warning: the '-l' alias for '--create-reflog' is deprecated;
> warning: it will be removed in a future version of Git
> usage: git branch [] [-r | -a] [--merged | --no-merged]
>or: git branch [] [-l] [-f]  []
>or: git branch [] [-r] (-d | -D) ...
>or: git branch [] (-m | -M) [] 
>or: git branch [] (-c | -C) [] 
>or: git branch [] [-r | -a] [--points-at]
>or: git branch [] [-r | -a] [--format]
> 
> 
> So, the warning message isn't lost. It just prevents the listing of
> branches.

Right, what I meant by "gentler" is that we continue to perform the same
behavior as the old version, alongside the warning. It's arguable here
because running "git branch -l" has _always_ been wrong. It's just wrong
in a way that happens to do what the user wants. ;)

> Wait, maybe I'm misunderstanding what you mean by "warning". You're
> probably meaning something related to the way Git exits in both cases.
> With your patch "git branch -l" prints a warning, lists the branches and
> has an exit status of 0. With my patch it prints the warning, the usage
> specifications with exit status 128. In that case, I still don't think
> it's bad to turn "git branch -l" into an error now as it's been
> incorrect for a long time now and it's not wrong if we correct it now.
> 
> Anyways, if you think it mustn't turn into an error now and only in the
> next stage, a suggestion follows in another thread.

I don't think "mustn't", but I have a slight preference for what I
posted, as I think it is a little friendlier during the transition (at
the risk of somebody missing the warning, but then step 2 turns it into
a hard error anyway, so they'll certainly find out then).

-Peff

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Kaartic Sivaraam]

On Friday 25 May 2018 08:10 AM, Jeff King wrote:
> Subject: [PATCH] branch: customize "-l" warning in list mode
> 
> People mistakenly use "git branch -l", thinking that it
> triggers list mode. It doesn't, but the lack of non-option
> arguments in that command does (and the "-l" becomes a
> silent noop).
> > Since afc968e579 (branch: deprecate "-l" option, 2018-03-26)
> 
> we've warned that "-l" is going away. But the warning text
> is primarily aimed at people who _meant_ to use "-l", as in
> "git branch -l foo". People who mistakenly said "git branch
> -l" may be left puzzled.
> 

So, this patch is to improve the user experience of people who use "git
branch -l" for listing and not for the people who forget to give a new
branch name argument for "-l". In that case, this makes sense.

BTW, I hope people don't start wondering why "git branch -d" doesn't
trigger list mode ;-)


> + warning("the '-l' option is an alias for 
> '--create-reflog' and");
> + warning("has no effect in list mode. This option will 
> soon be");
> + warning("removed and you should omit it (or use 
> '--list' instead).");

I suppose s/alias/deprecated alias/ makes the point that '-l' will be
removed more stronger.


-- 
Sivaraam

signature.asc
Description: This is a digitally signed message part

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Kaartic Sivaraam]

On Friday 25 May 2018 01:01 AM, Jeff King wrote:
> On Thu, May 24, 2018 at 03:22:14PM -0400, Jeff King wrote:
> 
> Hmm, actually, I suppose the true value of the warning is to help people
> doing "git branch -l foo", and it would still work there. The "more
> extreme" from your suggested patch would only affect "branch -l".
> 
> Still, I think I prefer the gentler version that we get by keeping it as
> a warning even in the latter case.
>

I never wanted to suppress the warning message in the latter case. I
just wanted to avoid listing the branches. Actually the patch I sent in
one the previous threads[1] that avoids listing the branches has the
following behaviour,

$ git branch -l
warning: the '-l' alias for '--create-reflog' is deprecated;
warning: it will be removed in a future version of Git
usage: git branch [] [-r | -a] [--merged | --no-merged]
   or: git branch [] [-l] [-f]  []
   or: git branch [] [-r] (-d | -D) ...
   or: git branch [] (-m | -M) [] 
   or: git branch [] (-c | -C) [] 
   or: git branch [] [-r | -a] [--points-at]
   or: git branch [] [-r | -a] [--format]


So, the warning message isn't lost. It just prevents the listing of
branches.

Wait, maybe I'm misunderstanding what you mean by "warning". You're
probably meaning something related to the way Git exits in both cases.
With your patch "git branch -l" prints a warning, lists the branches and
has an exit status of 0. With my patch it prints the warning, the usage
specifications with exit status 128. In that case, I still don't think
it's bad to turn "git branch -l" into an error now as it's been
incorrect for a long time now and it's not wrong if we correct it now.

Anyways, if you think it mustn't turn into an error now and only in the
next stage, a suggestion follows in another thread.


[1]: https://public-inbox.org/git/1527174618.10589.4.ca...@gmail.com/


-- 
Sivaraam

QUOTE:

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

  - John Sonmez


Sivaraam?

You possibly might have noticed that my signature recently changed from
'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the
new signature to be better for several reasons one of which is that the
former signature has a lot of ambiguities in the place I live as it is a
common name (NOTE: it's not a common spelling, just a common name). So,
I switched signatures before it's too late.

That said, I won't mind you calling me 'Kaartic' if you like it [of
course ;-)]. You can always call me using either of the names.


KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

As I'm not a native English speaker myself, there might be mistaeks in
my usage of English. I apologise for any mistakes that I make.

It would be "helpful" if you take the time to point out the mistakes.

It would be "super helpful" if you could provide suggestions about how
to correct those mistakes.

Thanks in advance!



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Jeff King  writes:

>> By the way, this is one of these times when I feel that we should
>> have a better multi-line message support in die/error/warning/info
>> functions.  Ideally, I should be able to write
>> 
>>  warning(_("the '-l' option is an alias for '--create-reflog' and\n"
>>"has no effect in list mode, This option will soon be\n"
>>"removed and you should omit it (or use '--list' instead)."));
>> 
>> and warning() would:
>> 
>>  - do the sprintf formatting thing as necessary to prepare a long multi-line
>>message;
>> 
>>  - chomp that into lines at '\n' boundary; and
>> 
>>  - give each of these lines with _("warning: ") prefixed.
>> 
>> That way, translators can choose to make the resulting message to
>> different number of lines from the original easily.
>
> Yep, I totally agree. In past discussions I was thinking mostly of
> the pain of writing these multi-line messages. But I imagine it is
> absolute hell for translators, and we should fix it for that reason.
>
> (Also, I guess this message probably ought to be marked for
> translation).

Needless to tell you, I worked backwards when noticing that these
three warning() lines are not marked for translation ;-).

But of course, fixing this in a naïve way will involve memory
allocation during execution of die(), which may well be due to OOM,
which is why we knew the need but haven't found a good solution to
it.

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Junio C Hamano  writes:

> With these two patches queued on top of jk/branch-l-0-deprecation,
> the follow-up patches jk/branch-l-1-removal that makes 'branch -l'
> to fail and then jk/branch-l-2-reincarnation that makes 'branch -l'
> a synonym to 'branch --list' needs rebasing.  Both are trivial and
> hopefully I did them correctly.
>
> -- >8 --
> From: Jeff King 
> Date: Mon, 26 Mar 2018 03:29:22 -0400
> Subject: [PATCH] branch: drop deprecated "-l" option

And this is the final "reincarnation" step.

-- >8 --
From: Jeff King 
Date: Mon, 26 Mar 2018 03:29:48 -0400
Subject: [PATCH] branch: make "-l" a synonym for "--list"

The other "mode" options of git-branch have short-option
aliases that are easy to type (e.g., "-d" and "-m"). Let's
give "--list" the same treatment.

This also makes it consistent with the similar "git tag -l"
option.

We didn't do this originally because "--create-reflog" was
squatting on the "-l" option. Now that sufficient time has
passed with that alias removed, we can finally repurpose it.

Signed-off-by: Jeff King 
Reviewed-by: Eric Sunshine 
Reviewed-by: Jacob Keller 
[jc: adjusted the new tests added earlier]
Signed-off-by: Junio C Hamano 
---
 builtin/branch.c  | 2 +-
 t/t3200-branch.sh | 8 +---
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 01b35b3c3d..1d06f5c547 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -612,7 +612,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPT_BIT('M', NULL, , N_("move/rename a branch, even if 
target exists"), 2),
OPT_BIT('c', "copy", , N_("copy a branch and its reflog"), 
1),
OPT_BIT('C', NULL, , N_("copy a branch, even if target 
exists"), 2),
-   OPT_BOOL(0, "list", , N_("list branch names")),
+   OPT_BOOL('l', "list", , N_("list branch names")),
OPT_BOOL(0, "create-reflog", , N_("create the branch's 
reflog")),
OPT_BOOL(0, "edit-description", _description,
 N_("edit the description for the branch")),
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index eca75d3ca1..022d6a41c8 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -45,12 +45,6 @@ test_expect_success 'git branch HEAD should fail' '
test_must_fail git branch HEAD
 '
 
-test_expect_success 'git branch -l no longer is --create-reflog' '
-   test_when_finished "git branch -D new-branch-with-reflog || :" &&
-   test_must_fail git branch -l new-branch-with-reflog &&
-   test_must_fail git rev-parse --verify refs/heads/new-branch-with-reflog
-'
-
 cat >expect < 1117150200 +
branch: Created from master
 EOF
@@ -294,7 +288,7 @@ test_expect_success 'git branch --list -v with --abbrev' '
 
 '
 
-test_expect_failure 'git branch -l eventually is --list' '
+test_expect_success 'git branch -l is --list' '
git branch --list >expect &&
git branch -l >actual &&
test_cmp expect actual
-- 
2.17.0-775-ge144d126d7

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Junio C Hamano <gits...@pobox.com> writes:

> Yup, thanks for being extra explicit.  I do imagine there are quite
> a few of us who would be puzzled without this update (but with the
> previous one to unhide it from behind the pager).

With these two patches queued on top of jk/branch-l-0-deprecation,
the follow-up patches jk/branch-l-1-removal that makes 'branch -l'
to fail and then jk/branch-l-2-reincarnation that makes 'branch -l'
a synonym to 'branch --list' needs rebasing.  Both are trivial and
hopefully I did them correctly.

-- >8 --
From: Jeff King <p...@peff.net>
Date: Mon, 26 Mar 2018 03:29:22 -0400
Subject: [PATCH] branch: drop deprecated "-l" option

We marked the "-l" option as deprecated back in . Now that sufficient time has passed, let's follow
through and get rid of it.

Signed-off-by: Jeff King <p...@peff.net>
Reviewed-by: Eric Sunshine <sunsh...@sunshineco.com>
Reviewed-by: Jacob Keller <jacob.kel...@gmail.com>
[jc: added a few tests]
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 builtin/branch.c  | 14 --
 t/t3200-branch.sh | 12 
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b0b33dab94..01b35b3c3d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -571,14 +571,6 @@ static int edit_branch_description(const char *branch_name)
return 0;
 }
 
-static int deprecated_reflog_option_cb(const struct option *opt,
-  const char *arg, int unset)
-{
-   used_deprecated_reflog_option = 1;
-   *(int *)opt->value = !unset;
-   return 0;
-}
-
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
int delete = 0, rename = 0, copy = 0, force = 0, list = 0;
@@ -622,12 +614,6 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPT_BIT('C', NULL, , N_("copy a branch, even if target 
exists"), 2),
OPT_BOOL(0, "list", , N_("list branch names")),
OPT_BOOL(0, "create-reflog", , N_("create the branch's 
reflog")),
-   {
-   OPTION_CALLBACK, 'l', NULL, , NULL,
-   N_("deprecated synonym for --create-reflog"),
-   PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
-   deprecated_reflog_option_cb
-   },
OPT_BOOL(0, "edit-description", _description,
 N_("edit the description for the branch")),
OPT__FORCE(, N_("force creation, move/rename, deletion"), 
PARSE_OPT_NOCOMPLETE),
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index da97b8a62b..eca75d3ca1 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -45,6 +45,12 @@ test_expect_success 'git branch HEAD should fail' '
test_must_fail git branch HEAD
 '
 
+test_expect_success 'git branch -l no longer is --create-reflog' '
+   test_when_finished "git branch -D new-branch-with-reflog || :" &&
+   test_must_fail git branch -l new-branch-with-reflog &&
+   test_must_fail git rev-parse --verify refs/heads/new-branch-with-reflog
+'
+
 cat >expect < 1117150200 +
branch: Created from master
 EOF
@@ -288,6 +294,12 @@ test_expect_success 'git branch --list -v with --abbrev' '
 
 '
 
+test_expect_failure 'git branch -l eventually is --list' '
+   git branch --list >expect &&
+   git branch -l >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'git branch --column' '
COLUMNS=81 git branch --column=column >actual &&
cat >expected <<\EOF &&
-- 
2.17.0-775-ge144d126d7

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Fri, May 25, 2018 at 06:14:16PM +0900, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> >> -  warning("the '-l' alias for '--create-reflog' is deprecated;");
> >> -  warning("it will be removed in a future version of Git");
> >> +  if (list) {
> >> +  warning("the '-l' option is an alias for 
> >> '--create-reflog' and");
> >> +  warning("has no effect in list mode. This option will 
> >> soon be");
> >> +  warning("removed and you should omit it (or use 
> >> '--list' instead).");
> >> +  } else {
> >> +  warning("the '-l' alias for '--create-reflog' is 
> >> deprecated;");
> >> +  warning("it will be removed in a future version of 
> >> Git");
> >> +  }
> 
> By the way, this is one of these times when I feel that we should
> have a better multi-line message support in die/error/warning/info
> functions.  Ideally, I should be able to write
> 
>   warning(_("the '-l' option is an alias for '--create-reflog' and\n"
> "has no effect in list mode, This option will soon be\n"
> "removed and you should omit it (or use '--list' instead)."));
> 
> and warning() would:
> 
>  - do the sprintf formatting thing as necessary to prepare a long multi-line
>message;
> 
>  - chomp that into lines at '\n' boundary; and
> 
>  - give each of these lines with _("warning: ") prefixed.
> 
> That way, translators can choose to make the resulting message to
> different number of lines from the original easily.

Yep, I totally agree. In past discussions I was thinking mostly of
the pain of writing these multi-line messages. But I imagine it is
absolute hell for translators, and we should fix it for that reason.

(Also, I guess this message probably ought to be marked for
translation).

-Peff

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Junio C Hamano  writes:

>> -warning("the '-l' alias for '--create-reflog' is deprecated;");
>> -warning("it will be removed in a future version of Git");
>> +if (list) {
>> +warning("the '-l' option is an alias for 
>> '--create-reflog' and");
>> +warning("has no effect in list mode. This option will 
>> soon be");
>> +warning("removed and you should omit it (or use 
>> '--list' instead).");
>> +} else {
>> +warning("the '-l' alias for '--create-reflog' is 
>> deprecated;");
>> +warning("it will be removed in a future version of 
>> Git");
>> +}

By the way, this is one of these times when I feel that we should
have a better multi-line message support in die/error/warning/info
functions.  Ideally, I should be able to write

warning(_("the '-l' option is an alias for '--create-reflog' and\n"
  "has no effect in list mode, This option will soon be\n"
  "removed and you should omit it (or use '--list' instead)."));

and warning() would:

 - do the sprintf formatting thing as necessary to prepare a long multi-line
   message;

 - chomp that into lines at '\n' boundary; and

 - give each of these lines with _("warning: ") prefixed.

That way, translators can choose to make the resulting message to
different number of lines from the original easily.

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Jeff King <p...@peff.net> writes:

> People mistakenly use "git branch -l", thinking that it
> triggers list mode. It doesn't, but the lack of non-option
> arguments in that command does (and the "-l" becomes a
> silent noop).
>
> Since afc968e579 (branch: deprecate "-l" option, 2018-03-26)
> we've warned that "-l" is going away. But the warning text
> is primarily aimed at people who _meant_ to use "-l", as in
> "git branch -l foo". People who mistakenly said "git branch
> -l" may be left puzzled.

Yup, thanks for being extra explicit.  I do imagine there are quite
a few of us who would be puzzled without this update (but with the
previous one to unhide it from behind the pager).

> Let's make it clear that:
>
>   1. No, "-l" didn't do what they thought here.
>
>   2. It's going away, and what they should do instead.
>
> Signed-off-by: Jeff King <p...@peff.net>
> ---
>  builtin/branch.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 55bfacd843..b0b33dab94 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -701,8 +701,14 @@ int cmd_branch(int argc, const char **argv, const char 
> *prefix)
>   setup_auto_pager("branch", 1);
>  
>   if (used_deprecated_reflog_option) {
> - warning("the '-l' alias for '--create-reflog' is deprecated;");
> - warning("it will be removed in a future version of Git");
> + if (list) {
> + warning("the '-l' option is an alias for 
> '--create-reflog' and");
> + warning("has no effect in list mode. This option will 
> soon be");
> + warning("removed and you should omit it (or use 
> '--list' instead).");
> + } else {
> + warning("the '-l' alias for '--create-reflog' is 
> deprecated;");
> + warning("it will be removed in a future version of 
> Git");
> + }
>   }
>  
>   if (delete) {

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Fri, May 25, 2018 at 10:55:45AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Hmm, actually, I suppose the true value of the warning is to help people
> > doing "git branch -l foo", and it would still work there. The "more
> > extreme" from your suggested patch would only affect "branch -l".
> 
> > Still, I think I prefer the gentler version that we get by keeping it as
> > a warning even in the latter case.
> 
> "git branch -l newbranch [forkpoint]" that warns "We won't be doing
> reflog creation with -l" is good, but "git branch -l" that warns "We
> won't be doing reflog creation with -l" sounds like a pure noise, as
> the user would say "Irrelevant, I am not doing that anyway--I am
> listing".
> 
> The warning to prepare users for the next step jk/branch-l-1-removal
> should say "we won't be accepting '-l' as a silent and unadvertised
> synonym soon. Spell it as --list" when "git branch -l" is given, I
> would think.

I hoped that reminding them that "-l is a synonym for --create-reflog"
would serve as a gentle reminder that they're Doing It Wrong. I guess we
could be more explicit, though.

It is not "we won't be accepting -l as a synonym" though. It was never a
synonym, it's just that it didn't happen to do anything in list mode.

> > @@ -700,6 +700,11 @@ int cmd_branch(int argc, const char **argv, const char 
> > *prefix)
> > if (list)
> > setup_auto_pager("branch", 1);
> >  
> > +   if (used_deprecated_reflog_option) {
> > +   warning("the '-l' alias for '--create-reflog' is deprecated;");
> > +   warning("it will be removed in a future version of Git");
> > +   }
> 
> So from that point of view, we may need a separate message to warn
> users who _do_ want listing with '-l' before jk/branch-l-1-removal
> removes it?  
> 
> The jk/branch-l-2-resurrection topic later repurposes '-l' for
> '--list' but until that happens 'git branch -l' will error not, no?

Yes, after step 1 it will error out. Again, I hoped the existing message
would prepare people. But maybe we should do this on top of what I
posted earlier?

-- >8 --
Subject: [PATCH] branch: customize "-l" warning in list mode

People mistakenly use "git branch -l", thinking that it
triggers list mode. It doesn't, but the lack of non-option
arguments in that command does (and the "-l" becomes a
silent noop).

Since afc968e579 (branch: deprecate "-l" option, 2018-03-26)
we've warned that "-l" is going away. But the warning text
is primarily aimed at people who _meant_ to use "-l", as in
"git branch -l foo". People who mistakenly said "git branch
-l" may be left puzzled.

Let's make it clear that:

  1. No, "-l" didn't do what they thought here.

  2. It's going away, and what they should do instead.

Signed-off-by: Jeff King 
---
 builtin/branch.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 55bfacd843..b0b33dab94 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -701,8 +701,14 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
setup_auto_pager("branch", 1);
 
if (used_deprecated_reflog_option) {
-   warning("the '-l' alias for '--create-reflog' is deprecated;");
-   warning("it will be removed in a future version of Git");
+   if (list) {
+   warning("the '-l' option is an alias for 
'--create-reflog' and");
+   warning("has no effect in list mode. This option will 
soon be");
+   warning("removed and you should omit it (or use 
'--list' instead).");
+   } else {
+   warning("the '-l' alias for '--create-reflog' is 
deprecated;");
+   warning("it will be removed in a future version of 
Git");
+   }
}
 
if (delete) {
-- 
2.17.0.1391.g6fdbf40724

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Jeff King  writes:

> Hmm, actually, I suppose the true value of the warning is to help people
> doing "git branch -l foo", and it would still work there. The "more
> extreme" from your suggested patch would only affect "branch -l".

> Still, I think I prefer the gentler version that we get by keeping it as
> a warning even in the latter case.

"git branch -l newbranch [forkpoint]" that warns "We won't be doing
reflog creation with -l" is good, but "git branch -l" that warns "We
won't be doing reflog creation with -l" sounds like a pure noise, as
the user would say "Irrelevant, I am not doing that anyway--I am
listing".

The warning to prepare users for the next step jk/branch-l-1-removal
should say "we won't be accepting '-l' as a silent and unadvertised
synonym soon. Spell it as --list" when "git branch -l" is given, I
would think.

> @@ -700,6 +700,11 @@ int cmd_branch(int argc, const char **argv, const char 
> *prefix)
>   if (list)
>   setup_auto_pager("branch", 1);
>  
> + if (used_deprecated_reflog_option) {
> + warning("the '-l' alias for '--create-reflog' is deprecated;");
> + warning("it will be removed in a future version of Git");
> + }

So from that point of view, we may need a separate message to warn
users who _do_ want listing with '-l' before jk/branch-l-1-removal
removes it?  

The jk/branch-l-2-resurrection topic later repurposes '-l' for
'--list' but until that happens 'git branch -l' will error not, no?

[PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Thu, May 24, 2018 at 03:22:14PM -0400, Jeff King wrote:

> On Thu, May 24, 2018 at 08:40:18PM +0530, Kaartic Sivaraam wrote:
> 
> > > On the other hand, I'm not sure this is that big a deal. The point of
> > > the deprecation warning is to catch people who are actually trying to
> > > use "-l" as "--create-reflog", and that case does not page. The people
> > > doing "git branch -l" are actually getting what they want eventually,
> > > which is to turn it into "--list". In the interim step where it becomes
> > > an unknown option, they'll get a hard error. 
> > 
> > I just thought we wouldn't want to surprise/confuse users who try to
> > use "git branch -l" with the warning message about "create reflog"
> > along-side the list of branches. That would just add to the confusion.
> > So, I thought we should error out when users do "git branch -l"
> > instead. Something like the following should help us prevent "git
> > branch -l" from listing branch names and might also prevent the
> > confusion.
> 
> Yeah, I think that's just a more extreme version of the current plan (it
> turns it immediately into a hard error instead of warning for a while).
> If we just make the warning easier to see in the paged case, I think
> that makes the current plan fine.
> 
> I'll wrap up the patch I sent earlier.

Hmm, actually, I suppose the true value of the warning is to help people
doing "git branch -l foo", and it would still work there. The "more
extreme" from your suggested patch would only affect "branch -l".

Still, I think I prefer the gentler version that we get by keeping it as
a warning even in the latter case.

Here's the patch. It goes on top of jk/branch-l-0-deprecation (and will
naturally conflict with the removal branch, but the resolution should be
obvious).

-- >8 --
Subject: [PATCH] branch: issue "-l" deprecation warning after pager starts

If you run "git branch -l", we issue a deprecation warning
that "-l" is probably not doing what you expect. But we do
so while parsing the options, which is _before_ we start the
pager. Depending on your pager settings, this means that the
warning may get totally overwritten by the pager.

Instead, let's delay the message until after we would have
started the pager. If we do page, then it will end up inside
the pager (since we redirect stderr). And if not (including
the case when you really did mean for "-l" to work as
"--create-reflog"), then it will still get shown on stderr.

Signed-off-by: Jeff King <p...@peff.net>
---
 builtin/branch.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index e50a5a1680..55bfacd843 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -34,6 +34,7 @@ static const char * const builtin_branch_usage[] = {
NULL
 };
 
+static int used_deprecated_reflog_option;
 static const char *head;
 static struct object_id head_oid;
 
@@ -573,8 +574,7 @@ static int edit_branch_description(const char *branch_name)
 static int deprecated_reflog_option_cb(const struct option *opt,
   const char *arg, int unset)
 {
-   warning("the '-l' alias for '--create-reflog' is deprecated;");
-   warning("it will be removed in a future version of Git");
+   used_deprecated_reflog_option = 1;
*(int *)opt->value = !unset;
return 0;
 }
@@ -700,6 +700,11 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
if (list)
setup_auto_pager("branch", 1);
 
+   if (used_deprecated_reflog_option) {
+   warning("the '-l' alias for '--create-reflog' is deprecated;");
+   warning("it will be removed in a future version of Git");
+   }
+
if (delete) {
if (!argc)
die(_("branch name required"));
-- 
2.17.0.1391.g6fdbf40724

Re: [PATCH] pager: set COLUMNS to term_columns()

[Duy Nguyen]

On Fri, May 11, 2018 at 11:25 AM, Jeff King <p...@peff.net> wrote:
> --- a/pager.c
> +++ b/pager.c
> @@ -109,10 +109,15 @@ void setup_pager(void)
> return;
>
> /*
> -* force computing the width of the terminal before we redirect
> -    * the standard output to the pager.
> +* After we redirect standard output, we won't be able to use an ioctl
> +* to get the terminal size. Let's grab it now, and then set $COLUMNS
> +* to communicate it to any sub-processes.
>  */
> -   (void) term_columns();
> +   {
> +   char buf[64];
> +   xsnprintf(buf, sizeof(buf), "%d", term_columns());
> +   setenv("COLUMNS", buf, 0);

I wonder if this affects bash being a subprocess. E.g. if COLUMNS is
exported will it still dynamically adjust COLUMNS? A quick test shows
that it adjusts $COLUMNS anyway, so even if we need to launch a shell
we should be good.

> +   }
>
> setenv("GIT_PAGER_IN_USE", "true", 1);
>
> --
> 2.17.0.984.g9b00a423a4
>
-- 
Duy

[PATCH] pager: set COLUMNS to term_columns()

[Jeff King]

On Fri, May 11, 2018 at 10:43:45AM +0200, Duy Nguyen wrote:

> > As an aside, I was confused while looking into this because I _thought_
> > I had COLUMNS set:
> >
> >   $ echo $COLUMNS
> >   119
> >
> > But it turns out that bash sets that by default (if you have the
> > checkwinsize option on) but does not export it. ;)
> 
> Yep. This confused me too and I was "f*ck it I don't want to deal with
> this tty crap again". I'll leave the term_columns() fix to you then,
> and limit this patch to the "while at there" part which should be
> fixed anyway.

Heh. OK, here it is. I wondered why we didn't notice this earlier and
elsewhere (like in "git -p help -a"). The answer is that git-tag is the
only program that uses the column filter. Everybody else does it
in-process.

-- >8 --
Subject: [PATCH] pager: set COLUMNS to term_columns()

After we invoke the pager, our stdout goes to a pipe, not the
terminal, meaning we can no longer use an ioctl to get the
terminal width. For that reason, ad6c3739a3 (pager: find out
the terminal width before spawning the pager, 2012-02-12)
started caching the terminal width.

But that cache is only an in-process variable. Any programs
we spawn will also not be able to run that ioctl, but won't
have access to our cache. They'll end up falling back to our
80-column default.

You can see the problem with:

  git tag --column=row

Since git-tag spawns a pager these days, its spawned
git-column helper will see neither the terminal on stdout
nor a useful COLUMNS value (assuming you do not export it
from your shell already). And you'll end up with 80-column
output in the pager, regardless of your terminal size.

We can fix this by setting COLUMNS right before spawning the
pager. That fixes this case, as well as any more complicated
ones (e.g., a paged program spawns another script which then
generates columnized output).

Signed-off-by: Jeff King <p...@peff.net>
---
 pager.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/pager.c b/pager.c
index 92b23e6cd1..226828f178 100644
--- a/pager.c
+++ b/pager.c
@@ -109,10 +109,15 @@ void setup_pager(void)
return;
 
/*
-    * force computing the width of the terminal before we redirect
-* the standard output to the pager.
+* After we redirect standard output, we won't be able to use an ioctl
+* to get the terminal size. Let's grab it now, and then set $COLUMNS
+* to communicate it to any sub-processes.
 */
-   (void) term_columns();
+   {
+   char buf[64];
+   xsnprintf(buf, sizeof(buf), "%d", term_columns());
+   setenv("COLUMNS", buf, 0);
+   }
 
setenv("GIT_PAGER_IN_USE", "true", 1);
 
-- 
2.17.0.984.g9b00a423a4

Re: [PATCH v2] git: add -P as a short option for --no-pager

[Junio C Hamano]

Eric Sunshine  writes:

>> available. Provide a short option, -P, to make the option easier
>> accessible.
>
> s/easier accessible/easier to access/
> --- or ---
> s/easier accessible/more easily accessible/
> --- or ---
> s/easier accessible/more accessible/
>
> The patch itself looks fine.

Thanks.  More easily accessible, it is.

Re: [PATCH v2] git: add -P as a short option for --no-pager

[Eric Sunshine]

On Thu, May 3, 2018 at 1:15 PM, Johannes Sixt <j...@kdbg.org> wrote:
> It is possible to configure 'less', the pager, to use an alternate
> screen to show the content, for example, by setting LESS=RS in the
> environment. When it is closed in this configuration, it switches
> back to the original screen, and all content is gone.
>
> It is not uncommon to request that the output remains visible in
> the terminal. For this, the option --no-pager can be used. But
> it is a bit cumbersome to type, even when command completion is
> available. Provide a short option, -P, to make the option easier
> accessible.

s/easier accessible/easier to access/
--- or ---
s/easier accessible/more easily accessible/
--- or ---
s/easier accessible/more accessible/

The patch itself looks fine.

> Signed-off-by: Johannes Sixt <j...@kdbg.org>

[PATCH v2] git: add -P as a short option for --no-pager

[Johannes Sixt]

It is possible to configure 'less', the pager, to use an alternate
screen to show the content, for example, by setting LESS=RS in the
environment. When it is closed in this configuration, it switches
back to the original screen, and all content is gone.

It is not uncommon to request that the output remains visible in
the terminal. For this, the option --no-pager can be used. But
it is a bit cumbersome to type, even when command completion is
available. Provide a short option, -P, to make the option easier
accessible.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 Given the positive feedback, I resurrect the patch.

 Changes since v1:
 - Use -P instead of -N
 - Commit message changed as proposed by Kaartic

 Documentation/git.txt | 3 ++-
 git.c | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4767860e72..c662f41c1d 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git' [--version] [--help] [-C ] [-c =]
 [--exec-path[=]] [--html-path] [--man-path] [--info-path]
-[-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
+[-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]
 [--git-dir=] [--work-tree=] [--namespace=]
 [--super-prefix=]
  []
@@ -103,6 +103,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string which 
`git config
configuration options (see the "Configuration Mechanism" section
below).
 
+-P::
 --no-pager::
Do not pipe Git output into a pager.
 
diff --git a/git.c b/git.c
index ceaa58ef40..71d013424e 100644
--- a/git.c
+++ b/git.c
@@ -7,7 +7,7 @@
 const char git_usage_string[] =
N_("git [--version] [--help] [-C ] [-c =]\n"
   "   [--exec-path[=]] [--html-path] [--man-path] 
[--info-path]\n"
-  "   [-p | --paginate | --no-pager] [--no-replace-objects] 
[--bare]\n"
+  "   [-p | --paginate | -P | --no-pager] 
[--no-replace-objects] [--bare]\n"
   "   [--git-dir=] [--work-tree=] 
[--namespace=]\n"
   "[]");
 
@@ -81,7 +81,7 @@ static int handle_options(const char ***argv, int *argc, int 
*envchanged)
exit(0);
} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
use_pager = 1;
-   } else if (!strcmp(cmd, "--no-pager")) {
+   } else if (!strcmp(cmd, "-P") || !strcmp(cmd, "--no-pager")) {
use_pager = 0;
if (envchanged)
*envchanged = 1;
-- 
2.17.0.69.g0c1d01d9b6

Re: js/no-pager-shorthand [

[Junio C Hamano]

Eric Sunshine <sunsh...@sunshineco.com> writes:

> Although paged output is generally nice, I frequently find myself
> typing --no-pager numerous times during the day, so I, for one,
> welcome having a short option (and would be sad to see this patch
> retracted). As with Hannes, I too find -P a reasonably intuitive and
> easy-to-remember short option.

Yeah, we often see in other people's tools that the -X option means
opposite of the -x option, and even though "git --paginate" may not
have short-and-sweet "git -p" short-hand (or does it?) it would be
natural to expect "-p" to mean that, and I do not find it a stretch
for "-P" to mean opposite of it by extension.

... goes and looks ...

Ah, "-p" does mean "--paginate", it seems.  So I am not opposed to
resurrecting the patch with s/N/P/, if that is what people want.  On
the other hand, I am perfectly OK if the patch stays retracted.

Re: js/no-pager-shorthand [was: What's cooking in git.git (Apr 2018, #04; Mon, 30)]

[Ævar Arnfjörð Bjarmason]

On Tue, May 01 2018, Johannes Schindelin wrote:

> I wonder whether `-!p` would be better/feasible?

In *nix shells `git -!p ...` unquoted will turn that !p into whatever
command you last ran that started with a "p", in my case `git -ping
8.8.8.8`. So there's a reason that's not the idiom in any *nix CLI
program (as opposed to +p which I think was mentioned in a side-thread)>

Re: js/no-pager-shorthand [was: What's cooking in git.git (Apr 2018, #04; Mon, 30)]

[Eric Sunshine]

On Tue, May 1, 2018 at 10:58 AM, Johannes Sixt <j...@kdbg.org> wrote:
> Am 01.05.2018 um 13:57 schrieb Johannes Schindelin:
>> On Mon, 30 Apr 2018, Johannes Sixt wrote:
>>> Am 30.04.2018 um 05:25 schrieb Junio C Hamano:
>>>> * js/no-pager-shorthand (2018-04-25) 1 commit
>>>>    - git: add -N as a short option for --no-pager
>>>
>>> I find -P is not that bad after all.
>>
>> To me, `-P` would suggest the positive action --pager rather than the
>> negative --no-pager.
> >
>> Your use case is quite the corner case, I hope you realize that, as it
>> seems that everybody else is fine with having -FRX as default options for
>> `less`... And with copy/pasting from the `less` output. So introducing a
>> sweet short option for --no-pager, for the benefit of maybe even only one
>> user, seems quite... unusual.
>
> Given the ambivalence (or inconclusiveness), I retract this patch without
> offering a replacement for the time being.

Although paged output is generally nice, I frequently find myself
typing --no-pager numerous times during the day, so I, for one,
welcome having a short option (and would be sad to see this patch
retracted). As with Hannes, I too find -P a reasonably intuitive and
easy-to-remember short option.

Re: js/no-pager-shorthand [was: What's cooking in git.git (Apr 2018, #04; Mon, 30)]

[Johannes Sixt]

Am 01.05.2018 um 13:57 schrieb Johannes Schindelin:

Hi Hannes,

On Mon, 30 Apr 2018, Johannes Sixt wrote:


Am 30.04.2018 um 05:25 schrieb Junio C Hamano:

* js/no-pager-shorthand (2018-04-25) 1 commit
   - git: add -N as a short option for --no-pager

   "git --no-pager cmd" did not have short-and-sweet single letter
   option. Now it does.

   Will merge to 'next'.


I consider your argument that -N is only an abbreviation for an unspecific
"no" a valid one. So, I would like to be sure that we are not painting us into
the wrong corner by squatting -N for --no-pager.

I find -P is not that bad after all.


To me, `-P` would suggest the positive action --pager rather than the
negative --no-pager.

I wonder whether `-!p` would be better/feasible?


We do not have this pattern, yet, nor do I know it from some other 
utility. I do not want to set a precedent.



Your use case is quite the corner case, I hope you realize that, as it
seems that everybody else is fine with having -FRX as default options for
`less`... And with copy/pasting from the `less` output. So introducing a
sweet short option for --no-pager, for the benefit of maybe even only one
user, seems quite... unusual.

Granted, you cannot simply introduce an alias for `git --no-pager`. But
maybe that is what we should do? Maybe we should start supporting aliases
without specifying commands, opening the door for things like `git -c
ui.color=false`, too.

Then you could add `alias.n=--no-pager` and call `git n show HEAD`, and
the -N and -P short options could still wait for a widely-popular option
to require a short name.


But then I can just as well have alias gitn='git --no-pager' in my 
.bashrc. Maybe I should do that.


Given the ambivalence (or inconclusiveness), I retract this patch 
without offering a replacement for the time being.


Thanks for a bit of sanity,
-- Hannes

Re: js/no-pager-shorthand [was: What's cooking in git.git (Apr 2018, #04; Mon, 30)]

[Johannes Schindelin]

Hi Hannes,

On Mon, 30 Apr 2018, Johannes Sixt wrote:

> Am 30.04.2018 um 05:25 schrieb Junio C Hamano:
> > * js/no-pager-shorthand (2018-04-25) 1 commit
> >   - git: add -N as a short option for --no-pager
> > 
> >   "git --no-pager cmd" did not have short-and-sweet single letter
> >   option. Now it does.
> > 
> >   Will merge to 'next'.
> 
> I consider your argument that -N is only an abbreviation for an unspecific
> "no" a valid one. So, I would like to be sure that we are not painting us into
> the wrong corner by squatting -N for --no-pager.
> 
> I find -P is not that bad after all.

To me, `-P` would suggest the positive action --pager rather than the
negative --no-pager.

I wonder whether `-!p` would be better/feasible?

Your use case is quite the corner case, I hope you realize that, as it
seems that everybody else is fine with having -FRX as default options for
`less`... And with copy/pasting from the `less` output. So introducing a
sweet short option for --no-pager, for the benefit of maybe even only one
user, seems quite... unusual.

Granted, you cannot simply introduce an alias for `git --no-pager`. But
maybe that is what we should do? Maybe we should start supporting aliases
without specifying commands, opening the door for things like `git -c
ui.color=false`, too.

Then you could add `alias.n=--no-pager` and call `git n show HEAD`, and
the -N and -P short options could still wait for a widely-popular option
to require a short name.

Ciao,
Dscho

Re: [PATCH] git: add -N as a short option for --no-pager

[Kaartic Sivaraam]

On Wednesday 25 April 2018 11:55 AM, Johannes Sixt wrote:
> 
> I considered -P, but thought that it would better be reserved for
> something related to "paths". We have --{html,man,info}-path, which
> would be served better with -[HMI]. That leaves --exec-path, which we
> would probably abbreviate as -x or -X. So, -P is perhaps not that bad
> after all.
> 

It might be a good choice but I don't think it's not the best choice
anyway because '-P' doesn't seem to communicate 'no pager' well.


> Perhaps --no-pager means also "unpaginated" (-u, -U), "linear" (-l, -L),
> "streamed" (-s, -S). Other ideas, anyone?
>

Considering the other alternatives you suggested (as I couldn't come up
with a better "single letter" option ;-) ), I think '-s' might be a nice
choice but would possibly need some explanation in the documentation to
the wondering users.

In the end, lacking a better one letter option, I think '-P' would be a
better choice after all. It would, possibly, be easily understood as a
negation of pagination (-p). So, I guess it would be better to go with
'-P' than '-N'.

-- 
Sivaraam

QUOTE:

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

  - Joel Spolsky

KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

As I'm not a native English speaker myself, there might be mistaeks in
my usage of English. I apologise for any mistakes that I make.

It would be "helpful" if you take the time to point out the mistakes.

It would be "super helpful" if you could provide suggestions about how
to correct those mistakes.

Thanks in advance!



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] git: add -N as a short option for --no-pager

[Kaartic Sivaraam]

On Thursday 26 April 2018 01:01 AM, Johannes Sixt wrote:
> 
> Right. But I have LESS=RS because I do *not* want it to remain on screen
> by default.
> 

In that case how about updating the commit message to be more clear
about it. How about,

It is possible to configure 'less', the pager, to use an
alternate screen to show the content. When it is closed in this
case, it switches back to the original screen, and all content
is gone.

-- 
Sivaraam

QUOTE:

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

  - Joel Spolsky

KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

As I'm not a native English speaker myself, there might be mistaeks in
my usage of English. I apologise for any mistakes that I make.

It would be "helpful" if you take the time to point out the mistakes.

It would be "super helpful" if you could provide suggestions about how
to correct those mistakes.

Thanks in advance!



signature.asc
Description: OpenPGP digital signature

js/no-pager-shorthand [was: What's cooking in git.git (Apr 2018, #04; Mon, 30)]

[Johannes Sixt]

Am 30.04.2018 um 05:25 schrieb Junio C Hamano:

* js/no-pager-shorthand (2018-04-25) 1 commit
  - git: add -N as a short option for --no-pager

  "git --no-pager cmd" did not have short-and-sweet single letter
  option. Now it does.

  Will merge to 'next'.


I consider your argument that -N is only an abbreviation for an 
unspecific "no" a valid one. So, I would like to be sure that we are not 
painting us into the wrong corner by squatting -N for --no-pager.


I find -P is not that bad after all.

-- Hannes

Re: [PATCH] git: add -N as a short option for --no-pager

[Johannes Sixt]

Am 25.04.2018 um 11:21 schrieb Phillip Wood:

On 24/04/18 17:59, Johannes Sixt wrote:


In modern setups, less, the pager, uses alternate screen to show
the content. When it is closed, it switches back to the original
screen, and all content is gone.


Are you setting LESS explicitly in the environment?

 From the git config man page:
When the LESS environment variable is unset, Git sets it to FRX (if LESS 
environment variable is set, Git does not change it at all).


 From the less man page the X option:
Disables  sending the termcap initialization and deinitialization 
strings to the terminal.  This is sometimes desirable if the 
deinitialization string does something unnecessary, like clearing the 
screen.


So with the default setup the output should remain on the screen.


Right. But I have LESS=RS because I do *not* want it to remain on screen 
by default.


-- Hannes

Re: [PATCH] git: add -N as a short option for --no-pager

[Johannes Sixt]

Am 25.04.2018 um 09:41 schrieb Johannes Schindelin:

Hi Hannes,

On Wed, 25 Apr 2018, Johannes Sixt wrote:


Am 25.04.2018 um 02:05 schrieb Junio C Hamano:

Johannes Sixt <j...@kdbg.org> writes:

It is not uncommon to request that the output remains visible in
the terminal.


I ran `git log` and then hit `q`, and the latest screen contents were
still visible in all of these scenarios:

- in a Linux VM via SSH client

- in Git Bash on Windows

- in Git CMD on Windows

Granted, this is only the latest screen, but that is usually good enough
here.

Is anything different happening in your setups?


I have LESS=RS in my environment, because I do want that the alternate 
screen is used by the pager. Nevertheless, occasionally I want git's 
output to remain visible.


-- Hannes

Re: [PATCH] git: add -N as a short option for --no-pager

[Phillip Wood]

On 24/04/18 17:59, Johannes Sixt wrote:


In modern setups, less, the pager, uses alternate screen to show
the content. When it is closed, it switches back to the original
screen, and all content is gone.


Are you setting LESS explicitly in the environment?

From the git config man page:
When the LESS environment variable is unset, Git sets it to FRX (if LESS 
environment variable is set, Git does not change it at all).


From the less man page the X option:
Disables  sending the termcap initialization and deinitialization 
strings to the terminal.  This is sometimes desirable if the 
deinitialization string does something unnecessary, like clearing the 
screen.


So with the default setup the output should remain on the screen.

Best Wishes

Phillip



It is not uncommon to request that the output remains visible in
the terminal. For this, the option --no-pager can be used. But
it is a bit cumbersome to type, even when command completion is
available. Provide a short option, -N, to make the option easier
accessible.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
  Documentation/git.txt | 3 ++-
  git.c | 4 ++--
  2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4767860e72..17b50b0dc6 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -11,7 +11,7 @@ SYNOPSIS
  [verse]
  'git' [--version] [--help] [-C ] [-c =]
  [--exec-path[=]] [--html-path] [--man-path] [--info-path]
-[-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
+[-p|--paginate|-N|--no-pager] [--no-replace-objects] [--bare]
  [--git-dir=] [--work-tree=] [--namespace=]
  [--super-prefix=]
   []
@@ -103,6 +103,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string which 
`git config
configuration options (see the "Configuration Mechanism" section
below).
  
+-N::

  --no-pager::
Do not pipe Git output into a pager.
  
diff --git a/git.c b/git.c

index ceaa58ef40..9e2d78f442 100644
--- a/git.c
+++ b/git.c
@@ -7,7 +7,7 @@
  const char git_usage_string[] =
N_("git [--version] [--help] [-C ] [-c =]\n"
   "   [--exec-path[=]] [--html-path] [--man-path] 
[--info-path]\n"
-  "   [-p | --paginate | --no-pager] [--no-replace-objects] 
[--bare]\n"
+  "   [-p | --paginate | -N | --no-pager] [--no-replace-objects] 
[--bare]\n"
   "   [--git-dir=] [--work-tree=] 
[--namespace=]\n"
   "[]");
  
@@ -81,7 +81,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)

exit(0);
} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
use_pager = 1;
-   } else if (!strcmp(cmd, "--no-pager")) {
+   } else if (!strcmp(cmd, "-N") || !strcmp(cmd, "--no-pager")) {
use_pager = 0;
if (envchanged)
*envchanged = 1;

Re: [PATCH] git: add -N as a short option for --no-pager

[Johannes Schindelin]

Hi Hannes,

On Wed, 25 Apr 2018, Johannes Sixt wrote:

> Am 25.04.2018 um 02:05 schrieb Junio C Hamano:
> > Johannes Sixt  writes:
> > > It is not uncommon to request that the output remains visible in
> > > the terminal.

I ran `git log` and then hit `q`, and the latest screen contents were
still visible in all of these scenarios:

- in a Linux VM via SSH client

- in Git Bash on Windows

- in Git CMD on Windows

Granted, this is only the latest screen, but that is usually good enough
here.

Is anything different happening in your setups?

Ciao,
Dscho

Re: [PATCH] git: add -N as a short option for --no-pager

[Johannes Sixt]

Am 25.04.2018 um 02:05 schrieb Junio C Hamano:

Johannes Sixt <j...@kdbg.org> writes:

It is not uncommon to request that the output remains visible in
the terminal. For this, the option --no-pager can be used. But
it is a bit cumbersome to type, even when command completion is
available. Provide a short option, -N, to make the option easier
accessible.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---


Heh, I used to append "|cat", which is four keystrokes that is a bit
shorter than " --no-pager", but that is only acceptable when you do
not care about colored output ;-)

I am not absolutely certain about the choice of a single letter. I
already checked we do not use "git -N cmd" for anything else right
now, so I am certain about the availability, but I am not sure if
capital 'N' is the best choice, when the other side is lowercase 'p'
(and more importantly, the other side 'p' has mneomonic value for
'pagination', but 'N' merely stands for 'no' and could be negating
anything, not related to pagination). But I agree that a short-hand
would be welcome.


I considered -P, but thought that it would better be reserved for 
something related to "paths". We have --{html,man,info}-path, which 
would be served better with -[HMI]. That leaves --exec-path, which we 
would probably abbreviate as -x or -X. So, -P is perhaps not that bad 
after all.


We could also choose +p, for which there is some precedent in other 
POSIX tools to mean negated -p, but not in git, I think. 
Implementationwise, it is not that trivial, either, because the section 
handling options is guarded by a check that the word begins with a dash.


Perhaps --no-pager means also "unpaginated" (-u, -U), "linear" (-l, -L), 
"streamed" (-s, -S). Other ideas, anyone?





diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4767860e72..17b50b0dc6 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -11,7 +11,7 @@ SYNOPSIS
  [verse]
  'git' [--version] [--help] [-C ] [-c =]
  [--exec-path[=]] [--html-path] [--man-path] [--info-path]
-[-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
+[-p|--paginate|-N|--no-pager] [--no-replace-objects] [--bare]
  [--git-dir=] [--work-tree=] [--namespace=]
  [--super-prefix=]
   []

Re: [PATCH] git: add -N as a short option for --no-pager

[Beat Bolli <bbo...@ewanet.ch>]

On Wed, Apr 25, 2018 at 09:05:56AM +0900, Junio C Hamano wrote:
> Johannes Sixt <j...@kdbg.org> writes:
> 
> > In modern setups, less, the pager, uses alternate screen to show
> > the content. When it is closed, it switches back to the original
> > screen, and all content is gone.
> >
> > It is not uncommon to request that the output remains visible in
> > the terminal. For this, the option --no-pager can be used. But
> > it is a bit cumbersome to type, even when command completion is
> > available. Provide a short option, -N, to make the option easier
> > accessible.
> >
> > Signed-off-by: Johannes Sixt <j...@kdbg.org>
> > ---
> 
> Heh, I used to append "|cat", which is four keystrokes that is a bit
> shorter than " --no-pager", but that is only acceptable when you do
> not care about colored output ;-)
> 
> I am not absolutely certain about the choice of a single letter. I
> already checked we do not use "git -N cmd" for anything else right
> now, so I am certain about the availability, but I am not sure if
> capital 'N' is the best choice, when the other side is lowercase 'p'
> (and more importantly, the other side 'p' has mneomonic value for
> 'pagination', but 'N' merely stands for 'no' and could be negating
> anything, not related to pagination). But I agree that a short-hand
> would be welcome.
> 

I'm quite fond of the notation "-p-", but that would set a precedent for
all other "--no-" options.

Maybe the option parser could be enhanced to allow for both?

Thanks,
Beat

> > diff --git a/Documentation/git.txt b/Documentation/git.txt
> > index 4767860e72..17b50b0dc6 100644
> > --- a/Documentation/git.txt
> > +++ b/Documentation/git.txt
> > @@ -11,7 +11,7 @@ SYNOPSIS
> >  [verse]
> >  'git' [--version] [--help] [-C ] [-c =]
> >  [--exec-path[=]] [--html-path] [--man-path] [--info-path]
> > -[-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
> > +[-p|--paginate|-N|--no-pager] [--no-replace-objects] [--bare]
> >  [--git-dir=] [--work-tree=] [--namespace=]
> >  [--super-prefix=]
> >   []
> > @@ -103,6 +103,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string which 
> > `git config
> > configuration options (see the "Configuration Mechanism" section
> > below).
> >  
> > +-N::
> >  --no-pager::
> > Do not pipe Git output into a pager.


signature.asc
Description: PGP signature

Re: [PATCH] git: add -N as a short option for --no-pager

[Junio C Hamano]

Johannes Sixt <j...@kdbg.org> writes:

> In modern setups, less, the pager, uses alternate screen to show
> the content. When it is closed, it switches back to the original
> screen, and all content is gone.
>
> It is not uncommon to request that the output remains visible in
> the terminal. For this, the option --no-pager can be used. But
> it is a bit cumbersome to type, even when command completion is
> available. Provide a short option, -N, to make the option easier
> accessible.
>
> Signed-off-by: Johannes Sixt <j...@kdbg.org>
> ---

Heh, I used to append "|cat", which is four keystrokes that is a bit
shorter than " --no-pager", but that is only acceptable when you do
not care about colored output ;-)

I am not absolutely certain about the choice of a single letter. I
already checked we do not use "git -N cmd" for anything else right
now, so I am certain about the availability, but I am not sure if
capital 'N' is the best choice, when the other side is lowercase 'p'
(and more importantly, the other side 'p' has mneomonic value for
'pagination', but 'N' merely stands for 'no' and could be negating
anything, not related to pagination). But I agree that a short-hand
would be welcome.

> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 4767860e72..17b50b0dc6 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -11,7 +11,7 @@ SYNOPSIS
>  [verse]
>  'git' [--version] [--help] [-C ] [-c =]
>  [--exec-path[=]] [--html-path] [--man-path] [--info-path]
> -[-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
> +[-p|--paginate|-N|--no-pager] [--no-replace-objects] [--bare]
>  [--git-dir=] [--work-tree=] [--namespace=]
>  [--super-prefix=]
>   []
> @@ -103,6 +103,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string which 
> `git config
>   configuration options (see the "Configuration Mechanism" section
>   below).
>  
> +-N::
>  --no-pager::
>   Do not pipe Git output into a pager.

[PATCH] git: add -N as a short option for --no-pager

[Johannes Sixt]

In modern setups, less, the pager, uses alternate screen to show
the content. When it is closed, it switches back to the original
screen, and all content is gone.

It is not uncommon to request that the output remains visible in
the terminal. For this, the option --no-pager can be used. But
it is a bit cumbersome to type, even when command completion is
available. Provide a short option, -N, to make the option easier
accessible.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 Documentation/git.txt | 3 ++-
 git.c | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4767860e72..17b50b0dc6 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git' [--version] [--help] [-C ] [-c =]
 [--exec-path[=]] [--html-path] [--man-path] [--info-path]
-[-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
+[-p|--paginate|-N|--no-pager] [--no-replace-objects] [--bare]
 [--git-dir=] [--work-tree=] [--namespace=]
 [--super-prefix=]
  []
@@ -103,6 +103,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string which 
`git config
configuration options (see the "Configuration Mechanism" section
below).
 
+-N::
 --no-pager::
Do not pipe Git output into a pager.
 
diff --git a/git.c b/git.c
index ceaa58ef40..9e2d78f442 100644
--- a/git.c
+++ b/git.c
@@ -7,7 +7,7 @@
 const char git_usage_string[] =
N_("git [--version] [--help] [-C ] [-c =]\n"
   "   [--exec-path[=]] [--html-path] [--man-path] 
[--info-path]\n"
-  "   [-p | --paginate | --no-pager] [--no-replace-objects] 
[--bare]\n"
+  "   [-p | --paginate | -N | --no-pager] 
[--no-replace-objects] [--bare]\n"
   "   [--git-dir=] [--work-tree=] 
[--namespace=]\n"
   "[]");
 
@@ -81,7 +81,7 @@ static int handle_options(const char ***argv, int *argc, int 
*envchanged)
exit(0);
} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
use_pager = 1;
-   } else if (!strcmp(cmd, "--no-pager")) {
+   } else if (!strcmp(cmd, "-N") || !strcmp(cmd, "--no-pager")) {
use_pager = 0;
if (envchanged)
*envchanged = 1;
-- 
2.17.0.69.g0c1d01d9b6

[PATCH 2/4] pager: refactor `pager_command_config()`

[Martin Ågren]

`pager_command_config()` checks for the config `pager.`. In the
next commit, we will want to also look for some strings on the form
`pager..foo`.

Refactor the code to verify upfront that the string starts with
"pager." and then check that the remainder is the empty string.
This makes it easy to look for other remainders in the next patch.

While at it, before assigning to `value`, free any old value we might
already have picked up.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 pager.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/pager.c b/pager.c
index 92b23e6cd..8968f26f1 100644
--- a/pager.c
+++ b/pager.c
@@ -191,14 +191,19 @@ struct pager_command_config_data {
 static int pager_command_config(const char *var, const char *value, void 
*vdata)
 {
struct pager_command_config_data *data = vdata;
-   const char *cmd;
+   const char *cmd, *remainder;
+
+   if (!skip_prefix(var, "pager.", ) ||
+   !skip_prefix(cmd, data->cmd, ))
+   return 0;
 
-   if (skip_prefix(var, "pager.", ) && !strcmp(cmd, data->cmd)) {
+   if (!*remainder) {
int b = git_parse_maybe_bool(value);
if (b >= 0)
data->want = b;
else {
data->want = 1;
+   free(data->value);
data->value = xstrdup(value);
}
}
-- 
2.15.0.415.gac1375d7e

[PATCH 3/4] pager: introduce `pager.*.command` and `.enable`

[Martin Ågren]

`pager.foo` conflates two concepts: how and whether `git foo` should
page. In particular, it can not be used to change *how* to page without
possibly also affecting *whether*.

Teach Git about two new config items, `pager.foo.command` and
`pager.foo.enable`.

Make this interact sanely with the existing `pager.foo`. For example,
make sure that `pager.foo=false` does not cause us to forget about a
command already configured through `pager.foo.command`, so that the
given pager command can be "re-activated" using
`pager.foo[.enable]=true`.

Where Documentation/ refers to `pager.tag`, write "the `pager.tag[.*]`
configuration options". In config.txt, `pager.blame` is mentioned more
as an example and it describes precisely the situation where one will
want to use the old mechanism, so leave that instance unchanged.

For symmetry with how `--paginate` disrespects any pager that might have
been configured with `pager.foo`, do the same for `pager.foo.command`.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 Documentation/config.txt  | 17 +++
 Documentation/git-tag.txt |  3 +-
 Documentation/git.txt |  2 +-
 t/t7006-pager.sh  | 73 +++
 pager.c   |  5 
 5 files changed, 98 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1ac0ae6ad..72558cc74 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2460,6 +2460,23 @@ pager.::
or `--no-pager` is specified on the command line, it takes
precedence over this option.  To disable pagination for all
commands, set `core.pager` or `GIT_PAGER` to `cat`.
++
+This is a less flexible alternative to `pager..command` and
+`pager..enable`. Using it with a boolean does the same as using
+`pager..enable`. Using it with a command does the same as using
+`pager..command` and `pager..enable=true`.
+
+pager..command::
+   Specifies the pager to use for the subcommand.
+   Whether the pager should be used is configured using
+   `pager..enable` or `pager.=`.
+
+pager..enable::
+   A boolean which turns on or off pagination of the output of a
+   particular Git subcommand when writing to a tty. If `--paginate`
+   or `--no-pager` is specified on the command line, it takes
+   precedence over this option.  To disable pagination for all
+   commands, set `core.pager` or `GIT_PAGER` to `cat`.
 
 pretty.::
Alias for a --pretty= format string, as specified in
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 956fc019f..9f9f33409 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -210,7 +210,8 @@ it in the repository configuration as follows:
 signingKey = 
 -
 
-`pager.tag` is only respected when listing tags, i.e., when `-l` is
+The `pager.tag[.*]` configuration options are only
+respected when listing tags, i.e., when `-l` is
 used or implied. The default is to use a pager.
 See linkgit:git-config[1].
 
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7a1d629ca..0a2eff7a6 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -99,7 +99,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string which `git 
config
 -p::
 --paginate::
Pipe all output into 'less' (or if set, $PAGER) if standard
-   output is a terminal.  This overrides the `pager.`
+   output is a terminal.  This overrides the `pager.[.*]`
configuration options (see the "Configuration Mechanism" section
below).
 
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index e890b2f64..6966627dd 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -588,4 +588,77 @@ test_expect_success 'command with underscores does not 
complain' '
test_cmp expect actual
 '
 
+test_expect_success 'setup' '
+   sane_unset PAGER GIT_PAGER GIT_PAGER_IN_USE &&
+   test_unconfig core.pager &&
+
+   git rev-list HEAD >rev-list &&
+   sed "s/^/foo:/" rev-list >expect &&
+
+   PAGER="cat >paginated.out" &&
+   export PAGER &&
+
+   test_unconfig pager.log &&
+   test_unconfig pager.rev-list
+'
+
+test_expect_success TTY 'configuration with .enable works' '
+   rm -f paginated.out &&
+   test_terminal git -c pager.log.enable=false log &&
+   ! test -e paginated.out
+'
+
+test_expect_success TTY '--paginate overrides .enable+.command' '
+   rm -f paginated.out &&
+   test_terminal git -c pager.log.command=bad -c pager.log.enable=false \
+ --paginate log &&
+   test -e paginated.out
+'
+
+test_expect_success TTY '--no-pager overrides .enable' '
+   rm -f paginated.out &&
+   test_terminal git -c pager.rev-list.enable --no-pager rev-l

[PATCH 4/4] pager: make `pager.foo.command` imply `.enable=true`

[Martin Ågren]

If `pager.foo.command` gets configured and there is no configuration
(yet) saying whether we should page, act as if `pager.foo.enable=true`.

This means that a lone `pager.foo` can always be written as a lone
`pager.foo.command` or `pager.foo.enable`.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 Documentation/config.txt | 2 ++
 t/t7006-pager.sh | 7 +++
 pager.c  | 2 ++
 3 files changed, 11 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 72558cc74..91cc8b110 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2470,6 +2470,8 @@ pager..command::
Specifies the pager to use for the subcommand.
Whether the pager should be used is configured using
    `pager..enable` or `pager.=`.
+   Implies `pager..enable=true` unless `pager..enable`
+   or `pager.` have already been given.
 
 pager..enable::
A boolean which turns on or off pagination of the output of a
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 6966627dd..c5246a57d 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -625,6 +625,13 @@ test_expect_success TTY '.enable discards non-boolean' '
test_must_fail git -c pager.log.enable=bad log
 '
 
+test_expect_success TTY 'configuration with .command turns on paging' '
+   >actual &&
+   test_terminal git -c pager.rev-list.command="sed s/^/foo:/ >actual" \
+ rev-list HEAD &&
+   test_cmp expect actual
+'
+
 test_expect_success TTY 'configuration remembers .command as .enable flips' '
>actual &&
test_terminal git -c pager.rev-list.command="sed s/^/foo:/ >actual" \
diff --git a/pager.c b/pager.c
index c8a6a01d8..0e037abf4 100644
--- a/pager.c
+++ b/pager.c
@@ -209,6 +209,8 @@ static int pager_command_config(const char *var, const char 
*value, void *vdata)
} else if (!strcmp(remainder, ".command")) {
free(data->value);
data->value = xstrdup(value);
+   if (data->want == -1)
+   data->want = 1;
} else if (!strcmp(remainder, ".enable")) {
data->want = git_config_bool(var, value);
}
-- 
2.15.0.415.gac1375d7e

Re: [PATCH v2] column: show auto columns when pager is active

[Kevin Daudt]

On Mon, Oct 23, 2017 at 02:52:46PM -0700, Jonathan Nieder wrote:
> Hi,
> 
> Kevin Daudt wrote:
> 
> > --- a/column.c
> > +++ b/column.c
> > @@ -5,6 +5,7 @@
> >  #include "parse-options.h"
> >  #include "run-command.h"
> >  #include "utf8.h"
> > +#include "pager.c"
> 
> Should this be pager.h?
> 
> Thanks,
> Jonathan

Thanks for catching this.

Re: [PATCH v2] column: show auto columns when pager is active

[Jonathan Nieder]

Junio C Hamano wrote:

> Subject: column: do not include pager.c
>
> Everything this file needs from the pager API (e.g. term_columns(),
> pager_in_use()) is already declared in the header file it includes.
>
> Noticed-by: Jonathan Nieder <jrnie...@gmail.com>
> Signed-off-by: Junio C Hamano <gits...@pobox.com>
> ---
>  column.c | 1 -
>  1 file changed, 1 deletion(-)

Reviewed-by: Jonathan Nieder <jrnie...@gmail.com>

Thanks.

> --- a/column.c
> +++ b/column.c
> @@ -5,7 +5,6 @@
>  #include "parse-options.h"
>  #include "run-command.h"
>  #include "utf8.h"
> -#include "pager.c"
>  
>  #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \
>   (x) * (d)->rows + (y) : \

Re: [PATCH v2] column: show auto columns when pager is active

[Junio C Hamano]

Jonathan Nieder <jrnie...@gmail.com> writes:

> Hi,
>
> Kevin Daudt wrote:
>
>> --- a/column.c
>> +++ b/column.c
>> @@ -5,6 +5,7 @@
>>  #include "parse-options.h"
>>  #include "run-command.h"
>>  #include "utf8.h"
>> +#include "pager.c"
>
> Should this be pager.h?

Ouch.  And I was not paying enough attention.

Thanks, I'll queue this on top.

-- >8 --
Subject: column: do not include pager.c

Everything this file needs from the pager API (e.g. term_columns(),
pager_in_use()) is already declared in the header file it includes.

Noticed-by: Jonathan Nieder <jrnie...@gmail.com>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 column.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/column.c b/column.c
index ded50337f7..49ab85b769 100644
--- a/column.c
+++ b/column.c
@@ -5,7 +5,6 @@
 #include "parse-options.h"
 #include "run-command.h"
 #include "utf8.h"
-#include "pager.c"
 
 #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \
(x) * (d)->rows + (y) : \

Re: [PATCH v2] column: show auto columns when pager is active

[Jonathan Nieder]

Hi,

Kevin Daudt wrote:

> --- a/column.c
> +++ b/column.c
> @@ -5,6 +5,7 @@
>  #include "parse-options.h"
>  #include "run-command.h"
>  #include "utf8.h"
> +#include "pager.c"

Should this be pager.h?

Thanks,
Jonathan

Re: [PATCH v2] column: show auto columns when pager is active

[Junio C Hamano]

Kevin Daudt <m...@ikke.info> writes:

> When columns are set to automatic for git tag and the output is
> paginated by git, the output is a single column instead of multiple
> columns.
>
> Standard behaviour in git is to honor auto values when the pager is
> active, which happens for example with commands like git log showing
> colors when being paged.
>
> Since ff1e72483 (tag: change default of `pager.tag` to "on",
> 2017-08-02), the pager has been enabled by default, exposing this
> problem to more people.
>
> finalize_colopts in column.c only checks whether the output is a TTY to
> determine if columns should be enabled with columns set to auto. Also
> check if the pager is active.
>
> Adding a test for git column is possible but requires some care to work
> around a race on stdin. See commit 18d8c2693 (test_terminal: redirect
> child process' stdin to a pty, 2015-08-04). Test git tag instead, since
> that does not involve stdin, and since that was the original motivation
> for this patch.

Nicely done.

> +test_expect_success TTY 'git tag with auto-columns ' '
> + test_commit one &&
> + test_commit two &&
> + test_commit three &&
> + test_commit four &&
> + test_commit five &&
> + cat >expected <<\EOF &&
> +initial  one  two  threefour five
> +EOF
> + test_terminal env PAGER="cat >actual.tag" COLUMNS=80 \
> + git -c column.ui=auto tag --sort=authordate &&
> + test_cmp expected actual.tag
> +'

I'd use <<-\EOF so that here document can be intended like other
tests, and also use expect vs actual that are used in the other
tests in the same script, instead of suddenly becoming creative
in only this single test.  I can do these clean-ups locally when
queuing so no need to resend only to collect these.

Thanks, will queue.

[PATCH v2] column: show auto columns when pager is active

[Kevin Daudt]

When columns are set to automatic for git tag and the output is
paginated by git, the output is a single column instead of multiple
columns.

Standard behaviour in git is to honor auto values when the pager is
active, which happens for example with commands like git log showing
colors when being paged.

Since ff1e72483 (tag: change default of `pager.tag` to "on",
2017-08-02), the pager has been enabled by default, exposing this
problem to more people.

finalize_colopts in column.c only checks whether the output is a TTY to
determine if columns should be enabled with columns set to auto. Also
check if the pager is active.

Adding a test for git column is possible but requires some care to work
around a race on stdin. See commit 18d8c2693 (test_terminal: redirect
child process' stdin to a pty, 2015-08-04). Test git tag instead, since
that does not involve stdin, and since that was the original motivation
for this patch.

Helped-by: Rafael Ascensão <rafa.al...@gmail.com>
Signed-off-by: Kevin Daudt <m...@ikke.info>
---
Changes since v1:

- Remove redundant -p flag in tests
- Explain why git tag is being tested instead of the more obvious git
  column

 column.c |  3 ++-
 t/t7006-pager.sh | 14 ++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/column.c b/column.c
index ff7bdab1a..ded50337f 100644
--- a/column.c
+++ b/column.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "run-command.h"
 #include "utf8.h"
+#include "pager.c"
 
 #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \
(x) * (d)->rows + (y) : \
@@ -224,7 +225,7 @@ int finalize_colopts(unsigned int *colopts, int 
stdout_is_tty)
if (stdout_is_tty < 0)
stdout_is_tty = isatty(1);
*colopts &= ~COL_ENABLE_MASK;
-   if (stdout_is_tty)
+   if (stdout_is_tty || pager_in_use())
*colopts |= COL_ENABLED;
}
return 0;
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index f0f1abd1c..e985b6873 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not 
complain' '
test_cmp expect actual
 '
 
+test_expect_success TTY 'git tag with auto-columns ' '
+   test_commit one &&
+   test_commit two &&
+   test_commit three &&
+   test_commit four &&
+   test_commit five &&
+   cat >expected <<\EOF &&
+initial  one  two  threefour five
+EOF
+   test_terminal env PAGER="cat >actual.tag" COLUMNS=80 \
+   git -c column.ui=auto tag --sort=authordate &&
+   test_cmp expected actual.tag
+'
+
 test_done
-- 
2.14.2

Re: [RFC] column: show auto columns when pager is active

[Jeff King]

On Wed, Oct 11, 2017 at 06:54:04AM +0200, Kevin Daudt wrote:

> > > Yeah, I didn't think of that with respect to the pager. This is a
> > > regression in v2.14.2, I think.
> > 
> > Yes.
> 
> Though 2.14.2 enabled the pager by default, even before that when
> someone would've enabled the pager theirselves (by setting pager.tag for
> example), it would also shown just a single column.
> 
> I could reproduce it as far as 2.8.3 (I could not test further due to
> libssl incompattibility).

Right, I think it has always been broken. It's just that it became a lot
more widespread with the increased use of the pager.

I specifically wanted to make sure this wasn't a regression in the v2.15
release candidates (since that would mean we'd want to deal with it
before shipping v2.15). It still _would_ be nice to deal with it soon,
but since it's already been in the wild for a bit, it can wait to hit
"maint" in the post-v2.15 cycle.

-Peff

Re: [PATCH] column: show auto columns when pager is active

[Martin Ågren]

On 11 October 2017 at 20:36, Kevin Daudt <m...@ikke.info> wrote:
> On Wed, Oct 11, 2017 at 08:12:35PM +0200, Martin Ågren wrote:
>> On 11 October 2017 at 19:23, Kevin Daudt <m...@ikke.info> wrote:
>> I wonder if it's useful to set COLUMNS a bit lower so that this has to
>> split across more than one line (but not six), i.e., to do something
>> non-trivial. I suppose that might lower the chances of some weird
>> breakage slipping through.
>
> Yeah, I was doubting about that, but wouldn't this amount to testing
> whether git column is working properly, instead of just testing whether
> it's being done at all?

Right, I think you'd need a pretty crazy bug in order to slip through
all tests here.

>> These were just the thoughts that occurred to me, not sure if any of
>> them is particularly significant. Thanks for cleaning up after me.
>>
>
> np. Just as I posted earlier, I think you did not actually cause the bug
> (because this has never worked), it just made it visible to more users.

Well, the general bug/behavior was always there, but I regressed a
particular use-case. In 2.14, you could do `git tag` with column.ui=auto
and it would do the columns-thing. But with ff1e72483, the behavior
changed. To add insult to injury, it might be non-obvious that the pager
is running, since with just a few tags, the pager simply exits silently.
So debugging this could probably be quite frustrating.

Martin

Re: [PATCH] column: show auto columns when pager is active

[Kevin Daudt]

On Wed, Oct 11, 2017 at 08:12:35PM +0200, Martin Ågren wrote:
> On 11 October 2017 at 19:23, Kevin Daudt <m...@ikke.info> wrote:
> > finalize_colopts in column.c only checks whether the output is a TTY to
> > determine if columns should be enabled with columns set to auto. Also check
> > if the pager is active.
> 
> Maybe you could say something about the difficulties of writing a test
> for `git column` proper. Something like this perhaps:
> 
>   Adding a test for git column is possible but requires some care to
>   work around a race on stdin. See commit 18d8c2693 (test_terminal:
>   redirect child process' stdin to a pty, 2015-08-04). Test git tag
>   instead, since that does not involve stdin, and since that was the
>   original motivation for this patch.

Right, makes sense.
> 
> > Helped-by: Rafael Ascensão <rafa.al...@gmail.com>
> > Signed-off-by: Kevin Daudt <m...@ikke.info>
> > ---
> >  column.c |  3 ++-
> >  t/t7006-pager.sh | 14 ++
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> Does the documentation on `column.ui` need to be updated? It talks about
> "if the output is to the terminal". That's similar to the documentation
> on the various `color.*`, so we should be fine, and arguably it's even
> better not to say anything since that makes it consistent.
> 
> > diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> > index f0f1abd1c..44c2ca5d3 100755
> > --- a/t/t7006-pager.sh
> > +++ b/t/t7006-pager.sh
> > @@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not 
> > complain' '
> > test_cmp expect actual
> >  '
> >
> > +test_expect_success TTY 'git tag with auto-columns ' '
> > +   test_commit one &&
> > +   test_commit two &&
> > +   test_commit three &&
> > +   test_commit four &&
> > +   test_commit five &&
> > +   cat >expected <<\EOF &&
> > +initial  one  two  threefour five
> > +EOF
> > +   test_terminal env PAGER="cat >actual.tag" COLUMNS=80 \
> > +   git -p -c column.ui=auto tag --sort=authordate &&
> > +   test_cmp expected actual.tag
> > +'
> > +
> >  test_done
> 
> Since `git tag` pages when it's listing, you don't need the `-p`. But
> it's not like it hurts to have it. Yeah, I know, you needed it with `git
> column`. :-)

Right, it was a bit of a left-over since I assumed the PAGER='cat 
>paginated.out'
from the beginning of the test was in place and I wasn't getting any
output, but it turned out PAGER wasn't set.

> I wonder if it's useful to set COLUMNS a bit lower so that this has to
> split across more than one line (but not six), i.e., to do something
> non-trivial. I suppose that might lower the chances of some weird
> breakage slipping through.

Yeah, I was doubting about that, but wouldn't this amount to testing
whether git column is working properly, instead of just testing whether
it's being done at all?

> This test uses "actual.tag" while most (all?) others in this file use
> "actual". Maybe you worry about checking the "wrong" file, e.g., in case
> the pager doesn't kick in. You could do `rm -f actual` before the
> `test_terminal`-invocation to protect against that.

Yeah, I actually ran into that, but rm-ing it is better, I agree.

> These were just the thoughts that occurred to me, not sure if any of
> them is particularly significant. Thanks for cleaning up after me.
> 

np. Just as I posted earlier, I think you did not actually cause the bug
(because this has never worked), it just made it visible to more users.

Kevin

Re: [PATCH] column: show auto columns when pager is active

[Martin Ågren]

On 11 October 2017 at 19:23, Kevin Daudt <m...@ikke.info> wrote:
> finalize_colopts in column.c only checks whether the output is a TTY to
> determine if columns should be enabled with columns set to auto. Also check
> if the pager is active.

Maybe you could say something about the difficulties of writing a test
for `git column` proper. Something like this perhaps:

  Adding a test for git column is possible but requires some care to
  work around a race on stdin. See commit 18d8c2693 (test_terminal:
  redirect child process' stdin to a pty, 2015-08-04). Test git tag
  instead, since that does not involve stdin, and since that was the
  original motivation for this patch.

> Helped-by: Rafael Ascensão <rafa.al...@gmail.com>
> Signed-off-by: Kevin Daudt <m...@ikke.info>
> ---
>  column.c |  3 ++-
>  t/t7006-pager.sh | 14 ++
>  2 files changed, 16 insertions(+), 1 deletion(-)

Does the documentation on `column.ui` need to be updated? It talks about
"if the output is to the terminal". That's similar to the documentation
on the various `color.*`, so we should be fine, and arguably it's even
better not to say anything since that makes it consistent.

> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index f0f1abd1c..44c2ca5d3 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not 
> complain' '
> test_cmp expect actual
>  '
>
> +test_expect_success TTY 'git tag with auto-columns ' '
> +   test_commit one &&
> +   test_commit two &&
> +   test_commit three &&
> +   test_commit four &&
> +   test_commit five &&
> +   cat >expected <<\EOF &&
> +initial  one  two  threefour five
> +EOF
> +   test_terminal env PAGER="cat >actual.tag" COLUMNS=80 \
> +   git -p -c column.ui=auto tag --sort=authordate &&
> +   test_cmp expected actual.tag
> +'
> +
>  test_done

Since `git tag` pages when it's listing, you don't need the `-p`. But
it's not like it hurts to have it. Yeah, I know, you needed it with `git
column`. :-)

I wonder if it's useful to set COLUMNS a bit lower so that this has to
split across more than one line (but not six), i.e., to do something
non-trivial. I suppose that might lower the chances of some weird
breakage slipping through.

This test uses "actual.tag" while most (all?) others in this file use
"actual". Maybe you worry about checking the "wrong" file, e.g., in case
the pager doesn't kick in. You could do `rm -f actual` before the
`test_terminal`-invocation to protect against that.

These were just the thoughts that occurred to me, not sure if any of
them is particularly significant. Thanks for cleaning up after me.

Martin

[PATCH] column: show auto columns when pager is active

[Kevin Daudt]

When columns are set to automatic for git tag and the output is
paginated by git, the output is a single column instead of multiple
columns.

Standard behaviour in git is to honor auto values when the pager is
active, which happens for example with commands like git log showing
colors when being paged.

Since ff1e72483 (tag: change default of `pager.tag` to "on",
2017-08-02), the pager has been enabled by default, exposing this
problem to more people.

finalize_colopts in column.c only checks whether the output is a TTY to
determine if columns should be enabled with columns set to auto. Also check
if the pager is active.

Helped-by: Rafael Ascensão <rafa.al...@gmail.com>
Signed-off-by: Kevin Daudt <m...@ikke.info>
---
 column.c |  3 ++-
 t/t7006-pager.sh | 14 ++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/column.c b/column.c
index ff7bdab1a..ded50337f 100644
--- a/column.c
+++ b/column.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "run-command.h"
 #include "utf8.h"
+#include "pager.c"
 
 #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \
(x) * (d)->rows + (y) : \
@@ -224,7 +225,7 @@ int finalize_colopts(unsigned int *colopts, int 
stdout_is_tty)
if (stdout_is_tty < 0)
stdout_is_tty = isatty(1);
*colopts &= ~COL_ENABLE_MASK;
-   if (stdout_is_tty)
+   if (stdout_is_tty || pager_in_use())
*colopts |= COL_ENABLED;
}
return 0;
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index f0f1abd1c..44c2ca5d3 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not 
complain' '
test_cmp expect actual
 '
 
+test_expect_success TTY 'git tag with auto-columns ' '
+   test_commit one &&
+   test_commit two &&
+   test_commit three &&
+   test_commit four &&
+   test_commit five &&
+   cat >expected <<\EOF &&
+initial  one  two  threefour five
+EOF
+   test_terminal env PAGER="cat >actual.tag" COLUMNS=80 \
+   git -p -c column.ui=auto tag --sort=authordate &&
+   test_cmp expected actual.tag
+'
+
 test_done
-- 
2.14.2

Re: [RFC] column: show auto columns when pager is active

[Kevin Daudt]

On Tue, Oct 10, 2017 at 07:02:00PM +0200, Martin Ågren wrote:
> On 10 October 2017 at 16:01, Jeff King <p...@peff.net> wrote:
> > On Tue, Oct 10, 2017 at 12:30:49PM +0200, Martin Ågren wrote:
> >> On 9 October 2017 at 23:45, Kevin Daudt <m...@ikke.info> wrote:
> >> > Since ff1e72483 (tag: change default of `pager.tag` to "on",
> >> > 2017-08-02), the pager has been enabled by default, exposing this
> >> > problem to more people.
> >>
> >> Oh. :( I didn't know about "column" to be honest.
> >
> > Yeah, I didn't think of that with respect to the pager. This is a
> > regression in v2.14.2, I think.
> 
> Yes.

Though 2.14.2 enabled the pager by default, even before that when
someone would've enabled the pager theirselves (by setting pager.tag for
example), it would also shown just a single column.

I could reproduce it as far as 2.8.3 (I could not test further due to
libssl incompattibility).

Re: [RFC] column: show auto columns when pager is active

[Kevin Daudt]

On Tue, Oct 10, 2017 at 07:04:42PM +0200, Martin Ågren wrote:
> On 10 October 2017 at 16:29, Jeff King <p...@peff.net> wrote:
> > On Tue, Oct 10, 2017 at 10:10:19AM -0400, Jeff King wrote:
> >
> > it will randomly succeed or fail, depending on whether sed manages to
> > read the input before the stdin terminal is closed.
> >
> > I'm not sure of an easy way to fix test-terminal, but we could work
> > around it like this (which also uses "-p" to actually invoke the pager,
> > and uses a pager that makes it clear when it's being run):
> >
> > diff --git a/t/t9002-column.sh b/t/t9002-column.sh
> > index 9441145bf0..d322c3b745 100755
> > --- a/t/t9002-column.sh
> > +++ b/t/t9002-column.sh
> > @@ -180,14 +180,14 @@ EOF
> >
> >  test_expect_success TTY '20 columns, mode auto, pager' '
> > cat >expected <<\EOF &&
> > -oneseven
> > -twoeight
> > -three  nine
> > -four   ten
> > -five   eleven
> > -six
> > +paged:oneseven
> > +paged:twoeight
> > +paged:three  nine
> > +paged:four   ten
> > +paged:five   eleven
> > +paged:six
> >  EOF
> > -   test_terminal env PAGER="cat|cat" git column --mode=auto  > >actual &&
> > +   test_terminal env PAGER="sed s/^/paged:/" sh -c "git -p column 
> > --mode=auto <lista" >actual &&
> > test_cmp expected actual
> >  '
> >  test_done
> 
> Makes sense. FWIW, I don't see the flakyness with this.
> 
> > All that said, I think I'd just as soon test a real command like "git
> > tag", which doesn't care about reading from stdin.
> 
> For reference for Kevin, in case you consider testing, e.g., git tag,
> the main reason I referred to the test I posted as "hacky", was that I
> just inserted it at a more-or-less random place in t7006. So it had to
> play with `git reset` to avoid upsetting the later tests. It could
> obviously go to the end instead, but I was too lazy to move it and
> define a pager.

Thanks Jeff and Martin, I will use your tips to build a test based on
git tag instead.

Re: [RFC] column: show auto columns when pager is active

[Martin Ågren]

On 10 October 2017 at 16:29, Jeff King <p...@peff.net> wrote:
> On Tue, Oct 10, 2017 at 10:10:19AM -0400, Jeff King wrote:
>
> it will randomly succeed or fail, depending on whether sed manages to
> read the input before the stdin terminal is closed.
>
> I'm not sure of an easy way to fix test-terminal, but we could work
> around it like this (which also uses "-p" to actually invoke the pager,
> and uses a pager that makes it clear when it's being run):
>
> diff --git a/t/t9002-column.sh b/t/t9002-column.sh
> index 9441145bf0..d322c3b745 100755
> --- a/t/t9002-column.sh
> +++ b/t/t9002-column.sh
> @@ -180,14 +180,14 @@ EOF
>
>  test_expect_success TTY '20 columns, mode auto, pager' '
> cat >expected <<\EOF &&
> -oneseven
> -twoeight
> -three  nine
> -four   ten
> -five   eleven
> -six
> +paged:oneseven
> +paged:twoeight
> +paged:three  nine
> +paged:four   ten
> +paged:five   eleven
> +paged:six
>  EOF
> -   test_terminal env PAGER="cat|cat" git column --mode=auto  >actual &&
> +   test_terminal env PAGER="sed s/^/paged:/" sh -c "git -p column 
> --mode=auto <lista" >actual &&
> test_cmp expected actual
>  '
>  test_done

Makes sense. FWIW, I don't see the flakyness with this.

> All that said, I think I'd just as soon test a real command like "git
> tag", which doesn't care about reading from stdin.

For reference for Kevin, in case you consider testing, e.g., git tag,
the main reason I referred to the test I posted as "hacky", was that I
just inserted it at a more-or-less random place in t7006. So it had to
play with `git reset` to avoid upsetting the later tests. It could
obviously go to the end instead, but I was too lazy to move it and
define a pager.

Re: [RFC] column: show auto columns when pager is active

[Martin Ågren]

On 10 October 2017 at 16:01, Jeff King <p...@peff.net> wrote:
> On Tue, Oct 10, 2017 at 12:30:49PM +0200, Martin Ågren wrote:
>> On 9 October 2017 at 23:45, Kevin Daudt <m...@ikke.info> wrote:
>> > Since ff1e72483 (tag: change default of `pager.tag` to "on",
>> > 2017-08-02), the pager has been enabled by default, exposing this
>> > problem to more people.
>>
>> Oh. :( I didn't know about "column" to be honest.
>
> Yeah, I didn't think of that with respect to the pager. This is a
> regression in v2.14.2, I think.

Yes.

>> In any case, it might make sense to test an
>> actual use-case also. Of course, the code should be largely the same,
>> but in builtin/tag.c, it's quite important that `setup_auto_pager()`
>> and `finalize_colopts()` are called in the right order.
>
> I think it might work out either way. If we have started the pager when
> we finalize_colopts(), then the pager_in_use() bit will kick in. If we
> haven't, then either:
>
>   1. stdout is a tty, and we'll kick in the auto behavior for columns,
>  and then later for the pager.
>
>   2. stdout isn't a tty, in which case we also won't kick in the pager.

Right you are.

Re: [RFC] column: show auto columns when pager is active

[Jeff King]

On Tue, Oct 10, 2017 at 10:10:19AM -0400, Jeff King wrote:

> That said, I'm still puzzled why it would return zero output. Strace
> shows that the read from stdin is getting no input. I suspect this may
> have to do with how we redirect stdin in test-terminal.perl.
> 
> See 18d8c26930 (test_terminal: redirect child process' stdin to a pty,
> 2015-08-04), which claims there's some raciness with closing the
> descriptor.

Ah, yeah, I think that is it. Try:

  echo input | test_terminal sed s/^/foo:/

it will randomly succeed or fail, depending on whether sed manages to
read the input before the stdin terminal is closed.

I'm not sure of an easy way to fix test-terminal, but we could work
around it like this (which also uses "-p" to actually invoke the pager,
and uses a pager that makes it clear when it's being run):

diff --git a/t/t9002-column.sh b/t/t9002-column.sh
index 9441145bf0..d322c3b745 100755
--- a/t/t9002-column.sh
+++ b/t/t9002-column.sh
@@ -180,14 +180,14 @@ EOF
 
 test_expect_success TTY '20 columns, mode auto, pager' '
cat >expected <<\EOF &&
-oneseven
-twoeight
-three  nine
-four   ten
-five   eleven
-six
+paged:oneseven
+paged:twoeight
+paged:three  nine
+paged:four   ten
+paged:five   eleven
+paged:six
 EOF
-   test_terminal env PAGER="cat|cat" git column --mode=auto actual 
&&
+   test_terminal env PAGER="sed s/^/paged:/" sh -c "git -p column 
--mode=auto <lista" >actual &&
test_cmp expected actual
 '
 test_done

All that said, I think I'd just as soon test a real command like "git
tag", which doesn't care about reading from stdin.

-Peff

Re: [RFC] column: show auto columns when pager is active

[Jeff King]

On Mon, Oct 09, 2017 at 11:45:43PM +0200, Kevin Daudt wrote:

> +test_expect_success TTY '20 columns, mode auto, pager' '
> + cat >expected <<\EOF &&
> +oneseven
> +twoeight
> +three  nine
> +four   ten
> +five   eleven
> +six
> +EOF
> + test_terminal env PAGER="cat|cat" git column --mode=auto actual 
> &&
> + test_cmp expected actual
> +'

I don't think "git column" will run the pager by default, will it?
You'd need "git -p" here.

That said, I'm still puzzled why it would return zero output. Strace
shows that the read from stdin is getting no input. I suspect this may
have to do with how we redirect stdin in test-terminal.perl.

See 18d8c26930 (test_terminal: redirect child process' stdin to a pty,
2015-08-04), which claims there's some raciness with closing the
descriptor.

-Peff

Re: [RFC] column: show auto columns when pager is active

[Jeff King]

On Tue, Oct 10, 2017 at 12:30:49PM +0200, Martin Ågren wrote:

> On 9 October 2017 at 23:45, Kevin Daudt <m...@ikke.info> wrote:
> > When columns are set to automatic for git tag and the output is
> > paginated by git, the output is a single column instead of multiple
> > columns.
> >
> > Standard behaviour in git is to honor auto values when the pager is
> > active, which happens for example with commands like git log showing
> > colors when being paged.
> >
> > Since ff1e72483 (tag: change default of `pager.tag` to "on",
> > 2017-08-02), the pager has been enabled by default, exposing this
> > problem to more people.
> 
> Oh. :( I didn't know about "column" to be honest.

Yeah, I didn't think of that with respect to the pager. This is a
regression in v2.14.2, I think.

I agree that anything that is "auto" on stdout probably ought to kick in
when the pager is in effect (since that only kicks in when stdout _was_
a tty before we stuck a pager in front of it).

> I had slightly more success with PAGER="cat >actual", but the test is
> flaky for some reason.

The test in t9002 should be immune to this, but the one you suggest in
t7006 would need to set COLUMNS to get consistent output, I think.

> In any case, it might make sense to test an
> actual use-case also. Of course, the code should be largely the same,
> but in builtin/tag.c, it's quite important that `setup_auto_pager()`
> and `finalize_colopts()` are called in the right order.

I think it might work out either way. If we have started the pager when
we finalize_colopts(), then the pager_in_use() bit will kick in. If we
haven't, then either:

  1. stdout is a tty, and we'll kick in the auto behavior for columns,
 and then later for the pager.

  2. stdout isn't a tty, in which case we also won't kick in the pager.

-Peff

Re: [RFC] column: show auto columns when pager is active

[Martin Ågren]

On 9 October 2017 at 23:45, Kevin Daudt <m...@ikke.info> wrote:
> When columns are set to automatic for git tag and the output is
> paginated by git, the output is a single column instead of multiple
> columns.
>
> Standard behaviour in git is to honor auto values when the pager is
> active, which happens for example with commands like git log showing
> colors when being paged.
>
> Since ff1e72483 (tag: change default of `pager.tag` to "on",
> 2017-08-02), the pager has been enabled by default, exposing this
> problem to more people.

Oh. :( I didn't know about "column" to be honest.

> finalize_colopts in column.c only checks whether the output is a TTY to
> determine if columns should be enabled with columns set to autol. Also check
> if the pager is active.
>
> Helped-by: Rafael Ascensão <rafa.al...@gmail.com>
> Signed-off-by: Kevin Daudt <m...@ikke.info>
> ---
> This came to light when someone wondered on irc why
> column.tag=[auto|always] did not work on Mac OS. Testing it myself, I
> found it to work with always, but not with auto.
>
> I could not get the test to work yet, because somehow it's not giving
> any output, so feedback regarding that is welcome.

I had slightly more success with PAGER="cat >actual", but the test is
flaky for some reason. In any case, it might make sense to test an
actual use-case also. Of course, the code should be largely the same,
but in builtin/tag.c, it's quite important that `setup_auto_pager()`
and `finalize_colopts()` are called in the right order. In other words,
there is some regression-potential. This is a whitespace-damaged and
hacky attempt to test. Maybe it helps a little. I hope I'll have more
time later today.

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index f0f1abd1c..91f2b5871 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -214,6 +214,19 @@ test_expect_success TTY 'git tag as alias
respects pager.tag with -l' '
! test -e paginated.out
 '

+test_expect_success TTY 'git tag with column.tag=auto' '
+   test_commit second &&
+   test_commit third &&
+   test_commit fourth &&
+   test_when_finished "git reset --hard HEAD~3" &&
+   cat >expected <<\EOF &&
+fourth   initial  second   third
+EOF
+   rm -f paginated.out &&
+   test_terminal git -c pager.tag -c column.tag=auto tag &&
+   test_cmp expected paginated.out
+'
+
 # A colored commit log will begin with an appropriate ANSI escape
 # for the first color; the text "commit" comes later.
 colorful() {

Re: [RFC] column: show auto columns when pager is active

[Eric Sunshine]

On Mon, Oct 9, 2017 at 5:45 PM, Kevin Daudt <m...@ikke.info> wrote:
> When columns are set to automatic for git tag and the output is
> paginated by git, the output is a single column instead of multiple
> columns.
>
> Standard behaviour in git is to honor auto values when the pager is
> active, which happens for example with commands like git log showing
> colors when being paged.
>
> Since ff1e72483 (tag: change default of `pager.tag` to "on",
> 2017-08-02), the pager has been enabled by default, exposing this
> problem to more people.
>
> finalize_colopts in column.c only checks whether the output is a TTY to
> determine if columns should be enabled with columns set to autol. Also check

Presumably: s/autol/auto/

> if the pager is active.
>
> Helped-by: Rafael Ascensão <rafa.al...@gmail.com>
> Signed-off-by: Kevin Daudt <m...@ikke.info>

[RFC] column: show auto columns when pager is active

[Kevin Daudt]

When columns are set to automatic for git tag and the output is
paginated by git, the output is a single column instead of multiple
columns.

Standard behaviour in git is to honor auto values when the pager is
active, which happens for example with commands like git log showing
colors when being paged.

Since ff1e72483 (tag: change default of `pager.tag` to "on",
2017-08-02), the pager has been enabled by default, exposing this
problem to more people.

finalize_colopts in column.c only checks whether the output is a TTY to
determine if columns should be enabled with columns set to autol. Also check
if the pager is active.

Helped-by: Rafael Ascensão <rafa.al...@gmail.com>
Signed-off-by: Kevin Daudt <m...@ikke.info>
---
This came to light when someone wondered on irc why
column.tag=[auto|always] did not work on Mac OS. Testing it myself, I
found it to work with always, but not with auto.

I could not get the test to work yet, because somehow it's not giving
any output, so feedback regarding that is welcome.


 column.c  |  3 ++-
 t/t9002-column.sh | 13 +
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/column.c b/column.c
index ff7bdab1a..ded50337f 100644
--- a/column.c
+++ b/column.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "run-command.h"
 #include "utf8.h"
+#include "pager.c"
 
 #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \
(x) * (d)->rows + (y) : \
@@ -224,7 +225,7 @@ int finalize_colopts(unsigned int *colopts, int 
stdout_is_tty)
if (stdout_is_tty < 0)
stdout_is_tty = isatty(1);
*colopts &= ~COL_ENABLE_MASK;
-   if (stdout_is_tty)
+   if (stdout_is_tty || pager_in_use())
*colopts |= COL_ENABLED;
}
return 0;
diff --git a/t/t9002-column.sh b/t/t9002-column.sh
index 89983527b..9441145bf 100755
--- a/t/t9002-column.sh
+++ b/t/t9002-column.sh
@@ -2,6 +2,7 @@
 
 test_description='git column'
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 test_expect_success 'setup' '
cat >lista <<\EOF
@@ -177,4 +178,16 @@ EOF
test_cmp expected actual
 '
 
+test_expect_success TTY '20 columns, mode auto, pager' '
+   cat >expected <<\EOF &&
+oneseven
+twoeight
+three  nine
+four   ten
+five   eleven
+six
+EOF
+   test_terminal env PAGER="cat|cat" git column --mode=auto actual 
&&
+   test_cmp expected actual
+'
 test_done
-- 
2.14.2

[PATCH v3 7/7] git.c: ignore pager.* when launching builtin as dashed external

[Martin Ågren]

When running, e.g., `git -c alias.bar=foo bar`, we expand the alias and
execute `git-foo` as a dashed external. This is true even if git foo is
a builtin. That is on purpose, and is motivated in a comment which was
added in commit 441981bc ("git: simplify environment save/restore
logic", 2016-01-26).

Shortly before we launch a dashed external, and unless we have already
found out whether we should use a pager, we check `pager.foo`. This was
added in commit 92058e4d ("support pager.* for external commands",
2011-08-18). If the dashed external is a builtin, this does not match
that commit's intention and is arguably wrong, since it would be cleaner
if we let the "dashed external builtin" handle `pager.foo`.

This has not mattered in practice, but a recent patch taught `git-tag`
to ignore `pager.tag` under certain circumstances. But, when started
using an alias, it doesn't get the chance to do so, as outlined above.
That recent patch added a test to document this breakage.

Do not check `pager.foo` before launching a builtin as a dashed
external, i.e., if we recognize the name of the external as a builtin.
Change the test to use `test_expect_success`.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 t/t7006-pager.sh | 2 +-
 git.c| 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index afa03f3b6..9128ec5ac 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -201,7 +201,7 @@ test_expect_success TTY 'git tag -a respects --paginate' '
test -e paginated.out
 '
 
-test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '
+test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '
test_when_finished "git tag -d newtag" &&
rm -f paginated.out &&
test_terminal git -c pager.tag -c alias.t=tag t -am message newtag &&
diff --git a/git.c b/git.c
index 82ac2a092..6b6d9f68e 100644
--- a/git.c
+++ b/git.c
@@ -559,7 +559,7 @@ static void execv_dashed_external(const char **argv)
if (get_super_prefix())
die("%s doesn't support --super-prefix", argv[0]);
 
-   if (use_pager == -1)
+   if (use_pager == -1 && !is_builtin(argv[0]))
use_pager = check_pager_config(argv[0]);
commit_pager_choice();
 
-- 
2.14.0.rc1.12.ge2d9c4613

Re: [PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external

[Martin Ågren]

On 31 July 2017 at 05:45, Jeff King  wrote:
> On Mon, Jul 17, 2017 at 10:10:52PM +0200, Martin Ågren wrote:
>
>> One could address this in run_argv(), by making the second call to
>> execv_dashed_external() conditional on "!is_builtin()" whereas a builtin
>> would be started as "git foo". (Possibly after unrolling and cleaning up
>> the "while (1)"-loop.) That seems like the wrong fix for this particular
>> issue, but might be a wanted change on its own -- or maybe not --, since
>> it would mean one could relay, e.g., "-c baz" to "git -c baz foo" (but
>> only for builtins...).
>
> We shouldn't need to relay them. They get added to the environment by
> the initial "git" invocation, and then are available everywhere (in
> fact, it would be wrong to relay them for multi-valued config).

Thanks for explaining. I did some very sloppy reading of the comment
in git.c that we "cannot take flags in between the 'git' and the
''" which I somehow misunderstood completely as "we cannot pass
that sort of information to git-". Silly. Thanks for taking the
time to explain what I should have found out myself...

So yeah, I meant the above and not this:

> Or did
> you mean that we could potentially allow:
>
>   [alias]
>   foo = "-c baz some-builtin"
>
> That's interesting, but I think the fact that it only works with
> builtins makes it a bad idea. And you can always do:
>
>   [alias]
>   foo = "!git -c baz some-builtin"
>
> which is equivalent.

Re: [PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external

[Jeff King]

On Mon, Jul 17, 2017 at 10:10:52PM +0200, Martin Ågren wrote:

> When running, e.g., `git -c alias.bar=foo bar`, we expand the alias and
> execute `git-foo` as a dashed external. This is true even if git foo is
> a builtin. That is on purpose, and is motivated in a comment which was
> added in commit 441981bc ("git: simplify environment save/restore
> logic", 2016-01-26).
> 
> Shortly before we launch a dashed external, and unless we have already
> found out whether we should use a pager, we check `pager.foo`. This was
> added in commit 92058e4d ("support pager.* for external commands",
> 2011-08-18). If the dashed external is a builtin, this does not match
> that commit's intention and is arguably wrong, since it would be cleaner
> if we let the "dashed external builtin" handle `pager.foo`.
> 
> This has not mattered in practice, but a recent patch taught `git-tag`
> to ignore `pager.tag` under certain circumstances. But, when started
> using an alias, it doesn't get the chance to do so, as outlined above.
> That recent patch added a test to document this breakage.
> 
> Do not check `pager.foo` before launching a builtin as a dashed
> external, i.e., if we recognize the name of the external as a builtin.
> Change the test to use `test_expect_success`.

I think floating this change to the end like this has made it much
easier to see why we must do it. The end result looks good to me.

> Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
> ---
> One could address this in run_argv(), by making the second call to
> execv_dashed_external() conditional on "!is_builtin()" whereas a builtin
> would be started as "git foo". (Possibly after unrolling and cleaning up
> the "while (1)"-loop.) That seems like the wrong fix for this particular
> issue, but might be a wanted change on its own -- or maybe not --, since
> it would mean one could relay, e.g., "-c baz" to "git -c baz foo" (but
> only for builtins...).

We shouldn't need to relay them. They get added to the environment by
the initial "git" invocation, and then are available everywhere (in
fact, it would be wrong to relay them for multi-valued config). Or did
you mean that we could potentially allow:

  [alias]
  foo = "-c baz some-builtin"

That's interesting, but I think the fact that it only works with
builtins makes it a bad idea. And you can always do:

  [alias]
  foo = "!git -c baz some-builtin"

which is equivalent.

-Peff

[PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external

[Martin Ågren]

When running, e.g., `git -c alias.bar=foo bar`, we expand the alias and
execute `git-foo` as a dashed external. This is true even if git foo is
a builtin. That is on purpose, and is motivated in a comment which was
added in commit 441981bc ("git: simplify environment save/restore
logic", 2016-01-26).

Shortly before we launch a dashed external, and unless we have already
found out whether we should use a pager, we check `pager.foo`. This was
added in commit 92058e4d ("support pager.* for external commands",
2011-08-18). If the dashed external is a builtin, this does not match
that commit's intention and is arguably wrong, since it would be cleaner
if we let the "dashed external builtin" handle `pager.foo`.

This has not mattered in practice, but a recent patch taught `git-tag`
to ignore `pager.tag` under certain circumstances. But, when started
using an alias, it doesn't get the chance to do so, as outlined above.
That recent patch added a test to document this breakage.

Do not check `pager.foo` before launching a builtin as a dashed
external, i.e., if we recognize the name of the external as a builtin.
Change the test to use `test_expect_success`.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
One could address this in run_argv(), by making the second call to
execv_dashed_external() conditional on "!is_builtin()" whereas a builtin
would be started as "git foo". (Possibly after unrolling and cleaning up
the "while (1)"-loop.) That seems like the wrong fix for this particular
issue, but might be a wanted change on its own -- or maybe not --, since
it would mean one could relay, e.g., "-c baz" to "git -c baz foo" (but
only for builtins...).

 t/t7006-pager.sh | 2 +-
 git.c| 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index df258c5d4..8b2ffb1aa 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -201,7 +201,7 @@ test_expect_success TTY 'git tag -a respects --paginate' '
test -e paginated.out
 '
 
-test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '
+test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '
# git-tag will be launched as a dashed external, which
# 1) is the source of a potential bug, and
# 2) is why we use test_config and not -c.
diff --git a/git.c b/git.c
index 82ac2a092..6b6d9f68e 100644
--- a/git.c
+++ b/git.c
@@ -559,7 +559,7 @@ static void execv_dashed_external(const char **argv)
if (get_super_prefix())
die("%s doesn't support --super-prefix", argv[0]);
 
-   if (use_pager == -1)
+   if (use_pager == -1 && !is_builtin(argv[0]))
use_pager = check_pager_config(argv[0]);
commit_pager_choice();
 
-- 
2.14.0.rc0

Re: [PATCH 0/7] tag: more fine-grained pager-configuration

[Martin Ågren]

On 11 July 2017 at 00:42, Junio C Hamano <gits...@pobox.com> wrote:
> Martin Ågren <martin.ag...@gmail.com> writes:
>> I am not moving all builtins to handling the pager on their own,
>> but instead introduce a flag IGNORE_PAGER_CONFIG and use it only
>> with the tag builtin. That means there's another flag to reason
>> about, but it avoids moving all builtins to handling the paging
>> themselves just to make one of them do something more "clever".
...
> Even though it is purely internal thing, IGNORE_PAGER_CONFIG
> probably is a bit confusion-inducing name.  After all, the
> subcommand specific configuration is not being ignored---we are
> merely delaying our reaction to it---instead of acting on it inside
> git.c without giving the subcommand a chance to make a decision, we
> are still letting (and we do expect) the subcommand to react to it.

Thank you for your comments. I'm working on v2 with the input from
you, Peff and Brandon. I'll make this DELAY_PAGER_CONFIG.

Martin

Re: [PATCH 0/7] tag: more fine-grained pager-configuration

[Junio C Hamano]

Jeff King  writes:

>> I see you CC'ed Peff who's passionate in this area, so these patches
>> are in good hands already ;-) I briefly skimmed your patches myself,
>> and did not spot anything glaringly wrong.
>
> Heh, I don't think of "paging tag output" as one of my passions, but you
> may be right. :)

Perhaps not "tag output", but the paging is the area you are the
person who spent most time on.

> I left a few comments on the code and direction, but I agree that
> overall it looks pretty good. And I'm very impressed with the attention
> to detail for a first-time contributor.

Yes.

Re: [PATCH 0/7] tag: more fine-grained pager-configuration

[Jeff King]

On Tue, Jul 11, 2017 at 03:50:39PM +0200, Martin Ågren wrote:

> > Would it makes sense to just have git-tag respect pager.tag in listing
> > mode, and otherwise ignore it completely?
> 
> Yes. I doubt anyone would notice, and no-one should mind with the
> change (famous last words).
> 
> It does mean there's a precedence for individual commands to get to
> choose when to honor pager.foo. If more such exceptions are added, at
> some point, some command will learn to ignore pager.foo in some
> particular situation and someone will consider it a regression.

Yes, perhaps. One could even argue that "git tag foo" is OK to page and
somebody would consider it a regression not to respect pager.tag. It
seems useless to me, but at least it's not totally broken like "git tag
-a foo" is.

But I find it pretty unlikely, as it doesn't produce copious output. I'd
worry more about eventually finding a command with two copious-output
modes, and somebody wants to distinguish between them. If we can't come
up with a plausible example now, I'm inclined to deal with it if and
when it ever comes up. Right now I just don't want to paint ourselves
into a corner, and I think we can always add more config later (e.g.,
tag.pageFooCommand=true or something; that's not great, but I'm mostly
betting that it will never come up).

> Well, I respect your hunch about .enable and .command and I certainly
> don't want to take things in a direction that makes that approach less
> clean. You have convinced me that I will instead try to teach git tag
> to be more clever about when to use the pager, at least to see what
> that looks like.

Thanks. I was the original proponent of "pager.tag.list", and only after
reading your series today did I think "gee, this seems unnecessarily
complex". So it's entirely possible I'm missing a corner case that you
may uncover through discussion or experimenting.

> Let's call such a "git tag" the "future git tag". Just to convince
> myself I've thought through the implications -- how would
> pager.tag.enable=true affect that future git tag? Would it be fair to
> say that enable=false means "definitely don't start the pager (unless
> --paginate)" and that enable=true means "feel free to use it (unless
> --no-paginate)"? The future git tag would default to using
> enable=true. Would --paginate also be "feel free to use it", or rather
> "the pager must be used"?

I think the rules would be:

  1. If --paginate is given, do that. Likewise for --no-pager.

  2. Otherwise, respect tag.pager.enable if it is set.

  3. Otherwise, use the built-in default.

  4. Any time the pager is run, respect tag.pager.command if it is set.

And then there are two optional bits:

  2a. If tag.pager is set to a boolean, behave as if tag.pager.enable is
  set to the same boolean. If it's set to a string, behave as if
  tag.pager.enable is set to "true", and tag.pager.command is set to
  the string. That should give us backwards compatibility.

  2b. If tag.pager.command is set but tag.pager.enable isn't, possibly we
  should assume that tag.pager.enable is set to "true". That makes
  the common case of "page and use this command" a single config
  block instead of two. It matters less for "tag" where paging would
  be the default.

So I think that what you asked above, but to be clear on the final
question: --paginate should always be "must be used". And that meshes
nicely with implementing it via the git.c wrapper, where it takes
precedence way before we ever hit the setup_auto_pager() call.

-Peff

Re: [PATCH 0/7] tag: more fine-grained pager-configuration

[Martin Ågren]

On 11 July 2017 at 12:19, Jeff King <p...@peff.net> wrote:
> On Mon, Jul 10, 2017 at 11:55:13PM +0200, Martin Ågren wrote:
>
>> Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such
>> as "Vim: Warning: Output is not to a terminal" and a garbled terminal.
>> A user who makes use of `git tag -a` and `git tag -l` will probably
>> choose not to configure `pager.tag` or to set it to "no", so that `git
>> tag -a` will actually work, at the cost of not getting the pager with
>> `git tag -l`.
>
> Right, I think we are all agreed that "pager.tag" as it is now is
> essentially worthless.
>
> If I understand your series correctly, though, it adds pager.tag.list
> but leaves "pager.tag" behaving more or less the same. It's good that we
> now have a way for a user to do the thing they actually want, but it
> feels like we're leaving pager.tag behind as a booby-trap.
>
> Should we disable it entirely (or only respect it in list mode)?
>
> At which point, I wonder if we actually need pager.tag.list at all.
> Slicing up the namespace further would be valuable if there were a
> command which had two pager-worthy modes, and somebody might want to
> enable the pager for one but not the other. But it seems like most
> commands in this boat (e.g., tag, branch, stash) really have two modes:
> listing things or creating things.
>
> Would it makes sense to just have git-tag respect pager.tag in listing
> mode, and otherwise ignore it completely?

Yes. I doubt anyone would notice, and no-one should mind with the
change (famous last words).

It does mean there's a precedence for individual commands to get to
choose when to honor pager.foo. If more such exceptions are added, at
some point, some command will learn to ignore pager.foo in some
particular situation and someone will consider it a regression.

> One nice side effect is that it keeps the multi-level pager.X.Y
> namespace clear. We've talked about transitioning to allow:
>
>   [pager "foo"]
>   enable = true
>   command = some-custom-pager
>
> to configure aspects of the pager separately for git-foo. This has two
> benefits:
>
>   1. Syntactically, it allows configuration for commands whose names
>  aren't valid config keys.
>
>   2. It would allow setting a command with "enable=false", so that "git
>  foo" did not paginate, but "git -p foo" paginated with a custom
>  command.
>
> Those are admittedly minor features. And assuming we don't go crazy with
> the multi-level names, we could have "pager.tag.list" live alongside
> "pager.tag.command". So it's not really an objection, so much as wonder
> out loud whether we can keep this as simple as possible.

Well, I respect your hunch about .enable and .command and I certainly
don't want to take things in a direction that makes that approach less
clean. You have convinced me that I will instead try to teach git tag
to be more clever about when to use the pager, at least to see what
that looks like.

Let's call such a "git tag" the "future git tag". Just to convince
myself I've thought through the implications -- how would
pager.tag.enable=true affect that future git tag? Would it be fair to
say that enable=false means "definitely don't start the pager (unless
--paginate)" and that enable=true means "feel free to use it (unless
--no-paginate)"? The future git tag would default to using
enable=true. Would --paginate also be "feel free to use it", or rather
"the pager must be used"?

At some point, I thought about "true"/"false"/"maybe", where "maybe"
would be what the future git tag implements. Of course, there's a fair
chance not everyone will agree what exactly should be paged with
"maybe". So it's back to adding various knobs. ;)

Anyway, this is more my thinking out loud. I'll drop pager.tag.list in
v2 and will instead make pager.tag more clever. That should force me
to think through this some more.

>> This is an attempt to implement something like that. I decided to let
>> `pager.tag.list` fall back to `pager.tag` before falling back to "on".
>> The default for `pager.tag` is still "off". I can see how that might
>> seem confusing. However, my argument is that it would be awkward for
>> `git tag -l` to ignore `pager.tag` -- we are after all running a
>> subcommand of `git tag`. Also, this avoids a regression for someone
>> who has set `pager.tag` and uses `git tag -l`.
>
> Yeah, I agree that turning "pager.tag" into a complete noop is probably
> a bad idea. But if we made it a silent noop for the non-list cases, that
> would be OK (and the hypothetical user who set it to make `git tag -l`
> work would see a strict improvement; they'd still get their paging but
> not the weird breakage with non-list modes). And I think that applies
> whether we have a pager.tag.list in addition or not.

Good thinking.

Thanks a lot for your comments. I appreciate it.

Martin

Re: [PATCH 0/7] tag: more fine-grained pager-configuration

[Jeff King]

On Mon, Jul 10, 2017 at 03:42:14PM -0700, Junio C Hamano wrote:

> > A review would be much appreciated. Comments on the way I
> > structured the series would be just as welcome as ones on the
> > final result.
> 
> I see you CC'ed Peff who's passionate in this area, so these patches
> are in good hands already ;-) I briefly skimmed your patches myself,
> and did not spot anything glaringly wrong.

Heh, I don't think of "paging tag output" as one of my passions, but you
may be right. :)

I left a few comments on the code and direction, but I agree that
overall it looks pretty good. And I'm very impressed with the attention
to detail for a first-time contributor.

-Peff

Re: [PATCH 7/7] tag: make git tag -l use pager by default

[Jeff King]

On Mon, Jul 10, 2017 at 11:55:20PM +0200, Martin Ågren wrote:

> The previous patch introduced `pager.tag.list`. Its default was to use
> the value of `pager.tag` or, if that was also not set, to fall back to
> "off".
> 
> Change that fallback value to "on". Let the default value for
> `pager.tag` remain at "off". Update documentation and tests.

Thanks for splitting this out. The default setting is definitely
orthogonal to allowing the config.

I've been running with this default for several years now (using the
patch I showed in the earlier thread you linked). It _does_ occasionally
annoy me, but I think overall it's an improvement. So it seems like a
good thing to try, and we can see how people respond as they try it out.

-Peff

Re: [PATCH 0/7] tag: more fine-grained pager-configuration

[Jeff King]

On Mon, Jul 10, 2017 at 11:55:13PM +0200, Martin Ågren wrote:

> Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such
> as "Vim: Warning: Output is not to a terminal" and a garbled terminal.
> A user who makes use of `git tag -a` and `git tag -l` will probably
> choose not to configure `pager.tag` or to set it to "no", so that `git
> tag -a` will actually work, at the cost of not getting the pager with
> `git tag -l`.

Right, I think we are all agreed that "pager.tag" as it is now is
essentially worthless.

If I understand your series correctly, though, it adds pager.tag.list
but leaves "pager.tag" behaving more or less the same. It's good that we
now have a way for a user to do the thing they actually want, but it
feels like we're leaving pager.tag behind as a booby-trap.

Should we disable it entirely (or only respect it in list mode)?

At which point, I wonder if we actually need pager.tag.list at all.
Slicing up the namespace further would be valuable if there were a
command which had two pager-worthy modes, and somebody might want to
enable the pager for one but not the other. But it seems like most
commands in this boat (e.g., tag, branch, stash) really have two modes:
listing things or creating things.

Would it makes sense to just have git-tag respect pager.tag in listing
mode, and otherwise ignore it completely?

One nice side effect is that it keeps the multi-level pager.X.Y
namespace clear. We've talked about transitioning to allow:

  [pager "foo"]
  enable = true
  command = some-custom-pager

to configure aspects of the pager separately for git-foo. This has two
benefits:

  1. Syntactically, it allows configuration for commands whose names
 aren't valid config keys.

  2. It would allow setting a command with "enable=false", so that "git
 foo" did not paginate, but "git -p foo" paginated with a custom
 command.

Those are admittedly minor features. And assuming we don't go crazy with
the multi-level names, we could have "pager.tag.list" live alongside
"pager.tag.command". So it's not really an objection, so much as wonder
out loud whether we can keep this as simple as possible.

> This is an attempt to implement something like that. I decided to let
> `pager.tag.list` fall back to `pager.tag` before falling back to "on".
> The default for `pager.tag` is still "off". I can see how that might
> seem confusing. However, my argument is that it would be awkward for
> `git tag -l` to ignore `pager.tag` -- we are after all running a
> subcommand of `git tag`. Also, this avoids a regression for someone
> who has set `pager.tag` and uses `git tag -l`.

Yeah, I agree that turning "pager.tag" into a complete noop is probably
a bad idea. But if we made it a silent noop for the non-list cases, that
would be OK (and the hypothetical user who set it to make `git tag -l`
work would see a strict improvement; they'd still get their paging but
not the weird breakage with non-list modes). And I think that applies
whether we have a pager.tag.list in addition or not.

> I am not moving all builtins to handling the pager on their own, but
> instead introduce a flag IGNORE_PAGER_CONFIG and use it only with the
> tag builtin. That means there's another flag to reason about, but it
> avoids moving all builtins to handling the paging themselves just to
> make one of them do something more "clever".

I think this is very sensible. It's the vast minority that would want to
enable this (git-branch is the other obvious one). At some point we may
need a plan to handle non-builtins (like stash), but that's largely
orthogonal to what you're doing here.

> The discussion mentioned above discusses various approaches. It also
> notes how the current pager.foo-configuration conflates _whether_ and
> _how_ to run a pager. Arguably, this series paints ourselves even
> further into that corner. The idea of `pager.foo.command` and
> `pager.foo.enabled` has been mentioned and this series might make such
> an approach slightly less clean, conceptually. We could introduce
> `paging.foo` as a "yes"/"no"/"maybe" to go alongside `pager.foo` which
> is then "less"/"cat"/ That's definitely outside this series, but
> should not be prohibited by it...

I think I covered my thoughts on this part above. :)

> A review would be much appreciated. Comments on the way I structured
> the series would be just as welcome as ones on the final result.

Overall the code looks good, though I'll respond with a few comments.

-Peff

Re: [PATCH 0/7] tag: more fine-grained pager-configuration

[Junio C Hamano]

Martin Ågren <martin.ag...@gmail.com> writes:

> Using, e.g., `git -c pager.tag tag -a new-tag` results in errors
> such as "Vim: Warning: Output is not to a terminal" and a garbled
> terminal. A user who makes use of `git tag -a` and `git tag -l`
> will probably choose not to configure `pager.tag` or to set it to
> "no", so that `git tag -a` will actually work, at the cost of not
> getting the pager with `git tag -l`.
>
> In the discussion at [1], it was brought up that 1) the individual
> builtins should be in charge of setting up the paging (as opposed
> to git.c which has no knowledge about what the command is about to
> do) and that 2) there could then be a configuration
> `pager.tag.list` to address the specific problem of `git tag`.
>
> This is an attempt to implement something like that. I decided to
> let `pager.tag.list` fall back to `pager.tag` before falling back
> to "on". The default for `pager.tag` is still "off". I can see how
> that might seem confusing. However, my argument is that it would
> be awkward for `git tag -l` to ignore `pager.tag` -- we are after
> all running a subcommand of `git tag`. Also, this avoids a
> regression for someone who has set `pager.tag` and uses `git tag
> -l`.
>
> I am not moving all builtins to handling the pager on their own,
> but instead introduce a flag IGNORE_PAGER_CONFIG and use it only
> with the tag builtin. That means there's another flag to reason
> about, but it avoids moving all builtins to handling the paging
> themselves just to make one of them do something more "clever".

All of the above smells like taking us in a sensible direction.  I
agree with these design decisions described in the above, i.e.
'pager.tag.list falling back to pager.tag', making this an opt-in to
make code transition easier.

Even though it is purely internal thing, IGNORE_PAGER_CONFIG
probably is a bit confusion-inducing name.  After all, the
subcommand specific configuration is not being ignored---we are
merely delaying our reaction to it---instead of acting on it inside
git.c without giving the subcommand a chance to make a decision, we
are still letting (and we do expect) the subcommand to react to it.

But that is a fairly minor thing we can fix.

> A review would be much appreciated. Comments on the way I
> structured the series would be just as welcome as ones on the
> final result.

I see you CC'ed Peff who's passionate in this area, so these patches
are in good hands already ;-) I briefly skimmed your patches myself,
and did not spot anything glaringly wrong.

Thanks.

[PATCH 7/7] tag: make git tag -l use pager by default

[Martin Ågren]

The previous patch introduced `pager.tag.list`. Its default was to use
the value of `pager.tag` or, if that was also not set, to fall back to
"off".

Change that fallback value to "on". Let the default value for
`pager.tag` remain at "off". Update documentation and tests.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 Documentation/git-tag.txt |  2 +-
 builtin/tag.c |  2 +-
 t/t7006-pager.sh  | 12 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 6ad5811a2..fbdfb3f59 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -103,7 +103,7 @@ as `--contains` is provided. See the documentation for each 
of those
 options for details.
 +
 When determining whether to use a pager, `pager.tag.list` is considered
-before `pager.tag`.
+before `pager.tag`. If neither is set, the default is to use a pager.
 See linkgit:git-config[1].
 
 --sort=::
diff --git a/builtin/tag.c b/builtin/tag.c
index e96ef7d70..ec69fca61 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -447,7 +447,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
if (cmdmode == 'l')
-   setup_auto_pager("tag.list", 0);
+   setup_auto_pager("tag.list", 1);
setup_auto_pager("tag", 0);
 
if (keyid) {
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index ed34c6734..94df89a5f 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -134,22 +134,22 @@ test_expect_success TTY 'configuration can enable pager 
(from subdir)' '
}
 '
 
-test_expect_success TTY 'git tag -l defaults to not paging' '
+test_expect_success TTY 'git tag -l defaults to paging' '
rm -f paginated.out &&
test_terminal git tag -l &&
-   ! test -e paginated.out
+   test -e paginated.out
 '
 
 test_expect_success TTY 'git tag -l respects pager.tag' '
rm -f paginated.out &&
-   test_terminal git -c pager.tag tag -l &&
-   test -e paginated.out
+   test_terminal git -c pager.tag=false tag -l &&
+   ! test -e paginated.out
 '
 
 test_expect_success TTY 'git tag -l respects pager.tag.list' '
rm -f paginated.out &&
-   test_terminal git -c pager.tag=false -c pager.tag.list tag -l &&
-   test -e paginated.out
+   test_terminal git -c pager.tag -c pager.tag.list=false tag -l &&
+   ! test -e paginated.out
 '
 
 test_expect_success TTY 'git tag -l respects --no-pager' '
-- 
2.13.2.653.gfb5159d

[PATCH 0/7] tag: more fine-grained pager-configuration

[Martin Ågren]

Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such as "Vim: 
Warning: Output is not to a terminal" and a garbled terminal. A user who makes 
use of `git tag -a` and `git tag -l` will probably choose not to configure 
`pager.tag` or to set it to "no", so that `git tag -a` will actually work, at 
the cost of not getting the pager with `git tag -l`.

In the discussion at [1], it was brought up that 1) the individual builtins 
should be in charge of setting up the paging (as opposed to git.c which has no 
knowledge about what the command is about to do) and that 2) there could then 
be a configuration `pager.tag.list` to address the specific problem of `git 
tag`.

This is an attempt to implement something like that. I decided to let 
`pager.tag.list` fall back to `pager.tag` before falling back to "on". The 
default for `pager.tag` is still "off". I can see how that might seem 
confusing. However, my argument is that it would be awkward for `git tag -l` to 
ignore `pager.tag` -- we are after all running a subcommand of `git tag`. Also, 
this avoids a regression for someone who has set `pager.tag` and uses `git tag 
-l`.

I am not moving all builtins to handling the pager on their own, but instead 
introduce a flag IGNORE_PAGER_CONFIG and use it only with the tag builtin. That 
means there's another flag to reason about, but it avoids moving all builtins 
to handling the paging themselves just to make one of them do something more 
"clever".

The discussion mentioned above discusses various approaches. It also notes how 
the current pager.foo-configuration conflates _whether_ and _how_ to run a 
pager. Arguably, this series paints ourselves even further into that corner. 
The idea of `pager.foo.command` and `pager.foo.enabled` has been mentioned and 
this series might make such an approach slightly less clean, conceptually. We 
could introduce `paging.foo` as a "yes"/"no"/"maybe" to go alongside 
`pager.foo` which is then "less"/"cat"/ That's definitely outside this 
series, but should not be prohibited by it...

A review would be much appreciated. Comments on the way I structured the series 
would be just as welcome as ones on the final result.

Martin

[1] https://public-inbox.org/git/nrmbrl$hsk$1...@blaine.gmane.org/T/

Martin Ågren (7):
  api-builtin.txt: document SUPPORT_SUPER_PREFIX
  git.c: let builtins opt for handling `pager.foo` themselves
  git.c: provide setup_auto_pager()
  t7006: add tests for how git tag paginates
  tag: handle `pager.tag`-configuration within the builtin
  tag: make git tag -l consider new config `pager.tag.list`
  tag: make git tag -l use pager by default

 Documentation/git-tag.txt   |  4 +++
 Documentation/technical/api-builtin.txt | 10 ++
 builtin.h   | 14 +
 builtin/tag.c   |  4 +++
 git.c   | 44 +--
 t/t7006-pager.sh| 54 +
 6 files changed, 127 insertions(+), 3 deletions(-)

-- 
2.13.2.653.gfb5159d

Re: git diff sometimes brings up buggy pager

[Samuel Lijin]

Any chance you can tell us what repo this happens on? + git version,
OS, and what the triggering diff invocation is.

On Thu, Jun 15, 2017 at 12:19 PM, Matthew Groth  wrote:
> When I do `git diff` sometimes I get this:
>
>
> ...skipping...
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ...skipping...
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ...skipping...
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
>
>
>  it goes on like this for about 10 times the length. Looks like this
> happens exclusively when I use git diff with a github remote that is at the
> same commit. I will update if I find any other case where this happens.
>
>
>
>

git diff sometimes brings up buggy pager

[Matthew Groth]

When I do `git diff` sometimes I get this:


...skipping...
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
...skipping...
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
...skipping...
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~
~


 it goes on like this for about 10 times the length. Looks like this 
happens exclusively when I use git diff with a github remote that is at 
the same commit. I will update if I find any other case where this happens.

Re: [PATCH 0/3] fix ^C killing pager when running alias

[Jacob Keller]

On Sat, Jan 7, 2017 at 3:26 PM, Jacob Keller <jacob.kel...@gmail.com> wrote:
> On Fri, Jan 6, 2017 at 5:14 PM, Jeff King <p...@peff.net> wrote:
>> On Fri, Jan 06, 2017 at 06:20:42PM -0500, Jeff King wrote:
>>
>>> > In general, I think it is wrong to wait for child processes when a signal
>>> > was received. After all, it is the purpose of a (deadly) signal to have 
>>> > the
>>> > process go away. There may be programs that know it better, like less, but
>>> > git should not attempt to know better in general.
>>> >
>>> > We do apply some special behavior for certain cases like we do for the
>>> > pager. And now the case with aliases is another special situation. The
>>> > parent git process only delegates to the child, and as such it is 
>>> > reasonable
>>> > that it binds its life time to the first child, which executes the 
>>> > expanded
>>> > alias.
>>>
>>> Yeah, I think I agree. That binding is something you want in many cases,
>>> but not necessarily all. The original purpose of clean_on_exit was to
>>> create a binding like that, but of course it can be (and with the
>>> smudge-filter stuff, arguably has been) used for other cases, too.
>>>
>>> I'll work up a patch that makes it a separate option, which should be
>>> pretty easy.
>>
>> Yeah, this did turn out to be really easy. I spent most of the time
>> trying to explain the issue in the commit message in a sane way.
>> Hopefully it didn't end up _too_ long. :)
>>
>> The interesting bit is in the third one. The first is a necessary
>> preparatory step, and the second is a cleanup I noticed in the
>> neighborhood.
>>
>>   [1/3]: execv_dashed_external: use child_process struct
>>   [2/3]: execv_dashed_external: stop exiting with negative code
>>   [3/3]: execv_dashed_external: wait for child on signal death
>>
>>  git.c | 36 +++-
>>  run-command.c | 19 +++
>>  run-command.h |  1 +
>>  3 files changed, 35 insertions(+), 21 deletions(-)
>>
>> -Peff
>
> I don't see the rest of the patches on the list..?
>
> Thanks,
> Jake

They showed up on public inbox so I assume it must be some spam filter
on my end..

Hmm,
Jake

Re: [PATCH 0/3] fix ^C killing pager when running alias

[Jacob Keller]

On Fri, Jan 6, 2017 at 5:14 PM, Jeff King <p...@peff.net> wrote:
> On Fri, Jan 06, 2017 at 06:20:42PM -0500, Jeff King wrote:
>
>> > In general, I think it is wrong to wait for child processes when a signal
>> > was received. After all, it is the purpose of a (deadly) signal to have the
>> > process go away. There may be programs that know it better, like less, but
>> > git should not attempt to know better in general.
>> >
>> > We do apply some special behavior for certain cases like we do for the
>> > pager. And now the case with aliases is another special situation. The
>> > parent git process only delegates to the child, and as such it is 
>> > reasonable
>> > that it binds its life time to the first child, which executes the expanded
>> > alias.
>>
>> Yeah, I think I agree. That binding is something you want in many cases,
>> but not necessarily all. The original purpose of clean_on_exit was to
>> create a binding like that, but of course it can be (and with the
>> smudge-filter stuff, arguably has been) used for other cases, too.
>>
>> I'll work up a patch that makes it a separate option, which should be
>> pretty easy.
>
> Yeah, this did turn out to be really easy. I spent most of the time
> trying to explain the issue in the commit message in a sane way.
> Hopefully it didn't end up _too_ long. :)
>
> The interesting bit is in the third one. The first is a necessary
> preparatory step, and the second is a cleanup I noticed in the
> neighborhood.
>
>   [1/3]: execv_dashed_external: use child_process struct
>   [2/3]: execv_dashed_external: stop exiting with negative code
>   [3/3]: execv_dashed_external: wait for child on signal death
>
>  git.c | 36 +++-
>  run-command.c | 19 +++
>  run-command.h |  1 +
>  3 files changed, 35 insertions(+), 21 deletions(-)
>
> -Peff

I don't see the rest of the patches on the list..?

Thanks,
Jake

[PATCH 0/3] fix ^C killing pager when running alias

[Jeff King]

On Fri, Jan 06, 2017 at 06:20:42PM -0500, Jeff King wrote:

> > In general, I think it is wrong to wait for child processes when a signal
> > was received. After all, it is the purpose of a (deadly) signal to have the
> > process go away. There may be programs that know it better, like less, but
> > git should not attempt to know better in general.
> > 
> > We do apply some special behavior for certain cases like we do for the
> > pager. And now the case with aliases is another special situation. The
> > parent git process only delegates to the child, and as such it is reasonable
> > that it binds its life time to the first child, which executes the expanded
> > alias.
> 
> Yeah, I think I agree. That binding is something you want in many cases,
> but not necessarily all. The original purpose of clean_on_exit was to
> create a binding like that, but of course it can be (and with the
> smudge-filter stuff, arguably has been) used for other cases, too.
> 
> I'll work up a patch that makes it a separate option, which should be
> pretty easy.

Yeah, this did turn out to be really easy. I spent most of the time
trying to explain the issue in the commit message in a sane way.
Hopefully it didn't end up _too_ long. :)

The interesting bit is in the third one. The first is a necessary
preparatory step, and the second is a cleanup I noticed in the
neighborhood.

  [1/3]: execv_dashed_external: use child_process struct
  [2/3]: execv_dashed_external: stop exiting with negative code
  [3/3]: execv_dashed_external: wait for child on signal death

 git.c | 36 +++-
 run-command.c | 19 +++
 run-command.h |  1 +
 3 files changed, 35 insertions(+), 21 deletions(-)

-Peff

Re: Regression: Ctrl-c from the pager in an alias exits it

[Jeff King]

On Fri, Jan 06, 2017 at 11:42:25PM +0100, Johannes Sixt wrote:

> > So I dunno. Maybe this waiting should be restricted only to certain
> > cases like executing git sub-commands.
> 
> If given it some thought.
> 
> In general, I think it is wrong to wait for child processes when a signal
> was received. After all, it is the purpose of a (deadly) signal to have the
> process go away. There may be programs that know it better, like less, but
> git should not attempt to know better in general.
> 
> We do apply some special behavior for certain cases like we do for the
> pager. And now the case with aliases is another special situation. The
> parent git process only delegates to the child, and as such it is reasonable
> that it binds its life time to the first child, which executes the expanded
> alias.

Yeah, I think I agree. That binding is something you want in many cases,
but not necessarily all. The original purpose of clean_on_exit was to
create a binding like that, but of course it can be (and with the
smudge-filter stuff, arguably has been) used for other cases, too.

I'll work up a patch that makes it a separate option, which should be
pretty easy.

-Peff

Re: Regression: Ctrl-c from the pager in an alias exits it

[Johannes Sixt]

Am 06.01.2017 um 20:41 schrieb Jeff King:

On Fri, Jan 06, 2017 at 03:39:59PM +0100, Johannes Sixt wrote:


diff --git a/run-command.c b/run-command.c
index ca905a9e80..db47c429b7 100644
--- a/run-command.c
+++ b/run-command.c
@@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;

 static void cleanup_children(int sig, int in_signal)
 {
+   struct child_to_clean *children_to_wait_for = NULL;
+
while (children_to_clean) {
struct child_to_clean *p = children_to_clean;
children_to_clean = p->next;
@@ -45,6 +47,17 @@ static void cleanup_children(int sig, int in_signal)
}

kill(p->pid, sig);
+   p->next = children_to_wait_for;
+   children_to_wait_for = p;
+   }
+
+   while (children_to_wait_for) {
+   struct child_to_clean *p = children_to_wait_for;
+   children_to_wait_for = p->next;
+
+   while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
+   ; /* spin waiting for process exit or error */
+
if (!in_signal)
free(p);
}



This looks like the minimal change necessary. I wonder, though, whether the
new local variable is really required. Wouldn't it be sufficient to walk the
children_to_clean chain twice?


Yeah, I considered that. The fact that we disassemble the list in the
first loop has two side effects:

  1. It lets us free the list as we go (for the !in_signal case).

  2. If we were to get another signal, it makes us sort-of reentrant. We
 will only kill and wait for each pid once.

Obviously (1) moves down to the lower loop, but I was trying to preserve
(2). I'm not sure if it is worth bothering, though.


Makes sense.


The way we pull
items off of the list is certainly not atomic (it does shorten the race
to a few instructions, though, versus potentially waiting on waitpid()
to return).

My bigger concern with the whole thing is whether we could hit some sort
of deadlock if the child doesn't die when we send it a signal. E.g.,
imagine we have a pipe open to the child and somebody sends SIGTERM to
us. We propagate SIGTERM to the child, and then waitpid() for it. The
child decides to ignore our SIGTERM for some reason and keep reading
until EOF on the pipe. It won't ever get it, and the two processes will
hang forever.

You can argue perhaps that the child is broken in that case. And I doubt
this could trigger when running a git sub-command. But we may add more
children in the future. Right now we use it for the new multi-file
clean/smudge filters. They use the hook feature to close the
descriptors, but note that that won't run in the in_signal case.

So I dunno. Maybe this waiting should be restricted only to certain
cases like executing git sub-commands.


If given it some thought.

In general, I think it is wrong to wait for child processes when a 
signal was received. After all, it is the purpose of a (deadly) signal 
to have the process go away. There may be programs that know it better, 
like less, but git should not attempt to know better in general.


We do apply some special behavior for certain cases like we do for the 
pager. And now the case with aliases is another special situation. The 
parent git process only delegates to the child, and as such it is 
reasonable that it binds its life time to the first child, which 
executes the expanded alias.


-- Hannes

Re: Regression: Ctrl-c from the pager in an alias exits it

[Jeff King]

On Fri, Jan 06, 2017 at 03:39:59PM +0100, Johannes Sixt wrote:

> > diff --git a/run-command.c b/run-command.c
> > index ca905a9e80..db47c429b7 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
> > 
> >  static void cleanup_children(int sig, int in_signal)
> >  {
> > +   struct child_to_clean *children_to_wait_for = NULL;
> > +
> > while (children_to_clean) {
> > struct child_to_clean *p = children_to_clean;
> > children_to_clean = p->next;
> > @@ -45,6 +47,17 @@ static void cleanup_children(int sig, int in_signal)
> > }
> > 
> > kill(p->pid, sig);
> > +   p->next = children_to_wait_for;
> > +   children_to_wait_for = p;
> > +   }
> > +
> > +   while (children_to_wait_for) {
> > +   struct child_to_clean *p = children_to_wait_for;
> > +   children_to_wait_for = p->next;
> > +
> > +   while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
> > +   ; /* spin waiting for process exit or error */
> > +
> > if (!in_signal)
> > free(p);
> > }
> > 
> 
> This looks like the minimal change necessary. I wonder, though, whether the
> new local variable is really required. Wouldn't it be sufficient to walk the
> children_to_clean chain twice?

Yeah, I considered that. The fact that we disassemble the list in the
first loop has two side effects:

  1. It lets us free the list as we go (for the !in_signal case).

  2. If we were to get another signal, it makes us sort-of reentrant. We
 will only kill and wait for each pid once.

Obviously (1) moves down to the lower loop, but I was trying to preserve
(2). I'm not sure if it is worth bothering, though. The way we pull
items off of the list is certainly not atomic (it does shorten the race
to a few instructions, though, versus potentially waiting on waitpid()
to return).

My bigger concern with the whole thing is whether we could hit some sort
of deadlock if the child doesn't die when we send it a signal. E.g.,
imagine we have a pipe open to the child and somebody sends SIGTERM to
us. We propagate SIGTERM to the child, and then waitpid() for it. The
child decides to ignore our SIGTERM for some reason and keep reading
until EOF on the pipe. It won't ever get it, and the two processes will
hang forever.

You can argue perhaps that the child is broken in that case. And I doubt
this could trigger when running a git sub-command. But we may add more
children in the future. Right now we use it for the new multi-file
clean/smudge filters. They use the hook feature to close the
descriptors, but note that that won't run in the in_signal case.

So I dunno. Maybe this waiting should be restricted only to certain
cases like executing git sub-commands.

-Peff

Re: Regression: Ctrl-c from the pager in an alias exits it

[Johannes Sixt]

Am 06.01.2017 um 08:32 schrieb Jeff King:

On Fri, Jan 06, 2017 at 02:26:02AM -0500, Jeff King wrote:


You'll notice that it actually calls wait() on the pager. That's due to
a3da882120 (pager: do wait_for_pager on signal death, 2009-01-22), which
IIRC was addressing a very similar problem. We want to stop feeding the
pager when we get a signal, but we don't want the main process to
actually exit, or the pager loses the controlling terminal.

In our new scenario we have an extra process, though. The git-log child
will wait on the pager, but the parent process can't. It doesn't know
about it. I think that it in turn needs to wait on the child when it
dies, and then the whole chain will stand still until the pager exits.


And here's a patch to do that. It seems to work.

I'll sleep on it and then write up a commit message tomorrow if it still
makes sense.

diff --git a/run-command.c b/run-command.c
index ca905a9e80..db47c429b7 100644
--- a/run-command.c
+++ b/run-command.c
@@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;

 static void cleanup_children(int sig, int in_signal)
 {
+   struct child_to_clean *children_to_wait_for = NULL;
+
while (children_to_clean) {
struct child_to_clean *p = children_to_clean;
children_to_clean = p->next;
@@ -45,6 +47,17 @@ static void cleanup_children(int sig, int in_signal)
}

kill(p->pid, sig);
+   p->next = children_to_wait_for;
+   children_to_wait_for = p;
+   }
+
+   while (children_to_wait_for) {
+   struct child_to_clean *p = children_to_wait_for;
+   children_to_wait_for = p->next;
+
+   while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
+   ; /* spin waiting for process exit or error */
+
if (!in_signal)
free(p);
}



This looks like the minimal change necessary. I wonder, though, whether 
the new local variable is really required. Wouldn't it be sufficient to 
walk the children_to_clean chain twice?


-- Hannes

Re: Regression: Ctrl-c from the pager in an alias exits it

[Trygve Aaberge]

On Fri, Jan 06, 2017 at 02:32:25 -0500, Jeff King wrote:
> And here's a patch to do that. It seems to work.

Nice, thanks! This works very well for me too.

-- 
Trygve Aaberge

Re: Regression: Ctrl-c from the pager in an alias exits it

[Jeff King]

On Fri, Jan 06, 2017 at 02:26:02AM -0500, Jeff King wrote:

> You'll notice that it actually calls wait() on the pager. That's due to
> a3da882120 (pager: do wait_for_pager on signal death, 2009-01-22), which
> IIRC was addressing a very similar problem. We want to stop feeding the
> pager when we get a signal, but we don't want the main process to
> actually exit, or the pager loses the controlling terminal.
> 
> In our new scenario we have an extra process, though. The git-log child
> will wait on the pager, but the parent process can't. It doesn't know
> about it. I think that it in turn needs to wait on the child when it
> dies, and then the whole chain will stand still until the pager exits.

And here's a patch to do that. It seems to work.

I'll sleep on it and then write up a commit message tomorrow if it still
makes sense.

diff --git a/run-command.c b/run-command.c
index ca905a9e80..db47c429b7 100644
--- a/run-command.c
+++ b/run-command.c
@@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
 
 static void cleanup_children(int sig, int in_signal)
 {
+   struct child_to_clean *children_to_wait_for = NULL;
+
while (children_to_clean) {
struct child_to_clean *p = children_to_clean;
children_to_clean = p->next;
@@ -45,6 +47,17 @@ static void cleanup_children(int sig, int in_signal)
}
 
kill(p->pid, sig);
+   p->next = children_to_wait_for;
+   children_to_wait_for = p;
+   }
+
+   while (children_to_wait_for) {
+   struct child_to_clean *p = children_to_wait_for;
+   children_to_wait_for = p->next;
+
+   while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
+   ; /* spin waiting for process exit or error */
+
if (!in_signal)
free(p);
}

Re: Regression: Ctrl-c from the pager in an alias exits it

[Jeff King]

On Fri, Jan 06, 2017 at 01:47:52AM -0500, Jeff King wrote:

> > > To reproduce, create e.g. the alias `l = log` and run `git l`. Then press
> > > Ctrl-c. The expected behavior is that nothing happens. The actual 
> > > behavior is
> > > that the pager exits.
> > 
> > That's weird. I can't reproduce at all here. But I also can't think of a
> > thing that Git could do that would impact the behavior. For example:
> 
> I take it back. I could not reproduce under my normal config, which sets
> the pager to "diff-highlight | less". But if I drop that config, I can
> reproduce easily. And bisect it to that same commit, 86d26f240f
> (setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..,
> 2015-12-20).

I made a little progress with strace. Prior to 86d26f240f, we would run
the "log" builtin directly inside the same executable. After it, we exec
a separate process, and the process tree looks like:

  |   `-bash,10123
  |   `-git,10125 l
  |   `-git-log,10127
  |   `-less,10128

Now if I strace all of those, I see (I've reordered and snipped a bit
for clarity):

  $ strace -p 10125 -p 10127 -p 10128
  strace: Process 10125 attached
  strace: Process 10127 attached
  strace: Process 10128 attached
  [pid 10127] write(1, "\n\33[33mcommit bae73e80d48ace1faa3"..., 1481 

  [pid 10128] read(5,  
  [pid 10125] wait4(10127,  

The main git process is waiting for the child to finish, the child is
blocked writing to the pager, and the pager is waiting for input from
the terminal (fd 5).

So I hit ^C:

  [pid 10128] <... read resumed> 0x7ffd39153b57, 1) = ? ERESTARTSYS (To be 
restarted if SA_RESTART is set)
  [pid 10127] <... write resumed> )   = ? ERESTARTSYS (To be restarted if 
SA_RESTART is set)
  [pid 10125] <... wait4 resumed> 0x7ffe88d0a560, 0, NULL) = ? ERESTARTSYS (To 
be restarted if SA_RESTART is set)
  [pid 10128] --- SIGINT {si_signo=SIGINT, si_code=SI_KERNEL} ---
  [pid 10127] --- SIGINT {si_signo=SIGINT, si_code=SI_KERNEL} ---
  [pid 10125] --- SIGINT {si_signo=SIGINT, si_code=SI_KERNEL} ---

Everybody gets the signal...

  [pid 10127] write(1, "\n\33[33mcommit bae73e80d48ace1faa3"..., 1481 


The blocked writer will resume its write() until it returns. This is the
same as it would get under the older code, too (after write() returns it
will handle the signal and die).

  [pid 10125] kill(10127, SIGINT 
  [pid 10125] <... kill resumed> )= 0

The parent git process tries to propagate the signal to the child.
Unnecessary in this instance, but helpful when the signal is only
delivered to the parent. This is due to 

  [pid 10128] rt_sigaction(SIGINT, {sa_handler=0x558dd1af0300, sa_mask=[INT], 
sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7f03fc8c2040},  
  [pid 10128] <... rt_sigaction resumed> {sa_handler=0x558dd1af0300, 
sa_mask=[INT], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7f03fc8c2040}, 8) 
= 0
  [pid 10128] rt_sigprocmask(SIG_SETMASK, [],  
  [pid 10128] <... rt_sigprocmask resumed> NULL, 8) = 0
  [pid 10128] write(1, "\7\r\33[K:\33[K", 9 
  [pid 10128] <... write resumed> )   = 9
  [pid 10128] read(5,  

And here's the pager handling the signal, and going back to waiting for
terminal input.

  [pid 10125] rt_sigaction(SIGINT, {sa_handler=SIG_DFL, sa_mask=[INT], 
sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7fbe2b4c1040},  
  [pid 10125] <... rt_sigaction resumed> {sa_handler=0x55aec373a6a0, 
sa_mask=[INT], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7fbe2b4c1040}, 8) 
= 0
  [pid 10125] rt_sigprocmask(SIG_BLOCK, ~[RTMIN RT_1], [INT], 8) = 0
  [pid 10125] getpid( 
  [pid 10125] <... getpid resumed> )  = 10125
  [pid 10125] gettid()= 10125
  [pid 10125] tgkill(10125, 10125, SIGINT) = 0
  [pid 10125] rt_sigprocmask(SIG_SETMASK, [INT], NULL, 8) = 0
  [pid 10125] rt_sigreturn({mask=[]}) = 61
  [pid 10125] --- SIGINT {si_signo=SIGINT, si_code=SI_TKILL, si_pid=10125, 
si_uid=1000} ---
  [pid 10125] +++ killed by SIGINT +++

The parent process pops the signal handler and propagates to itself,
dying.

At this point things pause, and nothing happens. But as soon as I hit a
key, the pager dies:

  [pid 10128] <... read resumed> "\r", 1) = 1
  [pid 10128] write(1, "\r\33[K", 4)  = 4
  [pid 10128] write(1, " \"Another round of MIPS fixe"..., 50) = 50
  [pid 10128] read(5, 0x7ffd39153b57, 1)  = -1 EIO (Input/output error)
  [pid 10128] write(1, "\r\33[K\33[?1l\33>", 11) = 11
  [pid 10128] fsync(5)= -1 EINVAL (Invalid argument)
  [pid 10128] ioctl(5, TCGETS, {B38400 opost isig -icanon -echo ...}) = 0
  [pid 10128] ioctl(5, SNDCTL_TMR_STOP or TCSETSW, {B38400 opost isig icanon 
echo ...}) = -1 EIO (Input/output error)
  [pid 10128] exit_group(1)   

Re: Regression: Ctrl-c from the pager in an alias exits it

[Jeff King]

On Fri, Jan 06, 2017 at 01:40:32AM -0500, Jeff King wrote:

> On Thu, Jan 05, 2017 at 03:25:29PM +0100, Trygve Aaberge wrote:
> 
> > I'm experiencing an issue when using aliases for commands that open the 
> > pager.
> > When I press Ctrl-c from the pager, it exits. This does not happen when I
> > don't use an alias and did not happen before. It causes problems because
> > Ctrl-c is also used for other things, such as canceling a search that hasn't
> > completed.
> > 
> > To reproduce, create e.g. the alias `l = log` and run `git l`. Then press
> > Ctrl-c. The expected behavior is that nothing happens. The actual behavior 
> > is
> > that the pager exits.
> 
> That's weird. I can't reproduce at all here. But I also can't think of a
> thing that Git could do that would impact the behavior. For example:

I take it back. I could not reproduce under my normal config, which sets
the pager to "diff-highlight | less". But if I drop that config, I can
reproduce easily. And bisect it to that same commit, 86d26f240f
(setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..,
2015-12-20).

-Peff

Re: Regression: Ctrl-c from the pager in an alias exits it

[Jeff King]

On Thu, Jan 05, 2017 at 03:25:29PM +0100, Trygve Aaberge wrote:

> I'm experiencing an issue when using aliases for commands that open the pager.
> When I press Ctrl-c from the pager, it exits. This does not happen when I
> don't use an alias and did not happen before. It causes problems because
> Ctrl-c is also used for other things, such as canceling a search that hasn't
> completed.
> 
> To reproduce, create e.g. the alias `l = log` and run `git l`. Then press
> Ctrl-c. The expected behavior is that nothing happens. The actual behavior is
> that the pager exits.

That's weird. I can't reproduce at all here. But I also can't think of a
thing that Git could do that would impact the behavior. For example:

  1. Git should never kill() the pager; in fact it waits for it to
 finish.

  2. Git can impact the pager environment and command line options, but
 those haven't changed anytime recently (and besides, I'm not sure
 there even is an option to convince less to die).

  3. Git can impact the set of blocked signals that the pager sees, but
 I think less would set up its own handler for SIGINT anyway.

I suppose it's possible that your shell or another program (tmux,
maybe?) catches the SIGINT and does a process-group kill. But then I
don't know why it would matter that you're using an alias. The process
tree without an alias:

  |-xterm,21861,peff
  |   `-bash,21863
  |   `-git,22376 log
  |   `-less,22377

and with:

  |-xterm,21861,peff
  |   `-bash,21863
  |   `-git,22391 l
  |   `-git-log,22393
  |   `-less,22394

are pretty similar.

Puzzling.

-Peff

Regression: Ctrl-c from the pager in an alias exits it

[Trygve Aaberge]

I'm experiencing an issue when using aliases for commands that open the pager.
When I press Ctrl-c from the pager, it exits. This does not happen when I
don't use an alias and did not happen before. It causes problems because
Ctrl-c is also used for other things, such as canceling a search that hasn't
completed.

To reproduce, create e.g. the alias `l = log` and run `git l`. Then press
Ctrl-c. The expected behavior is that nothing happens. The actual behavior is
that the pager exits.

I bisected the repo, and found that the commit 86d26f240 [0] introduced the
issue.

[0]: 86d26f240 (setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE
when .. - 2015-12-20)

-- 
Trygve Aaberge

Re: Bug: pager. doesn't work well with editors

[Jeff King]

On Thu, Sep 22, 2016 at 10:19:32AM -0700, Junio C Hamano wrote:

> The level at which configurability happens might be one issue
> (i.e. you may want different pager for two operating modes for the
> same command, hence your need to use "tag.list" not just "tag"), but
> I think another issue is that it conflates if the output need to be
> paged (on/off) and what pager should be used when the output is
> paged.  When we see that a user sets "pager.tag", we should not have
> made it an instruction to Git that _all_ output from "git tag" must
> be paged.

Yes, we could have done it the other way, but I think this was a natural
consequence of implementing it git.c. It _only_ knows about "all output
from git-tag" and nothing else.

At any rate, I do not see much point in moving away from it even if we
change the underlying implementation to be more flexible, if only
because it would be a gratuitous incompatibility.

> So I think we are fundamentally on the same page; it is just you are
> aiming higher than I was, but we both recognize the need for separate 
> codepaths in a single command to decide if the output should be paged.

Yeah. In my examples there are really two proposed improvements:

  1. The decision over whether and when to start a pager is pushed down
 from git.c into individual commands.

  2. As a side effect of (1), commands must declare "this is who I am"
 to look up the correct config.  But "who I am" no longer needs to
 be a whole command, so we are free to slice up the namespace more
 finely. But we do not have to.

 This might also be an opportunity to add more conditions. Like "run
 the pager if I am doing log output with -p, but not otherwise" or
 something. I dunno. That does not sound useful to me, but maybe
 somebody else would find it so.

And I think you are getting at a (3), which is something like:

  3. The config namespace can be made richer, so that "whether" and
 "how" are split. E.g., "pager.log.command" and "pager.log.enabled"
 or something.

 I do not mind that, but we would probably want to keep "pager.log"
 for compatibility, at which point I wonder if the new system is
 worth the bother.

-Peff

Re: Bug: pager. doesn't work well with editors

[Junio C Hamano]

Jeff King <p...@peff.net> writes:

> I don't think it is a bad move overall. I use "pager.log" to pipe
> through a specific command (that is different than I would use for other
> commands).
>
> So I like the idea of configurability; the problem is just that it is
> happening at the wrong level.

The level at which configurability happens might be one issue
(i.e. you may want different pager for two operating modes for the
same command, hence your need to use "tag.list" not just "tag"), but
I think another issue is that it conflates if the output need to be
paged (on/off) and what pager should be used when the output is
paged.  When we see that a user sets "pager.tag", we should not have
made it an instruction to Git that _all_ output from "git tag" must
be paged.

If there were no need for supporting separate pagers per operating
mode of a Git command, say "git tag", you would not want to page the
output unless you are producing "git tag [-l]" listing.  You do not
want your interaction with the usual "git tag  []"
to be paged, even if you want to use a pager different from GIT_PAGER
when you are viewing the tags.

It is good that each codepath can give default in this example

> The individual commands should be in
> charge of it, with something like:
>
>   setup_auto_pager("log", 1);
> ...
>   if (mode_list)
> setup_auto_pager("tag.list", 0);

as the second parameter to setup_auto_pager(), but I think the first
parameter being "tag.list" vs "tag" is a separate issue.  Until
there comes another codepath in "git tag" that wants to call
setup_auto_pager(), it does not make any difference from the end-user's
point of view.  Starting with "tag.list" may futureproof it
(e.g. perhaps somebody wants to use a separate pager for "git tag --help"
and "tag.help" can be added without disrupting existing use of "tag.list")

So I think we are fundamentally on the same page; it is just you are
aiming higher than I was, but we both recognize the need for separate 
codepaths in a single command to decide if the output should be paged.

> I don't have a particular plan to work on it anytime soon, but maybe
> somebody could pick it up as relatively low-hanging fruit.

;-)