"checkout --track -b" broken? (with suggested fix)

"checkout --track -b" broken? (with suggested fix)

[Junio C Hamano]

Sorry for bringing up a potentially old issue, but I do not think the test
added by 9ed36cf (branch: optionally setup branch.*.merge from upstream
local branches, 2008-02-19) is correct.  It does this:

    test_expect_success \
        'checkout w/--track from non-branch HEAD fails' '
        git checkout -b delete-me master &&
        rm .git/refs/heads/delete-me &&
        test refs/heads/delete-me = "$(git symbolic-ref HEAD)" &&
        test_must_fail git checkout --track -b track'

It creates branch 'delete-me' at the tip of 'master' to check it out, and
then it manually deletes the branch.  It expects "checkout --track -b track"
to fail because the current branch is broken and there is no way to set up
a tracking information for the new branch.

But I think this is bogus.  The checkout fails not because of lack of
trackability (due to broken current _branch_), but because there is no
current _commit_ to branch from.

	Jay CC'ed because 9ed36cf is his; Daniel CC'ed as branch.c was
	originally his.

Here is an attempt to fix the test, which then revealed that the feature
the commit introduced is broken in the tip of 'maint'.  Back when 9ed36cf
was written, test_must_fail was unavailable, and test_expect_failure meant
something different, so transplanting this on top of that old commit won't
reveal the breakage, but I strongly suspect that the feature was broken
from the very beginning.

The patch to branch.c is a quick fix for this issue.  The resulting code
passes all the tests, but I am not very proud of hardcoding the "HEAD" in
the code.  There must be a better way to do this.

 branch.c      |    4 +++-
 t/t7201-co.sh |   11 +++++------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git c/branch.c w/branch.c
