On Tuesday 03 April 2018 01:30 PM, Eric Sunshine wrote: >> Note that the "detached HEAD" test case might actually fail in some cases >> as the actual output of "git branch --list" might contain remote branch >> names which is not considered by the test case as it is rare to happen >> in the test environment. > > This paragraph was not in the original patch[1]. I _think_ what you > are saying (which took a while to decipher) is that if a command such > as "git checkout origin/next" ever gets inserted into the script > before the test, the test will be fooled since "git branch --list" > will show "detached HEAD origin/next" rather than "detached HEAD > d3adb33f", the latter of which is what the test is expecting. > Yeah, you're right. To know the reason for the unclear paragraph, see below. > Unfortunately, this paragraph makes it sound as if the test can fail > randomly (which, I believe, is not the case), and nobody would want a > test added which is unreliable, thus this paragraph is not helping to > sell this patch (in fact, it's actively hurting it). Ideally, the test > should be entirely deterministic so that it can't be fooled like this. > Rather than including this (harmful) paragraph in the commit message, > let's ensure that the test is deterministic (see below). > Sorry for the harmful and not so clear paragraph! I actually kept that paragraph there to **remind me** that I have to fix the issue which it describes before sending out the patch but I somehow forgot about it after I added it initially :-( >> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh >> @@ -1246,6 +1247,29 @@ test_expect_success '--merged is incompatible with --no-merged' ' >> +test_expect_success '--list during rebase from detached HEAD' ' >> + test_when_finished "reset_rebase && git checkout master" && >> + git checkout HEAD^0 && > > This is the line which I think is causing concern for you. If someone > inserted a new test before this one which invoked "git checkout > origin/next" (or something), then even after "git checkout HEAD^0", > "git branch --list" would still report the unexpected "detached HEAD > origin/next". Let's fix this, and make the test deterministic, by > doing this instead: > > git checkout master^0 && > Nice idea, will re-send a v3 with this fix and the harmful paragraph removed. Thanks, Kaartic
Attachment:
signature.asc
Description: OpenPGP digital signature
