Re: [PATCH v2 0/5] support negative refspecs in git remote show

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 16, 2022, 21:31 UTC
Message-ID: <xmqqv8t0qoqh.fsf@gitster.g>
In-Reply-To: <20220616205456.19081-1-jacob.e.keller@intel.com>

Jacob Keller <jacob.keller@gmail.com> writes:

Show 20 quoted lines

> This series adds support for negative refspecs to git remote show, fixing an
> issue reported by Pavel Rappo.
>
> In addition, it includes some cleanup of the t5505-remote.sh test script,
> focusing on removing subshells and using test_config more.
>
> To support this, test_config and test_unconfig are extended to take and
> handle more options. The test_config_global is removed in favor of just
> using test_config --global.
>
> In addition, test_config now passes the value and --fixed-value into
> test_unconfig so that only the specific value is removed (rather than all
> keys of the name).
>
> The original v1 can be found here:
> https://lore.kernel.org/git/20220614003251.16765-1-jacob.e.keller@intel.com/
>
> If the config changes are too controversial, I'm happy to split them out
> into a separate series for further discussion, or drop them if they aren't
> desirable.

I did not see anything in 5/5 that substantially depends on all the code churn done in 1/5-4/5. Am I mistaken?

It would have been much nicer to organize the patch series so that the first one is the [v2 5/5]. It may not be able to use the improved test_config, but writing test_when_finished instead would not be the end of the world. The three-line test body will still be three lines. Then test_when_finished will have to be updated in follow-up patches that corresponds to [v2 1/5]-[v2 4/5], but that is the cost of "clean up". The main "fix" patch shouldn't be the one that is paying the price for it.

The clean-up offered by [v2 1/5] is a worthwhile thing to do. It's just that I do not think it is wise to make the fix in [v2 5/5] wait for the 1.4k lines patch to be adequately reviewed.

Retiring "test_config_global" in [v2 2/5] is probably a good change, especially when we are to add more featurs to test_config. Again, [v2 5/5] shouldn't have to be made waiting on an extra 800-line patch to be reviewed.