Martin Ågren <martin.agren@xxxxxxxxx> 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.
