Re: "checkout --track -b" broken? (with suggested fix)
- From
Daniel Barkalow <barkalow@iabervon.org>
- Date
- Oct 17, 2008, 15:47 UTC
- Message-ID
- <alpine.LNX.1.00.0810171113340.19665@iabervon.org>
- In-Reply-To
- <7vej2fohfr.fsf@gitster.siamese.dyndns.org>
On Thu, 16 Oct 2008, Junio C Hamano wrote:
Show 33 quoted lines
> 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".
Show 42 quoted lines
> 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 &&
>