On Wed, May 4, 2022 at 7:27 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > "Tao Klerks via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > > > > @@ -510,63 +512,57 @@ test_expect_success 'git branch --copy dumps usage' ' > > test_expect_success 'git branch -c d e should work' ' > > git branch --create-reflog d && > > git reflog exists refs/heads/d && > > - git config branch.d.dummy Hello && > > + test_config branch.d.dummy Hello && > > git branch -c d e && > > git reflog exists refs/heads/d && > > git reflog exists refs/heads/e && > > - echo Hello >expect && > > - git config branch.e.dummy >actual && > > - test_cmp expect actual && > > - echo Hello >expect && > > - git config branch.d.dummy >actual && > > - test_cmp expect actual > > + test_cmp_config Hello branch.e.dummy && > > + test_cmp_config Hello branch.d.dummy > > ' > > This test used to leave both branch.d.dummy and branch.e.dummy behind > for later tests. Now with this patch, we clean branch.d.dummy > because we use test_config, but branch.e.dummy that was copied by > successful "git branch -c" will still be left. > > - It is unforunate that it is impossible to verify that the change > in behaviour for branch.d.dummy is correct. Without checking all > the remainder of the test (and no, grepping for branch.d.dummy is > not "checking all the remainder"---a later "branch -c d x" would > have created brnach.x.dummy in the original, but with this patch, > it would not), which is time consuming, that is. > > I trust you made sure that branch.d.dummy is never used after > this test is done---it would have been good to explain it either > in the proposed log message or after three-dash that you did > check and how to save reviewer bandwidth. I will add a comment, and will (first) more diligently check for dependencies on things now-removed. My strategy was, frankly, a little more haphazard: eyeball the intent, look for references in the following couple tests, and otherwise assume that if the tests still pass, it means they didn't unexpectedly depend on this. Fwiw, the not-trivial-but-still-straightforward approach I'm using is to search case-sensitively for the word "d" in this example. a, b, c and d are all bad because they are common english words or flags used throughout this test, but other letters and combinations are easier to check in this reasonably-simple way. > > - Are you deliberatly leaving branch.e.dummy uncleaned, or is it a > mere oversight? > Somewhere in-between. My intent with these changes was to use the right helpers/patterns, but I did not aspire to making sure nothing leaks between tests at all. Generally speaking, none of the tests I've seen (in this file or others) clean up / delete the *branches* they create - presumably because there's no significant need - the existence of branches rarely or never has side-effects unless explicitly referenced. Config, on the other hand, is a good thing to clean up by default, because it is much more likely to interfere with later tests. Something like configuring "branch.x.dummy" (or the more meaningful "branch.x.merge") straddles those 2 worlds - I would use test_config to be consistent if setting one manually, but it's still branch-specific stuff so I wouldn't be diligent about removing it if it was created as a side-effect of another command like "git branch -c" - just like I'm not removing the new branch itself. That said, now that you've called my attention to it, I'll look for leaky branch configs :) I will also add a note clarifying "intend to use the right patterns/helpers, but not necessarily to eliminate state leaks between tests" in the commit message. > > test_expect_success 'git branch --copy is a synonym for -c' ' > > git branch --create-reflog copy && > > git reflog exists refs/heads/copy && > > - git config branch.copy.dummy Hello && > > + test_config branch.copy.dummy Hello && > > git branch --copy copy copy-to && > > git reflog exists refs/heads/copy && > > git reflog exists refs/heads/copy-to && > > - echo Hello >expect && > > - git config branch.copy.dummy >actual && > > - test_cmp expect actual && > > - echo Hello >expect && > > - git config branch.copy-to.dummy >actual && > > - test_cmp expect actual > > + test_cmp_config Hello branch.copy.dummy && > > + test_cmp_config Hello branch.copy-to.dummy > > ' > > The same comment for branch.copy.dummy and branch.copy-to.dummy > applies. > > I'll stop here for now. Thanks for starting this clean-up. Thank you!
