patch, 7 partstag: more fine-grained pager-configuration

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(-)

Commit 74866d75 ("git: make super-prefix option", 2016-10-07) introduced SUPPORT_SUPER_PREFIX as a builtin flag without documenting it in api-builtin.txt. The next patch will add another flag, so document SUPPORT_SUPER_PREFIX, thereby bringing the documentation up to date with the available flags.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/technical/api-builtin.txt | 4 ++++
 1 file changed, 4 insertions(+)

On 07/10, Martin Ågren wrote:

I added SUPER_PREFIX as an implementation detail when trying to recurse submodules using multiple processes. Now that we have a 'struct repository' my plan is to remove SUPER_PREFIX in its entirety. Since this won't happen overnight it may still take a bit till its removed so maybe it makes sense to better document it until that happens?

Either way I think that this sort of Documention better lives in the code as it is easier to keep up to date. Last time I tried to look at stuff in Documentation/technical the files were either place holders or completely out of date with what the code did.

On 11 July 2017 at 00:50, Brandon Williams <bmwill@google.com> wrote:

I could add something about how this is "temporary" although I have no idea about the timeframe. ;-)

Agreed and I believe that's the long-term goal. I could replace this preparatory patch with another one where I move api-builtin.txt into builtin.h or git.c (and document SUPPORT_SUPER_PREFIX if that's wanted). That's probably better, since right now, patch 2/7 adds basically the same documentation for the new flag in two places.

Martin

On 07/11, Martin Ågren wrote:

Its probably better to ensure that things are documented (even if they are temporary) but you don't need to mark it as such. And as far as timeframe, that's my burden to bare ;)

I know that sort of migration may be out of the scope of your series (since its just documentation), though you're more than welcome to do the work as it would be much appreciated by me and a few other people :)

Before launching a builtin git foo and unless mechanisms with precedence are in use, we check for and handle the `pager.foo` config. This is done without considering exactly how git foo is being used, and indeed, git.c cannot (and should not) know what the arguments to git foo are supposed to achieve.

