git checkout creates strange '(null)'-branch

git checkout creates strange '(null)'-branch

[Erik Faye-Lund]

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.

Re: git checkout creates strange '(null)'-branch

[Johannes Sixt]

Am 5/8/2012 13:24, schrieb Erik Faye-Lund:
> +		git show-ref | !grep "(null)"
...
> Now, doing this doesn't make my test above pass,

It is unlikely to pass if you used this test script literally, because

   $ !grep
   !grep: command not found

"!" is not an operator in a shell script. Insert a space.

Re: git checkout creates strange '(null)'-branch

[Erik Faye-Lund]

On Tue, May 8, 2012 at 1:34 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 5/8/2012 13:24, schrieb Erik Faye-Lund:
>> +             git show-ref | !grep "(null)"
> ...
>> Now, doing this doesn't make my test above pass,
>
> It is unlikely to pass if you used this test script literally, because
>
>   $ !grep
>   !grep: command not found
>
> "!" is not an operator in a shell script. Insert a space.

Whoops, you are of course right, my bad. Actually, inserting a space
doesn't quite do the trick, but doing
"! (git show-ref | grep "(null)")" instead seems to work. I'm no
shell-script wizard ;)

The test still fails, though :)

I didn't actually fix up the code so the test failed, so I guess
that's why I didn't see a false negative. Instead, I fixed up the code
so it failed earlier.

But if I apply the following patch, the test passes. I'm not saying
it's the right thing to do, though.

(Warning: white-space damaged because of copying diffs between terminals)

---8<---
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 23fc56d..d70e819 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1096,8 +1096,11 @@ int cmd_checkout(int argc, const char **argv,
const char *prefix)
                int flag;

                if (!read_ref_full("HEAD", rev, 0, &flag) &&
-                   (flag & REF_ISSYMREF) && is_null_sha1(rev))
+                   (flag & REF_ISSYMREF) && is_null_sha1(rev)) {
+                       if (!opts.new_branch)
+                               return 0;
                        return switch_unborn_to_new_branch(&opts);
+               }
        }
        return switch_branches(&opts, &new);
 }
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 81827e6..5b1932c 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<---

Re: git checkout creates strange '(null)'-branch

[Andreas Schwab]

Erik Faye-Lund <kusmabite@gmail.com> writes:

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

You don't need the parens, since ! operates on the whole pipeline.  (And
for grouping the shell language uses { } instead of ( ).)

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: git checkout creates strange '(null)'-branch

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> But if I apply the following patch, the test passes. I'm not saying
> it's the right thing to do, though.
>
> (Warning: white-space damaged because of copying diffs between terminals)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 23fc56d..d70e819 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1096,8 +1096,11 @@ int cmd_checkout(int argc, const char **argv,
> const char *prefix)
>                 int flag;
>
>                 if (!read_ref_full("HEAD", rev, 0, &flag) &&
> -                   (flag & REF_ISSYMREF) && is_null_sha1(rev))
> +                   (flag & REF_ISSYMREF) && is_null_sha1(rev)) {
> +                       if (!opts.new_branch)
> +                               return 0;
>                         return switch_unborn_to_new_branch(&opts);
> +               }
>         }
>         return switch_branches(&opts, &new);
>  }

This patch, if we ignore submodules for a while, actually makes sense to
me.  If you have a commit, you are on that commit and you haven't done
anything since you have checked out that commit, "git checkout" (no other
parameters) would be a no-op.  If you "git init" a repository, and you
haven't done anything since then, the above makes "git checkout" (no other
parameters) a no-op.

Am I missing some corner cases that we _should_ error out, perhaps for the
sake of safety?

Re: git checkout creates strange '(null)'-branch

[Erik Faye-Lund]

On Tue, May 8, 2012 at 6:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> But if I apply the following patch, the test passes. I'm not saying
>> it's the right thing to do, though.
>>
>> (Warning: white-space damaged because of copying diffs between terminals)
>>
>> diff --git a/builtin/checkout.c b/builtin/checkout.c
>> index 23fc56d..d70e819 100644
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -1096,8 +1096,11 @@ int cmd_checkout(int argc, const char **argv,
>> const char *prefix)
>>                 int flag;
>>
>>                 if (!read_ref_full("HEAD", rev, 0, &flag) &&
>> -                   (flag & REF_ISSYMREF) && is_null_sha1(rev))
>> +                   (flag & REF_ISSYMREF) && is_null_sha1(rev)) {
>> +                       if (!opts.new_branch)
>> +                               return 0;
>>                         return switch_unborn_to_new_branch(&opts);
>> +               }
>>         }
>>         return switch_branches(&opts, &new);
>>  }
>
> This patch, if we ignore submodules for a while, actually makes sense to
> me.  If you have a commit, you are on that commit and you haven't done
> anything since you have checked out that commit, "git checkout" (no other
> parameters) would be a no-op.  If you "git init" a repository, and you
> haven't done anything since then, the above makes "git checkout" (no other
> parameters) a no-op.

I agree. It does seem to make sense.

> Am I missing some corner cases that we _should_ error out, perhaps for the
> sake of safety?

I don't know, and I was kind of hoping someone would jump in and
enlighten me of something, but that didn't happen (yet).

I have a slight preference for this operation, though I'm not
religious; it simply seem more consistent with other "git checkout"
operation.

Do you want me to prepare a proper patch including a variation of the
test-case from the other patch?

