git submodule -b ... of current HEAD fails

git submodule add -b $branch $repository

fails when HEAD already points to $branch in $repository.

---
Hi Klaus,

Klaus Ethgen wrote at <http://bugs.debian.org/605600>:

Interesting. The problem is in cmd_add of git-submodule.sh; this patch demonstrates a quick fix. Jens, any idea why git submodule is not using "clone --branch" directly?

index 33bc41f..6242d7f 100755
@@ -241,7 +241,7 @@ cmd_add()
 			# ash fails to wordsplit ${branch:+-b "$branch"...}
 			case "$branch" in
 			'') git checkout -f -q ;;
-			?*) git checkout -f -q -b "$branch" "origin/$branch" ;;
+			?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
 			esac
 		) || die "Unable to checkout submodule '$path'"
 	fi

Am 01.12.2010 19:50, schrieb Jonathan Nieder:

Nope, these lines date back to the time before I got involved in the submodule business ... Seems like this "git checkout" was added in March 2008 by Mark Levedahl (CCed), maybe he can shed some light on that.

But to me your change looks good, so feel free to add:

Ah, so the problem is that "clone --branch" did not exist. Sorry for the noise.

Another question can be also be easily answered by history examination:
the series of checks in module_clone are because 70c7ac22d:git-clone.sh did not have checks of its own for the target directory.

So there is some simplification within grasp.

'night,
Jonathan

Since 55892d23 "git clone" itself checks that the destination path is not a file but an empty directory if it exists, so there is no need anymore for module_clone() to check that too.

Two tests have been added to test the behavior of "git submodule add" when path is a file or a directory (A subshell had to be added to the former last test to stay in the right directory).

---

Am 03.12.2010 08:10, schrieb Jonathan Nieder:

Maybe something like this?

 git-submodule.sh           |   14 --------------
 t/t7400-submodule-basic.sh |   28 +++++++++++++++++++++++-----
 2 files changed, 23 insertions(+), 19 deletions(-)

index 33bc41f..8085876 100755
@@ -93,20 +93,6 @@ module_clone()
 	url=$2
 	reference="$3"
 
-	# If there already is a directory at the submodule path,
-	# expect it to be empty (since that is the default checkout
-	# action) and try to remove it.
-	# Note: if $path is a symlink to a directory the test will
-	# succeed but the rmdir will fail. We might want to fix this.
-	if test -d "$path"
-	then
-		rmdir "$path" 2>/dev/null ||
-		die "Directory '$path' exists, but is neither empty nor a git repository"
-	fi
-
-	test -e "$path" &&
-	die "A file already exist at path '$path'"
-
 	if test -n "$reference"
 	then
 		git-clone "$reference" -n "$url" "$path"

index 782b0a3..2c49db9 100755
@@ -421,11 +421,29 @@ test_expect_success 'add submodules without specifying an explicit path' '
 		git commit -m "repo commit 1"
 	) &&
 	git clone --bare repo/ bare.git &&
-	cd addtest &&
-	git submodule add "$submodurl/repo" &&
-	git config -f .gitmodules submodule.repo.path repo &&
-	git submodule add "$submodurl/bare.git" &&
-	git config -f .gitmodules submodule.bare.path bare
+	(
+		cd addtest &&
+		git submodule add "$submodurl/repo" &&
+		git config -f .gitmodules submodule.repo.path repo &&
+		git submodule add "$submodurl/bare.git" &&
+		git config -f .gitmodules submodule.bare.path bare
+	)
+'
+
+test_expect_success 'add should fail when path is used by a file' '
+	(
+		cd addtest &&
+		touch file &&
+		test_must_fail	git submodule add "$submodurl/repo" file
+	)
+'
+
+test_expect_success 'add should fail when path is used by an existing directory' '
+	(
+		cd addtest &&
+		mkdir empty-dir &&
+		test_must_fail git submodule add "$submodurl/repo" empty-dir
+	)
 '
 
 test_done

1.7.3.2.657.g92628.dirty

These lines were actually added by Ben Jackson in commit ea10b60c91 in 2009, long after I last touched that module.

Mark