index b1e59f2..6a75057 100644
--- c/branch.c
+++ w/branch.c
@@ -129,7 +129,9 @@ void create_branch(const char *head,
 			die("Cannot setup tracking information; starting point is not a branch.");
 		break;
 	case 1:
-		/* Unique completion -- good */
+		/* Unique completion -- good, only if it is a real ref */
+		if (track == BRANCH_TRACK_EXPLICIT && !strcmp(real_ref, "HEAD"))
+			die("Cannot setup tracking information; starting point is not a branch.");
 		break;
 	default:
 		die("Ambiguous object name: '%s'.", start_name);
diff --git c/t/t7201-co.sh w/t/t7201-co.sh
index fbec70d..da1fbf8 100755
--- c/t/t7201-co.sh
+++ w/t/t7201-co.sh
@@ -330,12 +330,11 @@ test_expect_success \
     test "$(git config branch.track2.merge)"
     git config branch.autosetupmerge false'
 
-test_expect_success \
-    'checkout w/--track from non-branch HEAD fails' '
-    git checkout -b delete-me master &&
-    rm .git/refs/heads/delete-me &&
-    test refs/heads/delete-me = "$(git symbolic-ref HEAD)" &&
-    test_must_fail git checkout --track -b track'
+test_expect_success 'checkout w/--track from non-branch HEAD fails' '
+    git checkout master^0 &&
+    test_must_fail git symbolic-ref HEAD &&
+    test_must_fail git checkout --track -b track
+'
 
 test_expect_success 'checkout an unmerged path should fail' '
 	rm -f .git/index &&

Re: "checkout --track -b" broken? (with suggested fix)

[Daniel Barkalow]

On Thu, 16 Oct 2008, Junio C Hamano wrote:

> Sorry for bringing up a potentially old issue, but I do not think the test
> added by 9ed36cf (branch: optionally setup branch.*.merge from upstream
> local branches, 2008-02-19) is correct.  It does this:
> 
>     test_expect_success \
>         'checkout w/--track from non-branch HEAD fails' '
>         git checkout -b delete-me master &&
>         rm .git/refs/heads/delete-me &&
>         test refs/heads/delete-me = "$(git symbolic-ref HEAD)" &&
>         test_must_fail git checkout --track -b track'
> 
> It creates branch 'delete-me' at the tip of 'master' to check it out, and
> then it manually deletes the branch.  It expects "checkout --track -b track"
> to fail because the current branch is broken and there is no way to set up
> a tracking information for the new branch.
> 
> But I think this is bogus.  The checkout fails not because of lack of
> trackability (due to broken current _branch_), but because there is no
> current _commit_ to branch from.
>
> 	Jay CC'ed because 9ed36cf is his; Daniel CC'ed as branch.c was
> 	originally his.
>
> Here is an attempt to fix the test, which then revealed that the feature
> the commit introduced is broken in the tip of 'maint'.  Back when 9ed36cf
> was written, test_must_fail was unavailable, and test_expect_failure meant
> something different, so transplanting this on top of that old commit won't
> reveal the breakage, but I strongly suspect that the feature was broken
> from the very beginning.
> 
> The patch to branch.c is a quick fix for this issue.  The resulting code
> passes all the tests, but I am not very proud of hardcoding the "HEAD" in
> the code.  There must be a better way to do this.

I agree with the change to the test. I think it would be better to 
hard-code "refs/heads/" instead of "HEAD", and I feel like we must have a 
"is this ref name a branch?" function, if only because someone could stick 
"refs/tags/foo" in HEAD, and we should still say it's not something you 
could track, despite it being something different from "HEAD".

>  branch.c      |    4 +++-
>  t/t7201-co.sh |   11 +++++------
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git c/branch.c w/branch.c
> index b1e59f2..6a75057 100644
> --- c/branch.c
> +++ w/branch.c
> @@ -129,7 +129,9 @@ void create_branch(const char *head,
>  			die("Cannot setup tracking information; starting point is not a branch.");
>  		break;
>  	case 1:
> -		/* Unique completion -- good */
> +		/* Unique completion -- good, only if it is a real ref */
> +		if (track == BRANCH_TRACK_EXPLICIT && !strcmp(real_ref, "HEAD"))
> +			die("Cannot setup tracking information; starting point is not a branch.");
>  		break;
>  	default:
>  		die("Ambiguous object name: '%s'.", start_name);
> diff --git c/t/t7201-co.sh w/t/t7201-co.sh
> index fbec70d..da1fbf8 100755
> --- c/t/t7201-co.sh
> +++ w/t/t7201-co.sh
> @@ -330,12 +330,11 @@ test_expect_success \
>      test "$(git config branch.track2.merge)"
>      git config branch.autosetupmerge false'
>  
> -test_expect_success \
> -    'checkout w/--track from non-branch HEAD fails' '
> -    git checkout -b delete-me master &&
> -    rm .git/refs/heads/delete-me &&
> -    test refs/heads/delete-me = "$(git symbolic-ref HEAD)" &&
> -    test_must_fail git checkout --track -b track'
> +test_expect_success 'checkout w/--track from non-branch HEAD fails' '
> +    git checkout master^0 &&
> +    test_must_fail git symbolic-ref HEAD &&
> +    test_must_fail git checkout --track -b track
> +'
>  
>  test_expect_success 'checkout an unmerged path should fail' '
>  	rm -f .git/index &&
> 

Re: "checkout --track -b" broken? (with suggested fix)

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Thu, 16 Oct 2008, Junio C Hamano wrote:
>
>> The patch to branch.c is a quick fix for this issue.  The resulting code
>> passes all the tests, but I am not very proud of hardcoding the "HEAD" in
>> the code.  There must be a better way to do this.
>
> I agree with the change to the test. I think it would be better to 
> hard-code "refs/heads/" instead of "HEAD", and I feel like we must have a 
> "is this ref name a branch?" function, if only because someone could stick 
> "refs/tags/foo" in HEAD, and we should still say it's not something you 
> could track, despite it being something different from "HEAD".

But you can track things under refs/remotes/, so...

Re: "checkout --track -b" broken? (with suggested fix)

[Daniel Barkalow]

On Fri, 17 Oct 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > On Thu, 16 Oct 2008, Junio C Hamano wrote:
> >
> >> The patch to branch.c is a quick fix for this issue.  The resulting code
> >> passes all the tests, but I am not very proud of hardcoding the "HEAD" in
> >> the code.  There must be a better way to do this.
> >
> > I agree with the change to the test. I think it would be better to 
> > hard-code "refs/heads/" instead of "HEAD", and I feel like we must have a 
> > "is this ref name a branch?" function, if only because someone could stick 
> > "refs/tags/foo" in HEAD, and we should still say it's not something you 
> > could track, despite it being something different from "HEAD".
> 
> But you can track things under refs/remotes/, so...

Ah, true. Maybe hard-code "refs/". Or have an "is this ref name a branch?" 
function that's paying more attention than I am, and returns true from 
both local branches and (local copies of) remote branches.

	-Daniel
*This .sig left intentionally blank*