Re: [PATCH, nitpickingly final version] git-branch, git-checkout: autosetup for remote branch tracking

[Junio C Hamano <junkio@cox.net>]

Junio C Hamano <junkio@cox.net> writes:

> Paolo Bonzini <paolo.bonzini@lu.unisi.ch> writes:
>
>> 	This fixes all the nits you pointed out. :-D
>> 	Now, this was an experience to make...
>
> Thanks.
>
> Will apply after reviewing once more tomorrow, with fix-ups
> locally if needed.  No need to resend.

Gaah.

The new create_branch() is totally borked, and I did not notice
it while exchanging review e-mails.

The problem is that we have:

	if (real_ref == NULL)
        	die();

which is completely bogus.  We do "checkout -b temp v2.6.20~15"
all the time, and in that case dwim_ref(v2.6.20~15) rightfully
would say "nope, that's not a branch tip".  We have no right to
die there.

I think your new tests should have caught this kind of breakage,
but now I notice that there is no such test that makes sure that
no auto set-up is made when the original branch is not a remote
tracking branch (your new tests are only interested in testing
to see if the new feature works, and it does not test that there
is no regression to old usage).  The breakage was caught by an
unrelated test, t3900.

The appended is on top of your "final".  This is what I am
considering to apply currently.


Grumble....

---

diff --git a/builtin-branch.c b/builtin-branch.c
index 011a2fc..1d6f0cb 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -409,7 +409,6 @@ static void set_branch_defaults(const char *name, const char *real_ref)
 }
 
 static void create_branch(const char *name, const char *start_name,
-			  unsigned char *start_sha1,
 			  int force, int reflog, int track)
 {
 	struct ref_lock *lock;
@@ -430,15 +429,22 @@ static void create_branch(const char *name, const char *start_name,
 		forcing = 1;
 	}
 
-	if (start_sha1) {
-		/* detached HEAD */
-		hashcpy(sha1, start_sha1);
+	real_ref = NULL;
+	if (get_sha1(start_name, sha1))
+		die("Not a valid object name: '%s'.", start_name);
+
+	switch (dwim_ref(start_name, strlen(start_name), sha1, &real_ref)) {
+	case 0:
+		/* Not branching from any existing branch */
 		real_ref = NULL;
-	}
-	else if (dwim_ref(start_name, strlen(start_name), sha1, &real_ref) > 1)
+		break;
+	case 1:
+		/* Unique completion -- good */
+		break;
+	default:
 		die("Ambiguous object name: '%s'.", start_name);
-	else if (real_ref == NULL)
-		die("Not a valid object name: '%s'.", start_name);
+		break;
+	}
 
 	if ((commit = lookup_commit_reference(sha1)) == NULL)
 		die("Not a valid branch point: '%s'.", start_name);
@@ -619,12 +625,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		rename_branch(head, argv[i], force_rename);
 	else if (rename && (i == argc - 2))
 		rename_branch(argv[i], argv[i + 1], force_rename);
-	else if (i == argc - 1)
-		create_branch(argv[i], head, head_sha1, force_create, reflog,
-			      track);
-	else if (i == argc - 2)
-		create_branch(argv[i], argv[i+1], NULL, force_create, reflog,
-			      track);
+	else if (i == argc - 1 || i == argc - 2)
+		create_branch(argv[i], (i == argc - 2) ? argv[i+1] : head,
+			      force_create, reflog, track);
 	else
 		usage(builtin_branch_usage);