I didn't mean to change any functionality -- I just wanted to fix a portability problem (/bin/sh is ash on FreeBSD, hence the comment at the top of the context). Looks like `checkout -B' (capital B) didn't even exist at that time. Seems reasonable, though.

Ben Jackson AD7GD
<[email protected]>
http://www.ben.com/

Does either of you want to add a test for this?

Am 07.12.2010 23:57, schrieb Junio C Hamano:

Will do.

git submodule add -b $branch $repository

fails when HEAD already points to $branch in $repository.

When the freshly cloned submodules HEAD is the same as the checked out branch, it doesn't make sense to update it again as "git checkout -b" would fail with »fatal: git checkout: branch $branch already exists«.

Thanks-to: Jonathan Nieder <[email protected]>

---

Am 08.12.2010 22:35, schrieb Jens Lehmann:

And as it happens from time to time, while writing the test you find out that the first attempt to fix the bug didn't work as expected ...

 git-submodule.sh           |    4 +++-
 t/t7400-submodule-basic.sh |    7 +++++++
 2 files changed, 10 insertions(+), 1 deletions(-)

index 33bc41f..bf2803f 100755
@@ -241,7 +241,9 @@ cmd_add()
 			# ash fails to wordsplit ${branch:+-b "$branch"...}
 			case "$branch" in
 			'') git checkout -f -q ;;
-			?*) git checkout -f -q -b "$branch" "origin/$branch" ;;
+			?*) if [ "$(git branch)" != "* $branch"  ]; then
+				git checkout -f -q -b "$branch" "origin/$branch"
+			fi ;;
 			esac
 		) || die "Unable to checkout submodule '$path'"
 	fi

index 782b0a3..e224da4 100755
@@ -131,6 +131,13 @@ test_expect_success 'submodule add --branch' '
 	test_cmp empty untracked
 '
 
+test_expect_success 'submodule add --branch succeeds even when branch is at HEAD' '
+	(
+		cd addtest &&
+		git submodule add -b master "$submodurl" submod-existing-branch
+	)
+'
+
 test_expect_success 'submodule add with ./ in path' '
 	echo "refs/heads/master" >expect &&
 	>empty &&

1.7.3.3.580.ged75d

I was reviewing the overall picture before tagging 1.7.4-rc0 and started wondering if this was a good change. If repository already had branch checked out, and if it was pointing at a commit that was different from whatever was taken from origin, shouldn't the command _fail_ to prevent the divergent commits from getting lost?

Am 28.12.2010 22:42, schrieb Junio C Hamano:

But doesn't this change only affect the case where the submodule is freshly cloned, so there is no chance of losing local changes?

(But AFAIK the patch doesn't really fix the issue, please see [1] and Jonathan's followup)

[1] http://thread.gmane.org/gmane.linux.debian.devel.bugs.general/772659/focus=163242

My fault, for not looking at the surrounding context.

I think we queued the later round just uses "checkout -B"; shouldn't that work?

Am 29.12.2010 01:34, schrieb Junio C Hamano:

That's what I thought too (and that is what I based my ack upon, the patch hit the right spot and used the - according to the documentation
- better suited -B option). But while writing the test you rightfully requested I noticed that using -B didn't change much. The reason is that the following commands both fail in a freshly cloned repo:

$ git checkout -f -q -b master origin/master
fatal: git checkout: branch master already exists
$ git checkout -f -q -B master origin/master
fatal: Cannot force update the current branch.

So maybe the real problem here is that "git checkout -B" barfs when it doesn't have anything to do instead of silently doing nothing?

So we maybe want to fix this issue in "git checkout"? Then the patch will start working (and the test for it can be added in a later patch).

Let me think aloud, as it may be a good time to summarize the current semantics of what "checking out a branch" means.

0. "git checkout $branch" to check out a branch is to start preparing to commit on that branch while keeping the tentative changes you have in your working tree and your index.

1. You might not have the $branch yet to check out in such a way. You could do

$ git branch $branch $start && git checkout $branch

and

$ git checkout -b $branch $start

is conceptually a short-hand of the above two command sequence.