In practice this means that, 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`.

To allow individual builtins to make more informed decisions about when to respect `pager.foo`, introduce a flag IGNORE_PAGER_CONFIG. If the flag is set, do not check `pager.foo`. This applies to two code-paths -- one in run_builtin() and one in execv_dashed_external().

Document the flag in api-builtin.txt.

Don't add any users of IGNORE_PAGER_CONFIG just yet, one will follow in a later patch.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/technical/api-builtin.txt |  6 ++++++
 git.c                                   | 14 ++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

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

Can this ever trigger in execv_dashed_external()? We should only get there if get_builtin() returned NULL in the first place. Otherwise, we'd run and exited via handle_builtin().

So I think this hunk:

...can just go away. And that highlights the issue with externals; we don't have any internal database that says "these ones handle their own pager". We don't even have a list of all the possibilities, since users can drop whatever they like into their $PATH.

So we'd have to make a (backwards-incompatible) decision that pager.* doesn't work for external commands, and they must manually trigger the pager themselves. I'm undecided on whether that's reasonable or not (certainly it's what git-stash would want, but it may be hurting other commands).

Anyway, I think that's out of scope for your series, which is just trying to make the builtins work better.

-Peff

On 11 July 2017 at 12:24, Jeff King <peff@peff.net> wrote:

I can trigger it with this:

$ git -c pager.tag="echo paging" -c pager.tag.list=no -c alias.t=tag t -l

where the alias is what triggers it and the two pager-configurations demonstrate the effect.

If I remove this, the call I gave above will page although it shouldn't, and it doesn't if I keep this hunk. There's this in run_argv: "If we tried alias and futzed with our environment, it no longer is safe to invoke builtins directly in general. We have to spawn them as dashed externals." There's also a NEEDSWORK.

Although, thinking about it, I'm not sure why when I remove this hunk, the child process doesn't set up the paging correctly. Maybe something related to my using "-c", or something about the launching of child processes. Those are both areas where I lack knowledge. Will look into it.

Martin

On Tue, Jul 11, 2017 at 03:46:08PM +0200, Martin Ågren wrote:

Interesting. As you noted below, I think the dashed external we exec should be choosing whether to run the pager. I suspect what's happening is that execv_dashed_external() says "a-ha, we're running 'tag', and I know how to check its pager config". But IMHO that is wrong in the first place (but it just never really made a difference until your series).

That's just a guess, though. I didn't dig.

-Peff

The previous patch introduced a way for builtins to declare that they will take responsibility for handling the `pager.foo`-config item. (See the commit message of that patch for why that could be useful.)

Provide setup_auto_pager(), which builtins can call in order to handle `pager.<cmd>`, including possibly starting the pager. Make this function don't do anything if a pager has already been started.

When the `cmd` given to setup_auto_pager() contains one or more '.', use a fallback strategy which checks `pager.foo.bar.baz`, then `pager.foo.bar`, then `pager.foo`, then resorts to the provided default value. This ensures a consistent fallback strategy for builtins which take this type of more fine-grained pager configuration.

An alternative fallback strategy would have been to check for `pager.foo.bar.baz`, then immediately fall back to `def`. However, that would have meant that git foo would sometimes completely ignore `pager.foo`, which seems conceptually wrong. It would also have meant that builtins that are moved to more fine-grained pager configurations would have regressed for certain usecases.

Whenever this function is called from a builtin, git.c will already have called commit_pager_choice(). Since commit_pager_choice() treats the special value -1 as "punt" or "not yet decided", it is not a problem that we might end up calling commit_pager_choice() many times. Make the new function use -1 in the same way and document it as "punt".

Don't add any users of setup_auto_pager just yet, one will follow in later patches.

setup_auto_pager() is more capable than it needs to be for this patch series. It would have been sufficient to handle zero or one '.'. We would probably have wanted some verification+BUG()-patterns or to define whether we split at the first or last '.', so it seems just as easy, and certainly more flexible, to handle the more general situation.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin.h | 14 ++++++++++++++
 git.c     | 28 ++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

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

I think you probably also want to return early here if pager_in_use() is true. If a script runs "git tag -l", you wouldn't want to start a new pager when the outer script has already been paged (either via config, or via "git -p").

This looks good. I wondered if we could fold this all into a single loop, rather than having the extra check_pager_config() beforehand (which confused me for a half-second at first). But I tried and I don't think the result ended up any more readable.

-Peff

On 11 July 2017 at 12:37, Jeff King <peff@peff.net> wrote:

Good point. Thanks.

Martin

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

Since we're about to add some finer granularity to the configuration, add tests around how git tag respects `pager.tag` and how that configuration is ignored if --no-pager or --paginate are used.

Construct tests with two different subcommands: using -l and using -a, where -a is being used essentially as a representative for "not -l".

Make one of the tests demonstrate the behavior mentioned above, where `git tag -a` respects `pager.tag`. Actually, the tests use `git tag -a` with -m, in which case no editor is launched, but that is irrelevant, since we just want to see whether the pager is used or not. (If `git tag -am` ever learns to avoid the pager, these tests will need to be updated and two of them will fail.)

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t7006-pager.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

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

Introduce `pager.tag.list`. Teach git tag to prefer it over `pager.tag` when running with -l. Update the documentation and add tests. Update an existing test to use `pager.tag.list` instead of `pager.tag` together with `git tag -l` since the former is arguably more relevant.

Do not introduce an "else" where we call setup_auto_pager(), although we could have. Omitting it might keep someone who later implements even more fine-grained configurations from building a correspondingly complicated decision tree which carefully ensures that setup_auto_pager() is called precisely once. A greedy approach such as the one taken here works just fine.

Noticed-by: Anatoly Borodin <anatoly.borodin@gmail.com>
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-tag.txt |  4 ++++
 builtin/tag.c             |  2 ++
 t/t7006-pager.sh          | 16 +++++++++++++++-
 3 files changed, 21 insertions(+), 1 deletion(-)

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

Ideally this would kick in whenever we are in list mode, even if the user didn't say "-l". So:

  $ git tag

should probably respect the list config. Likewise, certain options like "--contains" trigger list mode. I think the pager setup could just be bumped a few lines down, past the "if (!cmdmode)" block that sets up those defaults.

-Peff

On 11 July 2017 at 12:41, Jeff King <peff@peff.net> wrote:

Oops, that's embarassing. Thanks. That means the paging starts slightly later, but what happens in those few lines looks fairly trivial, so there shouldn't be any real difference in behavior. I'll add one or two tests.

Martin

Use the mechanisms introduced in two earlier patches to ignore `pager.tag` in git.c and let the `git tag` builtin handle it on its own.

This is in preparation for the next patch, where we will want to handle slightly different configuration variables depending on which options are used with `git tag`. For this reason, place the call to setup_auto_pager() after the options have been parsed.

No functional change is intended. That said, there is a window between where the pager is started before and after this patch, and if an error occurs within this window, as of this patch the error message might not be paged where it would have been paged before. Since operation-parsing has to happen inside this window, a difference can be seen with, e.g., `git -c pager.tag="echo pager is used" tag --unknown-option`. This change in paging-behavior should be acceptable since it only affects erroneous usages.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin/tag.c | 2 ++
 git.c         | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

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

Thanks for carefully thinking through the details. I agree that it's an acceptable change of behavior.

-Peff

The previous patch introduced `pager.tag.list`. Its default was to use the value of `pager.tag` or, if that was also not set, to fall back to "off".

Change that fallback value to "on". Let the default value for `pager.tag` remain at "off". Update documentation and tests.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-tag.txt |  2 +-
 builtin/tag.c             |  2 +-
 t/t7006-pager.sh          | 12 ++++++------
 3 files changed, 8 insertions(+), 8 deletions(-)

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

Thanks for splitting this out. The default setting is definitely orthogonal to allowing the config.

I've been running with this default for several years now (using the patch I showed in the earlier thread you linked). It _does_ occasionally annoy me, but I think overall it's an improvement. So it seems like a good thing to try, and we can see how people respond as they try it out.

-Peff

Martin Ågren <martin.agren@gmail.com> writes:

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.

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.

On Mon, Jul 10, 2017 at 03:42:14PM -0700, Junio C Hamano wrote:

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

Jeff King <peff@peff.net> writes:

Perhaps not "tag output", but the paging is the area you are the person who spent most time on.

Yes.

On 11 July 2017 at 00:42, Junio C Hamano <gitster@pobox.com> wrote:

...

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

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

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.

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

I think I covered my thoughts on this part above. :)

Overall the code looks good, though I'll respond with a few comments.

-Peff

On 11 July 2017 at 12:19, Jeff King <peff@peff.net> wrote:

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.

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.

Good thinking.

Thanks a lot for your comments. I appreciate it.

Martin

On Tue, Jul 11, 2017 at 03:50:39PM +0200, Martin Ågren wrote:

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

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.

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

This is the second version of "[PATCH 0/7] tag: more fine-grained pager-configuration" [1]. That series introduced `pager.tag.list` to address the fact that `pager.tag` can be useful with `git tag -l` but actively hostile with `git tag -a`. Thanks to Junio, Peff and Brandon for helpful feedback.

After that feedback, v2 drops `pager.tag.list` and instead teaches `git tag` to only consider `pager.tag` in list-mode, as suggested by Peff.

Patches 1-3/10 replace patch 1/7. They move Documentation/technical/ api-builtin.txt into builtin.h, tweak the formatting and bring it up to date. I may have gone overboard making this 3 patches...

Patches 4-7/10 correspond to patches 2-5/7. `setup_auto_pager()' is now much simpler since we do not need to handle "tag.list" with a clever fallback strategy. IGNORE_PAGER_CONFIG is now called DELAY_PAGER_CONFIG. I now check with pager_in_use() and I moved the handling of `pager.tag` a bit further down.

