On 11 July 2017 at 12:19, Jeff King <peff@xxxxxxxx> 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
