Re: [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --"

Jonathan Nieder [off-list ref] writes:

Hi,

Matthieu Moy wrote:

quoted

The "--" notation disambiguates files and branches, but as a side-effect
of the previous implementation, also disabled the branch auto-creation
when $branch does not exist.

Hm.  I am not sure that was just an implementation side-effect.

Normally 'git checkout <branch> --' means "Check out that branch,
and I mean it!".  'git checkout -- <pattern>' means "Check out
these paths from the index, and I mean it!"  'git checkout <blah>'
means "Do what I mean".

I'm not sure what was the intent, but I to me 'git checkout <branch> --'
means "I know <branch> is a ref, so don't try to interpret it
otherwise".

On the other hand, if I want to do 'git checkout <branch> --'
while disabling the "set up master to track origin/master" magic,
I can use 'git checkout --no-track <branch> --'.  So I think this
is a good change.

There's also --no-guess for that (purposely undocumented according to
the commit message of 46148dd7ea).

More importantly, what's the contract behind this function?  Is there
a simpler explanation than "If argument #2 is true, print a certain
message depending on argument #1; otherwise, return argument #3?".
If not, it might be clearer to inline it.

I made a function just because I needed the same pattern twice, and
having a function avoided overly nested if statements.

quoted

-	if (get_sha1_mb(arg, rev)) {
+	if (get_sha1_mb(arg, rev)) { /* case (1)? */

The check means that we are most likely not in case (1), since arg isn't
a commit name, right?

Not really. We're likely in an incorrect use of case 1, and want to give
an accurate error message (like "invalid reference").

quoted

+		int try_dwim = dwim_new_local_branch_ok;
+
+		if (check_filename(NULL, arg) && !has_dash_dash)
+			try_dwim = 0;
+		/*
+		 * Accept "git checkout foo" and "git checkout foo --"
+		 * as candidates for dwim.
+		 */
+		if (!(argc == 1 && !has_dash_dash) &&
+		    !(argc == 2 && has_dash_dash))
+			try_dwim = 0;
+
+		if (try_dwim) {
+			const char *remote = unique_tracking_name(arg, rev);
+			if (!remote)
+				return error_invalid_ref(arg, has_dash_dash, argcount);

This could be simplified by eliminating try_dwim local.

We are trying case (3) first:

		if (dwim_new_local_branch_ok &&
		    (argc == 1 || (argc == 2 && has_dash_dash)) &&
		    (has_dash_dash || !check_filename(NULL, arg))) {

I disagree that this is simpler. I introduced try_dwim precisely to
avoid this kind of monster boolean expression, and to leave room for
comments in the code.

You'd also need a "if (has_dash_dash)" inside this branch of the if to
give an accurate error message for "git checkout <non-branch> --".

[...]

quoted

--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh

@@ -164,4 +164,26 @@ test_expect_success 'checkout of branch from a single remote succeeds #4' '
 	test_branch_upstream eggs repo_d eggs
 '
 
+test_expect_success 'checkout of branch with a file having the same name fails' '
+	git checkout -B master &&
+	test_might_fail git branch -D spam &&
+
+	>spam &&
+	test_must_fail git checkout spam &&
+	test_must_fail git checkout spam &&

Why twice?

Oops, fixed.

Do we check that "git checkout --no-track spam --" avoids Dscho's
DWIM?

Could be done in addition, but I have no time for this, sorry.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/