Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes: > Glen Choo <chooglen@xxxxxxxxxx> writes: >> >> +test_expect_success 'should create branches if branch exists and --force is given' ' >> >> + test_when_finished "cleanup_branches super branch-a" && >> >> + ( >> >> + cd super && >> >> + git -C sub rev-parse HEAD >expected && >> >> + test_commit -C sub baz && >> >> + git -C sub branch branch-a HEAD~1 && >> >> + git branch --recurse-submodules --force branch-a && >> >> + git rev-parse branch-a && >> >> + # assert that sub:branch-a was moved >> >> + git -C sub rev-parse branch-a >actual && >> >> + test_cmp expected actual >> >> + ) >> >> +' >> > >> > Should we create branch-a at HEAD instead of HEAD~1? >> >> If we create branch-a at HEAD, we won't be testing that --force moves >> the branch head. > > Walking through the lines of the test: > >> >> + git -C sub rev-parse HEAD >expected && > > So "expected" is sub's HEAD at the start of the test. Let's call this > old-head. > >> >> + test_commit -C sub baz && > > We create a new commit on top, whose parent is old-head. Let's call this > new-head. > >> >> + git -C sub branch branch-a HEAD~1 && > > We create a new branch at HEAD~1, which is new-head's parent, which is > old-head. So this branch points to the same thing as "expected". > >> >> + git branch --recurse-submodules --force branch-a && > > When creating new branches with "--force", any branch information in the > submodule is ignored. So we would expect "branch-a" in sub to be > overridden from "old-head" to "old-head". > >> >> + git rev-parse branch-a && > > Verifying that branch-a exists, although upon second look, this would > work whether or not we recursed, so maybe this line can be deleted. > >> >> + # assert that sub:branch-a was moved >> >> + git -C sub rev-parse branch-a >actual && >> >> + test_cmp expected actual > > A check, as expected. > > Unless I missed something, branch-a was never moved - it was created at > "old-head" and then the "branch --force" is supposed to create it at > "old-head" anyway. It would make more sense to me if the branch was > created at "new-head", and then "branch --force" moved it to "old-head". Oh, yes, I tested this and you're right. That's a really good catch - it would have been hard to spot amongst all of the tests. Yes, I should have set it up the way you described. I'll modify the test to assert that "branch --force" moved the branch and instead of creating it where we expected. >> This means that the test might pass if we simply ignore >> any existing branch-a - which is not the intended behavior of --force, >> but this is behavior that we might want in the future (probably using >> another option). > > By "ignore", supposing that there is an existing branch-a, do you mean > overwrite branch-a, or not create any branch at all? What I meant was that if branch-a was not moved, the test would pass if we did not create branch-a at all (and that we might want an option that will not create a branch if it already exists). But.. that comment was made under wrong assumptions - I had assumed that branch-a was moved, and that your proposed fix would not move branch-a (which is the complete opposite of reality). We agree that the test makes more sense if branch-a is moved.
