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
