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

From: Jeff King <peff@peff.net>
Date: Jul 11, 2017, 10:19 UTC
Message-ID: <20170711101942.h2uwxtgzvgguzivu@sigill.intra.peff.net>
In-Reply-To: <cover.1499723297.git.martin.agren@gmail.com>

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

Show 6 quoted lines

> 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.

Show 7 quoted lines

> 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.

Show 5 quoted lines

> 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.

Show 9 quoted lines

> 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