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

On 11 July 2017 at 12:19, Jeff King [off-list ref] wrote:

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

quoted

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.

quoted

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