On Thu, Jun 16, 2022 at 2:31 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Jacob Keller <jacob.keller@xxxxxxxxx> writes: > > > 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@xxxxxxxxx/ > > > > 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. > Yea, I can re-order this. I generally opted to put it last since it added code which would get modified. But yes, I agree it makes more sense to be first. > 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. Yep, I'll re-order it.