3. When $branch exists, 2. should fail, as it involves resetting the tip of $branch to $start, potentially losing commits near its old tip. If you really mean it, you can force it by

$ git checkout -B $branch $start

which is conceptually a short-hand for:

$ git branch -f $branch $start && git checkout $branch

4. "git checkout $branch" refuses to discard your work when the tentative changes you made conflict with the difference between your current HEAD and $branch. You can give "-m" to force it to attempt a three-way merge with possible conflicts, or you can give "-f" to force it to discard all tentative changes.

5. The logical consequence of all of the above is that "git checkout [-f] -B $branch $start" should conceptually be a short-hand for:

$ git branch -f $branch $start && git checkout [-f] $branch

And "branch -f $branch" refuses to reset the tip of the current branch.

An end-user who runs "git checkout -f -B $branch $start" is telling us the following things:

- The $branch may or may not exist;

- The tip it currently may point at is immaterial and the user is willing to lose it (this is what capital-ness of -B means);

- The tentative changes in the working tree and the index is immaterial (this is what -f given to checkout means).

It seems to me that "checkout -f -B $branch $start" when $branch is the current one ought to do an equivalent of "reset --hard $start" and nothing else.

What happens if -f is not given? The user wants to keep the tentative changes, and that is done by comparing a commit in HEAD (i.e. the current branch) and the target $branch. The two step approach that can be illustrated by the short-hand definition of "checkout -B" however breaks down in this case, even if you change the variant of "git branch -f" it internally runs:

    $ git branch --allow-updating-current -f $branch $start &&
      git checkout $branch

The first step already resets $branch to its new value, and the second step does not have a way to know that your tentative changes are based on the previous value.

BUT ;-)

I think it is Ok after all. The actual implementation of "checkout -[b|B]" is more like:

    $ git read-tree -m -u HEAD $start
    $ git update-ref -f refs/heads/$branch $start
    $ git symbolic-ref HEAD refs/heads/$branch

That is, when we update the working tree and the index, we still haven't updated the HEAD yet (switch_branches() calls merge_working_tree() to run this two-way merge, and then calls update_refs_for_switch() which does the update-ref part including the branch creation).

The next question is if we want to enable "git branch -f $branch $start" for the current branch from the end user, which presumably would look as if the user ran "git reset $start". I am actually indifferent about this, but as the first step we probably should be conservative to forbid it as we do now.

So in conclusion, here is a patch that is not even compile tested ;-)

This was made on top of 'maint' only because that was the branch I happened to have checked out in my development working tree.

Note that the test script does not even test the case I was initially worried about (i.e. keeping local changes). I'll leave it to interested others ;-)

 branch.c                   |   12 ++++++++----
 branch.h                   |   23 +++++++++++++++--------
 builtin/branch.c           |    3 ++-
 builtin/checkout.c         |    4 +++-
 t/t2018-checkout-branch.sh |   13 +++++++++++++
 5 files changed, 41 insertions(+), 14 deletions(-)

