Re: [PATCH v6 5/5] branch: add --recurse-submodules option for branch creation

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jan 11, 2022, 20:15 UTC
Message-ID: <20220111201556.26345-1-jonathantanmy@google.com>
In-Reply-To: <kl6l4k6akurn.fsf@chooglen-macbookpro.roam.corp.google.com>

Glen Choo <chooglen@google.com> writes:

Show 19 quoted lines

> >> +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".

> 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?