Patches 8-9/10 teach `git tag` to only respect `pager.tag` in list-mode and flip the default value for that config to "on".

Patch 10/10 is somewhat similar to a hunk in patch 2/7, but is now a bug-fix instead of a feature. It teaches `execv_dashed_external()` not to check `pager.foo` when launching `git-foo` where foo is a builtin. I waffled about where to put this patch. Putting it earlier in the series as a preparatory step, I couldn't come up with a way of writing a test. So patch 8/10 introduces a `test_expect_failure` which this patch then fixes.

Martin

[1] https://public-inbox.org/git/cover.1499723297.git.martin.agren@gmail.com/T/

Martin Ågren (10):
  builtin.h: take over documentation from api-builtin.txt
  builtin.h: format documentation-comment properly
  builtin.h: 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: respect `pager.tag` in list-mode only
  tag: change default of `pager.tag` to "on"
  git.c: ignore pager.* when launching builtin as dashed external

 Documentation/git-tag.txt               |  3 +
 Documentation/technical/api-builtin.txt | 73 -------------------------
 t/t7006-pager.sh                        | 85 +++++++++++++++++++++++++++++
 builtin.h                               | 97 +++++++++++++++++++++++++++++++++
 builtin/tag.c                           |  3 +
 git.c                                   | 18 +++++-
 6 files changed, 203 insertions(+), 76 deletions(-)
 delete mode 100644 Documentation/technical/api-builtin.txt

Delete Documentation/technical/api-builtin.txt and move its content verbatim into builtin.h. Just wrap it in /* ... */. In order to make the move obviously correct, do not change any formatting, not even to format the comment into Git's preferred style. That will be done in a follow-up patch.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/technical/api-builtin.txt | 73 -------------------------------
 builtin.h                               | 76 +++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 73 deletions(-)
 delete mode 100644 Documentation/technical/api-builtin.txt

