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

[Martin Ågren]

On 11 July 2017 at 00:42, Junio C Hamano  wrote:
> Martin Ågren  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  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 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  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 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