git checkout creates strange '(null)'-branch

[Erik Faye-Lund <kusmabite@gmail.com>]

I'm not very familiar with submodules, but I gave them a go today, and
quite quickly bumped into something I consider a bit... odd behavior:
When I add an empty submodule and commit to it, a strangely named
"(null)"-branch gets created.

Here's a test that illustrate the issue:

---8<---
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 81827e6..ce47b0a 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -520,4 +520,16 @@ test_expect_success 'moving the superproject does
not break submodules' '
 	)
 '

+test_expect_failure 'committing to empty submodule does not create
(null) branch' '
+	test_create_repo empty-repo &&
+	git submodule add ./empty-repo empty-submodule &&
+	(
+		cd empty-submodule &&
+		echo "foo" > bar.txt &&
+		git add bar.txt &&
+		git commit -m. &&
+		git show-ref | !grep "(null)"
+	)
+'
+
 test_done
---8<---

Now, it could very well be that the best move here would be "don't do
that". But in that case, I think we should error out instead of
creating a cryptic branch.

So, I decided to dig a bit and see if I could figure out where this
strange branch-name came from. So I inserted a few calls to
test_pause, and noticed that:
1) empty-repo/.git/HEAD contains "ref: refs/heads/master"
2) .git/modules/empty-submodule/HEAD contains "ref: refs/heads/(null)"

Digging further, it turns out that this magical "(null)"-branch
creation can be reproduced without the use of "git submodule":
$ git init empty
$ cd empty
$ cat .git/HEAD
ref: refs/heads/master
$ git checkout
$ cat .git/HEAD
ref: refs/heads/(null)

The offending code seems to be switch_unborn_to_new_branch, when
opts->new_branch is NULL. This is relatively new code, introduced in
January this year by commit abe1998 ("git checkout -b: allow switching
out of an unborn branch") .

Before this commit, the checkout would error out with "fatal: You are
on a branch yet to be born". Perhaps abe1998 was too optimistic about
allowing this, and something like this would be in order?

---8<---
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 23fc56d..35924d4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1091,7 +1091,7 @@ int cmd_checkout(int argc, const char **argv,
const char *prefix)
 	if (opts.writeout_stage)
 		die(_("--ours/--theirs is incompatible with switching branches."));

-	if (!new.commit) {
+	if (!new.commit && opts.new_branch) {
 		unsigned char rev[20];
 		int flag;
---8<---

Now, doing this doesn't make my test above pass, but it makes the "git
submodule add" call fail when the submodule is empty, which is much
better than what we do now IMO.