Re: git checkout creates strange '(null)'-branch

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> On Tue, May 8, 2012 at 6:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> Am I missing some corner cases that we _should_ error out, perhaps for the
>> sake of safety?
>
> I don't know, and I was kind of hoping someone would jump in and
> enlighten me of something, but that didn't happen (yet).
>
> I have a slight preference for this operation, though I'm not
> religious; it simply seem more consistent with other "git checkout"
> operation.
>
> Do you want me to prepare a proper patch including a variation of the
> test-case from the other patch?

I think an official looking patch would draw more attention from others
and may uncover cases we did not anticipate if this turns out to be a bad
idea.

Re: git checkout creates strange '(null)'-branch

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 5/8/2012 13:24, schrieb Erik Faye-Lund:
>> +		git show-ref | !grep "(null)"
> ...
>> Now, doing this doesn't make my test above pass,
>
> It is unlikely to pass if you used this test script literally, because
>
>    $ !grep
>    !grep: command not found
>
> "!" is not an operator in a shell script. Insert a space.

Good eyes.  Also wouldn't it be a bit pointless to grep for (null) in the
first place?  It comes only because a particular implementation of
vsnprintf() happens to emit it when asked to turn a NULL pointer into a
string, and on other platforms it is allowed to segfault.

Re: git checkout creates strange '(null)'-branch

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

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

The patch looks like it will avoid flipping the HEAD, and in that case we
would go to switch_branches() codepath and will resurrect the old error
message, so it seems correct.

Thanks for digging.

Re: git checkout creates strange '(null)'-branch

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> +	test_create_repo empty-repo &&
> +	git submodule add ./empty-repo empty-submodule

You have an empty-repo but that is not part of the superproject; the
superproject will have empty-submodule submodule which is a clone of
empty-repo.

    $ git submodule add ./empty-repo empty-submodule
    Cloning into 'empty-submodule'...
    warning: You appear to have cloned an empty repository.
    done.

And after that, without you doing anything in empty-submodule, that
repository already exhibits the (null) problem.  Perhaps somebody, after
calling a successfull "git clone" of an empty repository (which is a silly
thing to do to begin with, with or without submodules involved, but at
least we do have a defined semantics of what happens when you do that),
tried to run "git checkout", even though there is nothing to be checked
out?  The culprit in that case to suspect would be "git submodule" script.
Does it blindly assume that it can do a "git checkout" and runs that
command?

Re: git checkout creates strange '(null)'-branch

[Erik Faye-Lund]

On Tue, May 8, 2012 at 5:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> +     test_create_repo empty-repo &&
>> +     git submodule add ./empty-repo empty-submodule
>
> You have an empty-repo but that is not part of the superproject; the
> superproject will have empty-submodule submodule which is a clone of
> empty-repo.
>
>    $ git submodule add ./empty-repo empty-submodule
>    Cloning into 'empty-submodule'...
>    warning: You appear to have cloned an empty repository.
>    done.
>
> And after that, without you doing anything in empty-submodule, that
> repository already exhibits the (null) problem.  Perhaps somebody, after
> calling a successfull "git clone" of an empty repository (which is a silly
> thing to do to begin with, with or without submodules involved, but at
> least we do have a defined semantics of what happens when you do that),
> tried to run "git checkout", even though there is nothing to be checked
> out?  The culprit in that case to suspect would be "git submodule" script.
> Does it blindly assume that it can do a "git checkout" and runs that
> command?

Yes, it does. Basically, it calls "git checkout -f -q" from cmd_add if
$branch is empty. And it fails if the checkout-call fails.

I'm not saying it's a sane thing to do. But to me, it kind of feels
natural to initialize the shared (i.e bare) repos for both the
superproject and the submodule, clone the superproject, add the
submodule, and populate these from there. But that won't work the way
things currently are, because you can't "git submodule add" an empty
project.

To allow that, we would have to make sure "git submodule add" works
with empty repositories. And we can probably archive this in multiple
ways.

But for now, I'm happy with just erroring out on the "git submodule
add"-call. I didn't really expect adding an empty submodule to work, I
was just hoping it did ;)

I'm sending a proper patch with a saner test soon.

Re: git checkout creates strange '(null)'-branch

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> Yes, it does. Basically, it calls "git checkout -f -q" from cmd_add if
> $branch is empty. And it fails if the checkout-call fails.
>
> I'm not saying it's a sane thing to do. But to me, it kind of feels
> natural to initialize the shared (i.e bare) repos for both the
> superproject and the submodule, clone the superproject, add the
> submodule, and populate these from there.

I sense a bit of slipperly slope here.  After doing this, the superproject
would have its .gitmodules file set up to point at something, but what
does its first commit look like?  Other files and .gitmodules, but no
empty-submodule?  Can you commit the superproject _with_ empty-submodule
without having any commit in that empty-submodule?  I think "git add"
should fail when there is no commit there in the first place, so you won't
create such a superproject commit that does not have anything in the
submodule.  Is that OK for everybody?  Or would we add yet another funny
special case for such a setting, perhaps by contaminating the index and
the tree in the superproject with a stand-in value to represent a "does
not yet exist" commit that ought to be at the tip of submodule's history?

> But that won't work the way
> things currently are, because you can't "git submodule add" an empty
> project.
>
> To allow that,...

Yes, allowing that does not seem to make sense at all in the larger
picture. What benefit would we get from that slipperly slope?