index 93dc866..74eb866 100644
@@ -137,7 +137,7 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
 
 void create_branch(const char *head,
 		   const char *name, const char *start_name,
-		   int force, int reflog, enum branch_track track)
+		   int flags, int reflog, enum branch_track track)
 {
 	struct ref_lock *lock = NULL;
 	struct commit *commit;

@@ -147,6 +147,9 @@ void create_branch(const char *head,
 	int forcing = 0;
 	int dont_change_ref = 0;
 	int explicit_tracking = 0;
+	int ok_to_update = flags & (CREATE_BRANCH_UPDATE_OK |
+				    CREATE_BRANCH_UPDATE_CURRENT_OK);
+	int ok_to_update_current = flags & CREATE_BRANCH_UPDATE_CURRENT_OK;
 
 	if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
 		explicit_tracking = 1;

@@ -155,11 +158,12 @@ void create_branch(const char *head,
 		die("'%s' is not a valid branch name.", name);
 
 	if (resolve_ref(ref.buf, sha1, 1, NULL)) {
-		if (!force && track == BRANCH_TRACK_OVERRIDE)
+		if (!ok_to_update && track == BRANCH_TRACK_OVERRIDE)
 			dont_change_ref = 1;
-		else if (!force)
+		else if (!ok_to_update)
 			die("A branch named '%s' already exists.", name);
-		else if (!is_bare_repository() && head && !strcmp(head, name))
+		else if (!ok_to_update_current &&
+			 !is_bare_repository() && head && !strcmp(head, name))
 			die("Cannot force update the current branch.");
 		forcing = 1;
 	}

index eed817a..0afbeae 100644
@@ -4,16 +4,23 @@
 /* Functions for acting on the information about branches. */
 
 /*
- * Creates a new branch, where head is the branch currently checked
- * out, name is the new branch name, start_name is the name of the
- * existing branch that the new branch should start from, force
- * enables overwriting an existing (non-head) branch, reflog creates a
- * reflog for the branch, and track causes the new branch to be
- * configured to merge the remote branch that start_name is a tracking
- * branch for (if any).
+ * Creates a new branch, where:
+ *
+ * - head is the branch currently checked out;
+ * - name is the new branch name;
+ * - start_name is the name of a commit that the new branch should start at
+ *   (could be another branch or a remote-tracking branch, in which case
+ *   track---see below---may also trigger);
+ * - flags indicates overwriting an existing branch and/or overwriting the
+ *   current branch is allowed;
+ * - reflog creates a reflog for the branch; and
+ * - track causes the new branch to be configured to merge the remote branch
+ *   that start_name is a tracking branch for (if any).
  */
+#define CREATE_BRANCH_UPDATE_OK 01
+#define CREATE_BRANCH_UPDATE_CURRENT_OK 02
 void create_branch(const char *head, const char *name, const char *start_name,
-		   int force, int reflog, enum branch_track track);
+		   int flags, int reflog, enum branch_track track);
 
 /*
  * Remove information about the state of working on the current

index 87976f0..ea5b411 100644
@@ -651,7 +651,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_BIT('m', NULL, &rename, "move/rename a branch and its reflog", 1),
 		OPT_BIT('M', NULL, &rename, "move/rename a branch, even if target exists", 2),
 		OPT_BOOLEAN('l', NULL, &reflog, "create the branch's reflog"),
-		OPT_BOOLEAN('f', "force", &force_create, "force creation (when already exists)"),
+		OPT_SET_INT('f', "force", &force_create, "force creation (when already exists)",
+			    CREATE_BRANCH_UPDATE_OK),
 		{
 			OPTION_CALLBACK, 0, "no-merged", &merge_filter_ref,
 			"commit", "print only not merged branches",

index a54583b..bf5b43a 100644
@@ -529,7 +529,9 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 		}
 		else
 			create_branch(old->name, opts->new_branch, new->name,
-				      opts->new_branch_force ? 1 : 0,
+				      opts->new_branch_force
+				      ? CREATE_BRANCH_UPDATE_CURRENT_OK
+				      : 0,
 				      opts->new_branch_log, opts->track);
 		new->name = opts->new_branch;
 		setup_branch_path(new);

index fa69016..37e4fdb 100755
@@ -169,4 +169,17 @@ test_expect_success 'checkout -f -B to an existing branch with mergeable changes
 	test_must_fail test_dirty_mergeable
 '
 
+test_expect_success 'checkout -b/B the current branch' '
+	git reset --hard &&
+	git checkout branch1 &&
+	it=$(git rev-parse --verify HEAD) &&
+	test_must_fail git checkout -b branch1 HEAD &&
+	git checkout -B branch1 $it &&
+	test "$it" = "$(git rev-parse --verify HEAD)" &&
+	test "refs/heads/branch1" = "$(git symbolic-ref HEAD)" &&
+	git checkout -B branch1 HEAD &&
+	test "$it" = "$(git rev-parse --verify HEAD)" &&
+	test "refs/heads/branch1" = "$(git symbolic-ref HEAD)"
+'
+
 test_done

Am 29.12.2010 23:13, schrieb Jens Lehmann:

Just for the record: It does compile (at least for me ;-) and together with Jonathan's patch referenced in the the subject passes all tests, including the new 'submodule add --branch succeeds even when branch is at HEAD' test I came up with to verify this issue.