Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode

[Junio C Hamano <gitster@pobox.com>]

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

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

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. 

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

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

Thanks.