The previous commit moved technical documentation verbatim into builtin.h. Prefix all the lines with '*' to follow the coding guidelines.

Remove a '+' which was needed when the information was formatted for AsciiDoc. Similarly, change "::" to ":".

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin.h | 146 +++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 73 insertions(+), 73 deletions(-)

Commit 74866d75 ("git: make super-prefix option", 2016-10-07) introduced SUPPORT_SUPER_PREFIX as a builtin flag without documenting it. The next patch will add another flag, so document SUPPORT_SUPER_PREFIX, thereby bringing the documentation up to date with the available flags.

While at it, correct '3 more things to do' to '4 more things to do'.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Before launching a builtin git foo and unless mechanisms with precedence are in use, we check for and handle the `pager.foo` config. This is done without considering exactly how git foo is being used, and indeed, git.c cannot (and should not) know what the arguments to git foo are supposed to achieve.

In practice this means that, 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`.

To allow individual builtins to make more informed decisions about when to respect `pager.foo`, introduce a flag DELAY_PAGER_CONFIG. If the flag is set, do not check `pager.foo`.

Do not check for DELAY_PAGER_CONFIG in `execv_dashed_external()`. That call site is arguably wrong, although in a way that is not yet visible, and will be changed in a slightly different direction in a later patch.

Don't add any users of DELAY_PAGER_CONFIG just yet, one will follow in a later patch.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin.h | 8 ++++++++
 git.c     | 4 +++-
 2 files changed, 11 insertions(+), 1 deletion(-)

The previous patch introduced a way for builtins to declare that they will take responsibility for handling the `pager.foo`-config item. (See the commit message of that patch for why that could be useful.)

Provide setup_auto_pager(), which builtins can call in order to handle `pager.<cmd>`, including possibly starting the pager. Make this function don't do anything if a pager has already been started, as indicated by use_pager or pager_in_use().

Whenever this function is called from a builtin, git.c will already have called commit_pager_choice(). Since commit_pager_choice() treats the special value -1 as "punt" or "not yet decided", it is not a problem that we might end up calling commit_pager_choice() once in git.c and once (or more) in the builtin. Make the new function use -1 in the same way and document it as "punt".

Don't add any users of setup_auto_pager just yet, one will follow in a later patch.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin.h |  9 +++++++++
 git.c     | 10 ++++++++++
 2 files changed, 19 insertions(+)

On Mon, Jul 17, 2017 at 10:10:47PM +0200, Martin Ågren wrote:

At first I wasn't sure if it would ever make sense to use "-1" here. The "punt" that happens in earlier calls to commit_pager_choice() is there because we might adjust our decision later. And this would generally be the final decision, I would think. So I'd be surprised if we had anything besides "0" or "1" in the "def" argument.

But thinking on it, the most plausible case is something like:

  setup_auto_pager("foo", -1);
  ...
  /* fallback to some historical compatibility name */
  setup_auto_pager("bar", 0);

And it's important for the "-1" there to be a true punt, and not do anything in commit_pager_choice(). So it's probably worth documenting the "-1" behavior as you did here as a possible value for "def".

-Peff

Jeff King <peff@peff.net> writes:

Thanks for reading it over. I agree that the "punt" behaviour is a sensible one in that use case, and I also agree that it would be good to explain it.

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

Since we're about to change how `git tag` respects `pager.tag`, add tests around this, including how the configuration is ignored if --no-pager or --paginate are used.

Construct tests with a few different subcommands. First, use -l. Second, use "no arguments" and --contains, since those imply -l. (There are more arguments which imply -l, but using these two should be enough.) Third, use -a as a representative for "not -l".

Make one of the tests demonstrate the behavior mentioned above, where `git tag -a` respects `pager.tag`. Actually, the tests use `git tag -am` so no editor is launched, but that is irrelevant, since we just want to see whether the pager is used or not.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t7006-pager.sh | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

On Mon, Jul 17, 2017 at 10:10:48PM +0200, Martin Ågren wrote:

I don't mind an expect_success like this, where it documents a sane and existing default, even if we're going to flip that default later in the series.

But here...

I think this behavior is just buggy, and it might be better introduced as a test_expect_failure on "git tag -a does not respect pager.tag".

Kind of a minor nit, as the series should end up in the right place either way, but it can be helpful if you end up digging back in history to the introduction of the test.

-Peff

Jeff King <peff@peff.net> writes:

Yes, I think that is essentially the same reaction I had to patches 7 and 8, where it carries the "buggy" behaviour forward and then fixes it. The way the series lays groundwork to introduce a mechanism that can be used to address this behaviour in its earlier patches strongly suggests to the users that this is considered a bug by the author of the series to the user from early on, so adding this as "expect failure" and then flip it to "expect success" when the bug is fixed would be a more natural sequence of changes.

Thanks.

On 31 July 2017 at 18:37, Junio C Hamano <gitster@pobox.com> wrote:

Thanks both for very helpful comments. I admit I viewed it less as "fix buggy behavior" and more like "redefine wanted behavior". So I wanted to postpone the redefinition of the behavior until all the restructuring was done. Looking at this as a bug-fix does make carefully moving the bug forward look rather silly.

I haven't responded to each of your suggestions individually where the answers would have been a mere "thanks, will do". They're still much appreciated and will help make v3 much better. Thanks.

Martin

Use the mechanisms introduced in two earlier patches to ignore `pager.tag` in git.c and let the `git tag` builtin handle it on its own.

This is in preparation for the next patch, where we will want to ignore `pager.tag` unless we run in list-mode. For this reason, place the call to setup_auto_pager() after the options have been parsed and we have decided whether we are in list-mode.

No functional change is intended. That said, there is a window between where the pager is started before and after this patch, and if an error occurs within this window, as of this patch the error message might not be paged where it would have been paged before. Since operation-parsing has to happen inside this window, a difference can be seen with, e.g., `git -c pager.tag="echo pager is used" tag --unknown-option`. This change in paging-behavior should be acceptable since it only affects erroneous usages.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin/tag.c | 2 ++
 git.c         | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

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

Teach git tag to only respect `pager.tag` when running in list-mode. Update the documentation and update tests.

If an alias is used to run `git tag -a`, then `pager.tag` will still be respected. Document this known breakage. It will be fixed in a later commit. Add a similar test for `-l`, which works as it should.

Noticed-by: Anatoly Borodin <anatoly.borodin@gmail.com>
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-tag.txt |  3 +++
 t/t7006-pager.sh          | 25 +++++++++++++++++++++----
 builtin/tag.c             |  3 ++-
 3 files changed, 26 insertions(+), 5 deletions(-)

On Mon, Jul 17, 2017 at 10:10:50PM +0200, Martin Ågren wrote:

This changes, I guess, because pager.tag should not be having any impact at all. So it would not hurt to leave it, but the in the final state of the test what is interesting is that "--paginate" kicks in. I do wonder if it could just drop the pager.tag bit when it is added in the first place. We test elsewhere that the command-line overrides the config (for a case that actually _does_ respect the config).

I'm OK with it either way, though.

-Peff

The previous patch taught `git tag` to only respect `pager.tag` in list-mode. That patch left the default value of `pager.tag` at "off".

After that patch, it makes sense to let the default value be "on" instead, since it will help with listing many tags, but will not hurt users of `git tag -a` as it would have before. Make that change. Update documentation and tests.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-tag.txt |  2 +-
 t/t7006-pager.sh          | 28 ++++++++++++++--------------
 builtin/tag.c             |  2 +-
 3 files changed, 16 insertions(+), 16 deletions(-)

When running, e.g., `git -c alias.bar=foo bar`, we expand the alias and execute `git-foo` as a dashed external. This is true even if git foo is a builtin. That is on purpose, and is motivated in a comment which was added in commit 441981bc ("git: simplify environment save/restore logic", 2016-01-26).

Shortly before we launch a dashed external, and unless we have already found out whether we should use a pager, we check `pager.foo`. This was added in commit 92058e4d ("support pager.* for external commands", 2011-08-18). If the dashed external is a builtin, this does not match that commit's intention and is arguably wrong, since it would be cleaner if we let the "dashed external builtin" handle `pager.foo`.

This has not mattered in practice, but a recent patch taught `git-tag` to ignore `pager.tag` under certain circumstances. But, when started using an alias, it doesn't get the chance to do so, as outlined above. That recent patch added a test to document this breakage.

Do not check `pager.foo` before launching a builtin as a dashed external, i.e., if we recognize the name of the external as a builtin. Change the test to use `test_expect_success`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
One could address this in run_argv(), by making the second call to
execv_dashed_external() conditional on "!is_builtin()" whereas a builtin
would be started as "git foo". (Possibly after unrolling and cleaning up
the "while (1)"-loop.) That seems like the wrong fix for this particular
issue, but might be a wanted change on its own -- or maybe not --, since
it would mean one could relay, e.g., "-c baz" to "git -c baz foo" (but
only for builtins...).

 t/t7006-pager.sh | 2 +-
 git.c            | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

On Mon, Jul 17, 2017 at 10:10:52PM +0200, Martin Ågren wrote:

I think floating this change to the end like this has made it much easier to see why we must do it. The end result looks good to me.

We shouldn't need to relay them. They get added to the environment by the initial "git" invocation, and then are available everywhere (in fact, it would be wrong to relay them for multi-valued config). Or did you mean that we could potentially allow:

  [alias]
  foo = "-c baz some-builtin"

That's interesting, but I think the fact that it only works with builtins makes it a bad idea. And you can always do:

  [alias]
  foo = "!git -c baz some-builtin"

which is equivalent.

-Peff

On 31 July 2017 at 05:45, Jeff King <peff@peff.net> wrote:

Thanks for explaining. I did some very sloppy reading of the comment in git.c that we "cannot take flags in between the 'git' and the 'xxxx'" which I somehow misunderstood completely as "we cannot pass that sort of information to git-xxxx". Silly. Thanks for taking the time to explain what I should have found out myself...

So yeah, I meant the above and not this:

Martin Ågren <martin.agren@gmail.com> writes:

That does sound like a more sensible and safer approach. I may have comments on individual patches, which I will send while I read them.

Thanks.

Martin Ågren <martin.agren@gmail.com> writes:

I tend to agree with you that 1-3/10 may be better off being a single patch (or 3/10 dropped, as Brandon is working on losing it nearby). I would have expected 7-8/10 to be a single patch, as by the time a reader reaches 07/10, because of the groundwork laid by 04-06/10, it is obvious that the general direction is to allow the caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in some but not all circumstances, and 07/10 being faithful to the original behaviour (only to be updated in 08/10) is somewhat counter intuitive. It is not wrong per-se; it was just unexpected.

I haven't thought about ramifications of 9-10/10 to make a comment yet, but overall the series was a pleasant read.

Thanks.

On 21 July 2017 at 00:27, Junio C Hamano <gitster@pobox.com> wrote:

Thanks for your comments. I will be away for a few days, but once I get back, I'll try to produce a v3 based on this and any further feedback.

Martin

On Sun, Jul 23, 2017 at 08:17:42PM +0200, Martin Ågren wrote:

Overall it looks good to me. I left a few minor comments.

I actually like the split. I found it pretty easy to follow (though squashing as Junio suggested would be fine with me, too).

Thanks again for working on this.

-Peff

On 31 July 2017 at 05:46, Jeff King <peff@peff.net> wrote:

I assume that by "the split" you mean patches 7-8, not 1-3. Anyway, I'll squash since you're fine with it and Junio prefers it.

Martin

On Mon, Jul 31, 2017 at 07:44:27PM +0200, Martin Ågren wrote:

I actually meant both, but as I said, I'm OK with it either way.

-Peff

Jeff King <peff@peff.net> writes:

I am OK with it either way, too ;-)

This is the third version of my attempt to make `pager tag` useful (v1 at [1], v2 at [2]). Thanks to Junio and Peff for comments on v2.

I've squashed patches 01-03/10 and 07-08/10, respectively. The interdiff is below. I managed to clean up some tests thanks to a drive-by comment by Peff which cleared up a misunderstanding I had. Some minor changes, e.g., I write "built-in" instead of "builtin", since that seemed preferred, at least locally in builtin.h. I documented why a default value of "punt" could be useful, but also that it's probably not wanted.

Martin

[1] https://public-inbox.org/git/cover.1499723297.git.martin.agren@gmail.com/T/

[2] https://public-inbox.org/git/cover.1500321657.git.martin.agren@gmail.com/T/#u

Martin Ågren (7):
  builtin.h: take over documentation from api-builtin.txt
  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: respect `pager.tag` in list-mode only
  tag: change default of `pager.tag` to "on"
  git.c: ignore pager.* when launching builtin as dashed external

 Documentation/git-tag.txt               |   3 +
 Documentation/technical/api-builtin.txt |  73 -----------------------
 t/t7006-pager.sh                        |  80 +++++++++++++++++++++++++
 builtin.h                               | 100 ++++++++++++++++++++++++++++++++
 builtin/tag.c                           |   3 +
 git.c                                   |  18 +++++-
 6 files changed, 201 insertions(+), 76 deletions(-)
 delete mode 100644 Documentation/technical/api-builtin.txt

Interdiff vs v2:

Delete Documentation/technical/api-builtin.txt and move its content into builtin.h. Format it as a comment. Remove a '+' which was needed when the information was formatted for AsciiDoc. Similarly, change "::" to ":".

Document SUPPORT_SUPER_PREFIX, thereby bringing the documentation up to date with the available flags.

While at it, correct '3 more things to do' to '4 more things to do'.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
I'm still documenting SUPPORT_SUPER_PREFIX. As Junio pointed out, a
nearby patch series is working on losing it (or one user of it). I tried
removing that part, but felt like I was leaving things incomplete.

 Documentation/technical/api-builtin.txt | 73 ------------------------------
 builtin.h                               | 80 +++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 73 deletions(-)
 delete mode 100644 Documentation/technical/api-builtin.txt

Martin Ågren <martin.agren@gmail.com> writes:

Not a new problem but it will become much easier to follow if we moved this item between the "implement cmd_foo()" and "declare cmd_foo in builtin.h", like so:

 . Define the implementation of the built-in command `foo` with
   signature:

	int cmd_foo(int argc, const char **argv, const char *prefix);

   in a new file `builtin/foo.c`.

 . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.

Alternatively, we may merge these two into one item (i.e. "in a new file `builtin/foo.c` and add `builtin/foo.o` to ...").

But of course, this patch 1/7 should not do any of the above. I am suggesting a possible future clean-up for anybody on the list listening from sidelines, and you do not have to be the person who does it.

Thanks.

On 3 August 2017 at 19:44, Junio C Hamano <gitster@pobox.com> wrote:

Thank you. If this series needs to be rerolled, I could do it as patch 2. And if not, I could try to remember to do it once this series has landed. A that point in time, I'd also like to try changing other commands ("git branch") similar to "git tag" (although maybe your suggestion above shouldn't be part of that series, but go on its own).

Since this is my first code contribution to Git, I'll ask about this part of SubmittingPatches:

"After the list reached a consensus that it is a good idea to apply the patch, re-send it with "To:" set to the maintainer [*1*] and "cc:" the list [*2*] for inclusion."

I will boldly assume that I should not be doing this. It seems to me this doesn't happen very often or not at all -- possibly because you tend to be involved in virtually all threads anyway, before the list reaches a consensus.

Which brings me to my final point: Thanks for your very helpful feedback throughout all three versions and even more thanks for your work on Git. I find it amazing how much time you are able to constantly spend on all aspects -- "high and low" -- of Git. It goes a long way to explaining how Git can be so very useful. Thanks a lot!

Martin

Martin Ågren <martin.agren@gmail.com> writes:

Yeah, that is in the "ideal patch flow" section, isn't it? We rarely achieve the "ideal" and often instead go for a more expedited option, it appears---perhaps I should try to be less involved in individual patch reviews and place more review burden on other reviewers ;-)

In any case, it was a pleasure to cheer-lead on the progress of this series. Thanks.

On 4 August 2017 at 18:00, Junio C Hamano <gitster@pobox.com> wrote:

Yes and no. It's in the main part above it, under "(4) Sending your patches." But you are right that the ideal patch flow section then says the same thing: "(4) The list forms consensus that the last round of your patch is good. Send it to the maintainer and cc the list."

:-)

Before launching a builtin git foo and unless mechanisms with precedence are in use, we check for and handle the `pager.foo` config. This is done without considering exactly how git foo is being used, and indeed, git.c cannot (and should not) know what the arguments to git foo are supposed to achieve.

In practice this means that, 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. Someone who makes use of both `git tag -a` and `git tag -l` will probably not set `pager.tag`, so that `git tag -a` will actually work, at the cost of not paging output of `git tag -l`.

To allow individual builtins to make more informed decisions about when to respect `pager.foo`, introduce a flag DELAY_PAGER_CONFIG. If the flag is set, do not check `pager.foo`.

Do not check for DELAY_PAGER_CONFIG in `execv_dashed_external()`. That call site is arguably wrong, although in a way that is not yet visible, and will be changed in a slightly different direction in a later patch.

Don't add any users of DELAY_PAGER_CONFIG just yet, one will follow in a later patch.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin.h | 8 ++++++++
 git.c     | 4 +++-
 2 files changed, 11 insertions(+), 1 deletion(-)

The previous patch introduced a way for builtins to declare that they will take responsibility for handling the `pager.foo`-config item. (See the commit message of that patch for why that could be useful.)

Provide setup_auto_pager(), which builtins can call in order to handle `pager.<cmd>`, including possibly starting the pager. Make this function don't do anything if a pager has already been started, as indicated by use_pager or pager_in_use().

Whenever this function is called from a builtin, git.c will already have called commit_pager_choice(). Since commit_pager_choice() treats the special value -1 as "punt" or "not yet decided", it is not a problem that we might end up calling commit_pager_choice() once in git.c and once (or more) in the builtin. Make the new function use -1 in the same way and document it as "punt".

Don't add any users of setup_auto_pager just yet, one will follow in a later patch.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin.h | 12 ++++++++++++
 git.c     | 10 ++++++++++
 2 files changed, 22 insertions(+)

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. Someone who makes use of both `git tag -a` and `git tag -l` will probably not set `pager.tag`, so that `git tag -a` will actually work, at the cost of not paging output of `git tag -l`.

Since we're about to change how `git tag` respects `pager.tag`, add tests around this, including how the configuration is ignored if --no-pager or --paginate are used.

Construct tests with a few different subcommands. First, use -l. Second, use "no arguments" and --contains, since those imply -l. (There are more arguments which imply -l, but using these two should be enough.)

Third, use -a as a representative for "not -l". Actually, the tests use `git tag -am` so no editor is launched, but that is irrelevant, since we just want to see whether the pager is used or not. Make one of the tests demonstrate the broken behavior mentioned above, where `git tag -a` respects `pager.tag`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
In one place, I now use test_expect_failure (thanks Peff).

 t/t7006-pager.sh | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

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. Someone who makes use of both `git tag -a` and `git tag -l` will probably not set `pager.tag`, so that `git tag -a` will actually work, at the cost of not paging output of `git tag -l`.

Use the mechanisms introduced in two earlier patches to ignore `pager.tag` in git.c and let the `git tag` builtin handle it on its own. Only respect `pager.tag` when running in list-mode.

There is a window between where the pager is started before and after this patch. This means that early errors can behave slightly different before and after this patch. Since operation-parsing has to happen inside this window, this can be seen with `git -c pager.tag="echo pager is used" tag -l --unknown-option`. This change in paging-behavior should be acceptable since it only affects erroneous usages.

Update the documentation and update tests.

If an alias is used to run `git tag -a`, then `pager.tag` will still be respected. Document this known breakage. It will be fixed in a later commit. Add a similar test for `-l`, which works.

Noticed-by: Anatoly Borodin <anatoly.borodin@gmail.com>
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-tag.txt |  3 +++
 t/t7006-pager.sh          | 15 ++++++++++++++-
 builtin/tag.c             |  3 +++
 git.c                     |  2 +-
 4 files changed, 21 insertions(+), 2 deletions(-)

The previous patch taught `git tag` to only respect `pager.tag` in list-mode. That patch left the default value of `pager.tag` at "off".

After that patch, it makes sense to let the default value be "on" instead, since it will help with listing many tags, but will not hurt users of `git tag -a` as it would have before. Make that change. Update documentation and tests.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-tag.txt |  2 +-
 t/t7006-pager.sh          | 28 ++++++++++++++--------------
 builtin/tag.c             |  2 +-
 3 files changed, 16 insertions(+), 16 deletions(-)

When running, e.g., `git -c alias.bar=foo bar`, we expand the alias and execute `git-foo` as a dashed external. This is true even if git foo is a builtin. That is on purpose, and is motivated in a comment which was added in commit 441981bc ("git: simplify environment save/restore logic", 2016-01-26).

Shortly before we launch a dashed external, and unless we have already found out whether we should use a pager, we check `pager.foo`. This was added in commit 92058e4d ("support pager.* for external commands", 2011-08-18). If the dashed external is a builtin, this does not match that commit's intention and is arguably wrong, since it would be cleaner if we let the "dashed external builtin" handle `pager.foo`.

This has not mattered in practice, but a recent patch taught `git-tag` to ignore `pager.tag` under certain circumstances. But, when started using an alias, it doesn't get the chance to do so, as outlined above. That recent patch added a test to document this breakage.

Do not check `pager.foo` before launching a builtin as a dashed external, i.e., if we recognize the name of the external as a builtin. Change the test to use `test_expect_success`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t7006-pager.sh | 2 +-
 git.c            | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Martin Ågren <martin.agren@gmail.com> writes:

Thanks for working on this. The whole series was well reasoned and was a very pleasant read.

Will queue.

On Wed, Aug 02, 2017 at 09:40:48PM +0200, Martin Ågren wrote:

This looks good to me overall. One minor question from the interdiff:

These should behave the same, right? So this is just a style/consistency fix, not a bugfix?

-Peff

On 3 August 2017 at 21:29, Jeff King <peff@peff.net> wrote:

Right. I realized I was using "false" everywhere else. It wouldn't have hurt to exercise the config-parsing a tiny bit differently, but I assume that's already being done explicitly in some other test, so I went for consistency.

Thanks for all the feedback and thoughts throughout the different versions of this series. It changed quite a bit since v1, so thanks a lot.

Martin

On Fri, Aug 04, 2017 at 06:21:47AM +0200, Martin Ågren wrote:

Right, that makes perfect sense. Thanks for confirming.

You're very welcome. I hope we see more patches from you in the future. :)

-Peff