[PATCH 0/5] parse_branchname_arg(): make code easier to understand

[PATCH 0/5] parse_branchname_arg(): make code easier to understand

[Alexandr Miloslavskiy via GitGitGadget]

My bigger goal is to complete my --pathspec-from-file series of patches.

For this, I needed to evaluate whether parse_branchname_arg() heuristics
needs any changes when pathspec is passed, but NOT in args.

I found it surprisingly hard to reason about the code in this function. This
mostly happens due to "obfuscated" variables, where they have a clear name
and a different actual meaning. Ultimately, it was hard to mentally expand
them to true meaning AND see all possible combinations of branches at once.

I have split this refactoring in 4 patches, so that diffs are not too big in
every single patch.

To my understanding, there should be no changes in git's behavior, except
for a couple better die() messages.

Alexandr Miloslavskiy (5):
  parse_branchname_arg(): fix dash_dash_pos, drop argcount
  parse_branchname_arg(): introduce expect_commit_only
  parse_branchname_arg(): update code comments
  parse_branchname_arg(): refactor the decision making
  t2024: cover more cases

 builtin/checkout.c       | 174 ++++++++++++++++-----------------------
 t/t2024-checkout-dwim.sh |  27 ++++++
 2 files changed, 97 insertions(+), 104 deletions(-)


base-commit: d9f6f3b6195a0ca35642561e530798ad1469bd41
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-479%2FSyntevoAlex%2F%230225(git)_refactor_parse_branchname_arg-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-479/SyntevoAlex/#0225(git)_refactor_parse_branchname_arg-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/479
-- 
gitgitgadget

[PATCH 1/5] parse_branchname_arg(): fix dash_dash_pos, drop argcount

[Alexandr Miloslavskiy via GitGitGadget]

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

`dash_dash_pos` was only calculated under `opts->accept_pathspec`. This
is unexpected to readers and made it harder to reason about the code.
Fix this by restoring the expected meaning.

Simplify the code by dropping `argcount` and useless `argc` / `argv`
manipulations.

This should not change behavior in any way.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
 builtin/checkout.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3634a3dac1..655b389756 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1121,7 +1121,6 @@ static int parse_branchname_arg(int argc, const char **argv,
 				int *dwim_remotes_matched)
 {
 	const char **new_branch = &opts->new_branch;
-	int argcount = 0;
 	const char *arg;
 	int dash_dash_pos;
 	int has_dash_dash = 0;
@@ -1180,17 +1179,21 @@ static int parse_branchname_arg(int argc, const char **argv,
 	arg = argv[0];
 	dash_dash_pos = -1;
 	for (i = 0; i < argc; i++) {
-		if (opts->accept_pathspec && !strcmp(argv[i], "--")) {
+		if (!strcmp(argv[i], "--")) {
 			dash_dash_pos = i;
 			break;
 		}
 	}
-	if (dash_dash_pos == 0)
-		return 1; /* case (2) */
-	else if (dash_dash_pos == 1)
-		has_dash_dash = 1; /* case (3) or (1) */
-	else if (dash_dash_pos >= 2)
-		die(_("only one reference expected, %d given."), dash_dash_pos);
+
+	if (opts->accept_pathspec) {
+	    if (dash_dash_pos == 0)
+		    return 1; /* case (2) */
+	    else if (dash_dash_pos == 1)
+		    has_dash_dash = 1; /* case (3) or (1) */
+	    else if (dash_dash_pos >= 2)
+		    die(_("only one reference expected, %d given."), dash_dash_pos);
+	}
+
 	opts->count_checkout_paths = !opts->quiet && !has_dash_dash;
 
 	if (!strcmp(arg, "-"))
@@ -1241,15 +1244,10 @@ static int parse_branchname_arg(int argc, const char **argv,
 		if (!recover_with_dwim) {
 			if (has_dash_dash)
 				die(_("invalid reference: %s"), arg);
-			return argcount;
+			return 0;
 		}
 	}
 
-	/* we can't end up being in (2) anymore, eat the argument */
-	argcount++;
-	argv++;
-	argc--;
-
 	setup_new_branch_info_and_source_tree(new_branch_info, opts, rev, arg);
 
 	if (!opts->source_tree)                   /* case (1): want a tree */
@@ -1262,15 +1260,11 @@ static int parse_branchname_arg(int argc, const char **argv,
 		 * even if there happen to be a file called 'branch';
 		 * it would be extremely annoying.
 		 */
-		if (argc)
+		if (argc > 1)
 			verify_non_filename(opts->prefix, arg);
-	} else if (opts->accept_pathspec) {
-		argcount++;
-		argv++;
-		argc--;
 	}
 
-	return argcount;
+	return (dash_dash_pos == 1) ? 2 : 1;
 }
 
 static int switch_unborn_to_new_branch(const struct checkout_opts *opts)
-- 
gitgitgadget

[PATCH 2/5] parse_branchname_arg(): introduce expect_commit_only

[Alexandr Miloslavskiy via GitGitGadget]

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

`has_dash_dash` unexpectedly takes `opts->accept_pathspec` into account.
While this may sound clever at first sight, it becomes pretty hard to
reason (and not be a victim) about code, especially in combination with
`argc` here:

	if (!(argc == 1 && !has_dash_dash) &&
	    !(argc == 2 && has_dash_dash) &&
	    opts->accept_pathspec)
		recover_with_dwim = 0;

Introduce a new non-obfuscated variable to reduce the amount of diffs in
next patch.

This should not change behavior in any way.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
 builtin/checkout.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 655b389756..5c6131dbe6 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1123,7 +1123,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 	const char **new_branch = &opts->new_branch;
 	const char *arg;
 	int dash_dash_pos;
-	int has_dash_dash = 0;
+	int has_dash_dash = 0, expect_commit_only = 0;
 	int i;
 
 	/*
@@ -1194,7 +1194,10 @@ static int parse_branchname_arg(int argc, const char **argv,
 		    die(_("only one reference expected, %d given."), dash_dash_pos);
 	}
 
-	opts->count_checkout_paths = !opts->quiet && !has_dash_dash;
+	if (has_dash_dash)
+	    expect_commit_only = 1;
+
+	opts->count_checkout_paths = !opts->quiet && !expect_commit_only;
 
 	if (!strcmp(arg, "-"))
 		arg = "@{-1}";
@@ -1210,10 +1213,10 @@ static int parse_branchname_arg(int argc, const char **argv,
 		 */
 		int recover_with_dwim = dwim_new_local_branch_ok;
 
-		int could_be_checkout_paths = !has_dash_dash &&
+		int could_be_checkout_paths = !expect_commit_only &&
 			check_filename(opts->prefix, arg);
 
-		if (!has_dash_dash && !no_wildcard(arg))
+		if (!expect_commit_only && !no_wildcard(arg))
 			recover_with_dwim = 0;
 
 		/*
@@ -1242,7 +1245,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 		}
 
 		if (!recover_with_dwim) {
-			if (has_dash_dash)
+			if (expect_commit_only)
 				die(_("invalid reference: %s"), arg);
 			return 0;
 		}
@@ -1253,7 +1256,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 	if (!opts->source_tree)                   /* case (1): want a tree */
 		die(_("reference is not a tree: %s"), arg);
 
-	if (!has_dash_dash) {	/* case (3).(d) -> (1) */
+	if (!expect_commit_only) {	/* case (3).(d) -> (1) */
 		/*
 		 * Do not complain the most common case
 		 *	git checkout branch
-- 
gitgitgadget

[PATCH 3/5] parse_branchname_arg(): update code comments

[Alexandr Miloslavskiy via GitGitGadget]

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

These parts repeat git documentation:
    ... if <something> is A...B <...>
    ... remote named in checkout.defaultRemote ...

Some parts repeat the code below. With next patch, code will be easier
to understand, so this is no longer needed.

This is a separate patch to reduce the amount of diffs in next patch.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
 builtin/checkout.c | 86 +++++++++++-----------------------------------
 1 file changed, 21 insertions(+), 65 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5c6131dbe6..723aaca0ef 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1127,45 +1127,21 @@ static int parse_branchname_arg(int argc, const char **argv,
 	int i;
 
 	/*
-	 * case 1: git checkout <ref> -- [<paths>]
-	 *
-	 *   <ref> must be a valid tree, everything after the '--' must be
-	 *   a path.
-	 *
-	 * case 2: git checkout -- [<paths>]
-	 *
-	 *   everything after the '--' must be paths.
-	 *
-	 * case 3: git checkout <something> [--]
-	 *
-	 *   (a) If <something> is a commit, that is to
-	 *       switch to the branch or detach HEAD at it.  As a special case,
-	 *       if <something> is A...B (missing A or B means HEAD but you can
-	 *       omit at most one side), and if there is a unique merge base
-	 *       between A and B, A...B names that merge base.
-	 *
-	 *   (b) If <something> is _not_ a commit, either "--" is present
-	 *       or <something> is not a path, no -t or -b was given, and
-	 *       and there is a tracking branch whose name is <something>
-	 *       in one and only one remote (or if the branch exists on the
-	 *       remote named in checkout.defaultRemote), then this is a
-	 *       short-hand to fork local <something> from that
-	 *       remote-tracking branch.
-	 *
-	 *   (c) Otherwise, if "--" is present, treat it like case (1).
-	 *
-	 *   (d) Otherwise :
-	 *       - if it's a reference, treat it like case (1)
-	 *       - else if it's a path, treat it like case (2)
-	 *       - else: fail.
-	 *
-	 * case 4: git checkout <something> <paths>
-	 *
-	 *   The first argument must not be ambiguous.
-	 *   - If it's *only* a reference, treat it like case (1).
-	 *   - If it's only a path, treat it like case (2).
-	 *   - else: fail.
-	 *
+	 * Resolve ambiguity where argv[0] may be <pathspec> or <commit>.
+	 * High-level approach is:
+	 * 1) Use various things to reduce ambiguity, examples:
+	 *    * '--' is present
+	 *    * command doesn't accept <pathspec>
+	 *    * additional options like '-b' were given
+	 * 2) If ambiguous and matches both existing <commit> and existing
+	 *    file, complain. However, in 1-argument 'git checkout <arg>'
+	 *    treat as <commit> to avoid annoying users.
+	 * 3) Otherwise, if it matches some existing <commit>, treat as
+	 *    <commit>.
+	 * 4) Otherwise, if it matches a remote branch, and it's considered
+	 *    reasonable to DWIM to create a local branch from remote branch,
+	 *    do that and proceed with (2)(3).
+	 * 5) Otherwise, let caller proceed with <pathspec> interpretation.
 	 */
 	if (!argc)
 		return 0;
@@ -1187,9 +1163,9 @@ static int parse_branchname_arg(int argc, const char **argv,
 
 	if (opts->accept_pathspec) {
 	    if (dash_dash_pos == 0)
-		    return 1; /* case (2) */
+		    return 1;
 	    else if (dash_dash_pos == 1)
-		    has_dash_dash = 1; /* case (3) or (1) */
+		    has_dash_dash = 1;
 	    else if (dash_dash_pos >= 2)
 		    die(_("only one reference expected, %d given."), dash_dash_pos);
 	}
@@ -1203,14 +1179,6 @@ static int parse_branchname_arg(int argc, const char **argv,
 		arg = "@{-1}";
 
 	if (get_oid_mb(arg, rev)) {
-		/*
-		 * Either case (3) or (4), with <something> not being
-		 * a commit, or an attempt to use case (1) with an
-		 * invalid ref.
-		 *
-		 * It's likely an error, but we need to find out if
-		 * we should auto-create the branch, case (3).(b).
-		 */
 		int recover_with_dwim = dwim_new_local_branch_ok;
 
 		int could_be_checkout_paths = !expect_commit_only &&
@@ -1219,10 +1187,6 @@ static int parse_branchname_arg(int argc, const char **argv,
 		if (!expect_commit_only && !no_wildcard(arg))
 			recover_with_dwim = 0;
 
-		/*
-		 * Accept "git checkout foo", "git checkout foo --"
-		 * and "git switch foo" as candidates for dwim.
-		 */
 		if (!(argc == 1 && !has_dash_dash) &&
 		    !(argc == 2 && has_dash_dash) &&
 		    opts->accept_pathspec)
@@ -1238,7 +1202,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 					    arg);
 				*new_branch = arg;
 				arg = remote;
-				/* DWIMmed to create local branch, case (3).(b) */
+				/* DWIMmed to create local branch */
 			} else {
 				recover_with_dwim = 0;
 			}
@@ -1253,19 +1217,11 @@ static int parse_branchname_arg(int argc, const char **argv,
 
 	setup_new_branch_info_and_source_tree(new_branch_info, opts, rev, arg);
 
-	if (!opts->source_tree)                   /* case (1): want a tree */
+	if (!opts->source_tree)
 		die(_("reference is not a tree: %s"), arg);
 
-	if (!expect_commit_only) {	/* case (3).(d) -> (1) */
-		/*
-		 * Do not complain the most common case
-		 *	git checkout branch
-		 * even if there happen to be a file called 'branch';
-		 * it would be extremely annoying.
-		 */
-		if (argc > 1)
-			verify_non_filename(opts->prefix, arg);
-	}
+	if (!expect_commit_only && argc > 1)
+		verify_non_filename(opts->prefix, arg);
 
 	return (dash_dash_pos == 1) ? 2 : 1;
 }
-- 
gitgitgadget

[PATCH 4/5] parse_branchname_arg(): refactor the decision making

[Alexandr Miloslavskiy via GitGitGadget]

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

Make it easier to understand which branches handle which cases.

Drop obfuscated variable `has_dash_dash` which also took
`opts->accept_pathspec` into account, making it pretty hard to reason
about code, especially when used together with `argc` and
`opts->accept_pathspec` here:

	if (!(argc == 1 && !has_dash_dash) &&
		!(argc == 2 && has_dash_dash) &&
		opts->accept_pathspec)
		recover_with_dwim = 0;

Avoid double-negation in the code mentioned above ("it is not OK to
proceed if it's not one of those cases").

Avoid hard-to-understand condition `opts->accept_pathspec` in the code
mentioned above.

There are some minor die() message changes for:
`git switch <commit> <unexpected>`
`git switch <commit> -- <unexpected>`

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
 builtin/checkout.c | 71 +++++++++++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 29 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 723aaca0ef..8f679d1b6a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1122,9 +1122,8 @@ static int parse_branchname_arg(int argc, const char **argv,
 {
 	const char **new_branch = &opts->new_branch;
 	const char *arg;
-	int dash_dash_pos;
-	int has_dash_dash = 0, expect_commit_only = 0;
-	int i;
+	int dash_dash_pos, i;
+	int recover_with_dwim, expect_commit_only;
 
 	/*
 	 * Resolve ambiguity where argv[0] may be <pathspec> or <commit>.
@@ -1143,15 +1142,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 	 *    do that and proceed with (2)(3).
 	 * 5) Otherwise, let caller proceed with <pathspec> interpretation.
 	 */
-	if (!argc)
-		return 0;
-
-	if (!opts->accept_pathspec) {
-		if (argc > 1)
-			die(_("only one reference expected"));
-		has_dash_dash = 1; /* helps disambiguate */
-	}
-
+	 
 	arg = argv[0];
 	dash_dash_pos = -1;
 	for (i = 0; i < argc; i++) {
@@ -1161,17 +1152,46 @@ static int parse_branchname_arg(int argc, const char **argv,
 		}
 	}
 
-	if (opts->accept_pathspec) {
-	    if (dash_dash_pos == 0)
-		    return 1;
-	    else if (dash_dash_pos == 1)
-		    has_dash_dash = 1;
-	    else if (dash_dash_pos >= 2)
-		    die(_("only one reference expected, %d given."), dash_dash_pos);
-	}
+	if (dash_dash_pos == -1) {
+		if (argc == 0) {
+			/* 'git checkout/switch/restore' */
+			return 0;
+		} else if (argc == 1) {
+			/* 'git checkout/switch/restore <something>' */
+			recover_with_dwim = dwim_new_local_branch_ok;
+		} else if (!opts->accept_pathspec) {
+			/* 'git switch <commit> <unexpected> [...]' */
+			die(_("only one reference expected, %d given."), argc);
+		} else {
+			/* 'git checkout/restore <something> <pathspec> [...]' */
+			recover_with_dwim = 0;
+		}
+
+		/* Absence of '--' leaves <pathspec>/<commit> ambiguity */
+		expect_commit_only = !opts->accept_pathspec;
+	} else if (dash_dash_pos == 0) {
+		/* 'git checkout/switch/restore -- [...]' */
+		return 1;  /* Eat '--' */
+	} else if (dash_dash_pos == 1) {
+		if (!opts->accept_pathspec) {
+			/* 'git switch <commit> -- [...]' */
+			die(_("incompatible with pathspec arguments"));
+		}
 
-	if (has_dash_dash)
-	    expect_commit_only = 1;
+		if (argc == 2) {
+			/* 'git checkout/restore <commit> --' */
+			recover_with_dwim = dwim_new_local_branch_ok;
+		} else {
+			/* 'git checkout/restore <commit> -- <pathspec> [...]' */
+			recover_with_dwim = 0;
+		}
+
+		/* Presence of '--' makes it certain that arg is <commit> */
+		expect_commit_only = 1;
+	} else {
+		/* 'git checkout/switch/restore <commit> <unxpected> [...] -- [...]' */
+		die(_("only one reference expected, %d given."), dash_dash_pos);
+	}
 
 	opts->count_checkout_paths = !opts->quiet && !expect_commit_only;
 
@@ -1179,19 +1199,12 @@ static int parse_branchname_arg(int argc, const char **argv,
 		arg = "@{-1}";
 
 	if (get_oid_mb(arg, rev)) {
-		int recover_with_dwim = dwim_new_local_branch_ok;
-
 		int could_be_checkout_paths = !expect_commit_only &&
 			check_filename(opts->prefix, arg);
 
 		if (!expect_commit_only && !no_wildcard(arg))
 			recover_with_dwim = 0;
 
-		if (!(argc == 1 && !has_dash_dash) &&
-		    !(argc == 2 && has_dash_dash) &&
-		    opts->accept_pathspec)
-			recover_with_dwim = 0;
-
 		if (recover_with_dwim) {
 			const char *remote = unique_tracking_name(arg, rev,
 								  dwim_remotes_matched);
-- 
gitgitgadget

[PATCH 5/5] t2024: cover more cases

[Alexandr Miloslavskiy via GitGitGadget]

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
 t/t2024-checkout-dwim.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index fa0718c730..2fe77387a6 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -132,6 +132,33 @@ test_expect_success 'checkout of branch from a single remote succeeds #2' '
 	test_branch_upstream baz repo_b baz
 '
 
+test_expect_success 'checkout of branch from a single remote succeeds with --' '
+	git checkout -B master &&
+	test_might_fail git branch -D baz &&
+
+	git checkout baz -- &&
+	status_uno_is_clean &&
+	test_branch baz &&
+	test_cmp_rev remotes/other_b/baz HEAD &&
+	test_branch_upstream baz repo_b baz
+'
+
+test_expect_success 'dont DWIM with pathspec #1' '
+	git checkout -B master &&
+	test_might_fail git branch -D baz &&
+
+	test_must_fail git checkout baz nonExistingFile 2>err &&
+	test_i18ngrep "did not match any file(s) known to git" err
+'
+
+test_expect_success 'dont DWIM with pathspec #2' '
+	git checkout -B master &&
+	test_might_fail git branch -D baz &&
+
+	test_must_fail git checkout baz -- nonExistingFile 2>err &&
+	test_i18ngrep "fatal: invalid reference: baz" err
+'
+
 test_expect_success '--no-guess suppresses branch auto-vivification' '
 	git checkout -B master &&
 	status_uno_is_clean &&
-- 
gitgitgadget

Re: [PATCH 1/5] parse_branchname_arg(): fix dash_dash_pos, drop argcount

[Junio C Hamano]

"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
>
> `dash_dash_pos` was only calculated under `opts->accept_pathspec`. This
> is unexpected to readers and made it harder to reason about the code.
> Fix this by restoring the expected meaning.

Perhaps.  I think the original reasoning was to compute only where
dash_dash_pos will be needed, but the code changes over time, and
places that need the value of dash_dash_pos would change over time,
so from that point of view, I think it makes sense to make sure it
gets always computed like this patch does.

> Simplify the code by dropping `argcount` and useless `argc` / `argv`
> manipulations.

I am not sure if this is a good change, though.  It goes against the
reasoning that makes the above "dash-dash-pos" change is a good one,
doesn't it?  When the code changes over time, wouldn't it make the
code more clear to keep track of the count of args it saw in a
separate variable, than relying on the invariant that currently
happens to hold which is "if dash-dash is after the first argument,
return 2 and otherwise return 1"?

> @@ -1121,7 +1121,6 @@ static int parse_branchname_arg(int argc, const char **argv,
>  				int *dwim_remotes_matched)
>  {
>  	const char **new_branch = &opts->new_branch;
> -	int argcount = 0;
>  	const char *arg;
>  	int dash_dash_pos;
>  	int has_dash_dash = 0;
> @@ -1180,17 +1179,21 @@ static int parse_branchname_arg(int argc, const char **argv,
>  	arg = argv[0];
>  	dash_dash_pos = -1;
>  	for (i = 0; i < argc; i++) {
> -		if (opts->accept_pathspec && !strcmp(argv[i], "--")) {
> +		if (!strcmp(argv[i], "--")) {
>  			dash_dash_pos = i;
>  			break;
>  		}
>  	}
> -	if (dash_dash_pos == 0)
> -		return 1; /* case (2) */
> -	else if (dash_dash_pos == 1)
> -		has_dash_dash = 1; /* case (3) or (1) */
> -	else if (dash_dash_pos >= 2)
> -		die(_("only one reference expected, %d given."), dash_dash_pos);
> +
> +	if (opts->accept_pathspec) {
> +	    if (dash_dash_pos == 0)
> +		    return 1; /* case (2) */
> +	    else if (dash_dash_pos == 1)
> +		    has_dash_dash = 1; /* case (3) or (1) */
> +	    else if (dash_dash_pos >= 2)
> +		    die(_("only one reference expected, %d given."), dash_dash_pos);

Non-standard indentation?  In our code, a level of indent is another
horizontal tab.

> +	}

Re: [PATCH 2/5] parse_branchname_arg(): introduce expect_commit_only

[Junio C Hamano]

"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
>
> `has_dash_dash` unexpectedly takes `opts->accept_pathspec` into account.

You also touched the code that depends on opts->accept_pathspec in
the earlier step 1/5; these two steps seem harder to reason about
than necessary---I wonder if it is easier to explain and understand
if these two steps are merged into one and explained together?

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 655b389756..5c6131dbe6 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1123,7 +1123,7 @@ static int parse_branchname_arg(int argc, const char **argv,
>  	const char **new_branch = &opts->new_branch;
>  	const char *arg;
>  	int dash_dash_pos;
> -	int has_dash_dash = 0;
> +	int has_dash_dash = 0, expect_commit_only = 0;
>  	int i;
>  
>  	/*
> @@ -1194,7 +1194,10 @@ static int parse_branchname_arg(int argc, const char **argv,
>  		    die(_("only one reference expected, %d given."), dash_dash_pos);
>  	}
>  
> -	opts->count_checkout_paths = !opts->quiet && !has_dash_dash;
> +	if (has_dash_dash)
> +	    expect_commit_only = 1;

Non-standard indentation here.

> +	opts->count_checkout_paths = !opts->quiet && !expect_commit_only;

OK.  count_checkout_paths is relevant only when checking out paths
out of a tree-ish, so "expect-commit-only" would be false in such a
situation.  On the other hand, if we were checking out a branch (or
detaching), we must have a commit and a tree-ish is insufficient,
so expect_commit_only would be true in such a case.

Makes sense.  I am wondering if we still need has_dash_dash, and
also if expect_commit_only is the best name for the variable.

Thanks.

> @@ -1210,10 +1213,10 @@ static int parse_branchname_arg(int argc, const char **argv,
>  		 */
>  		int recover_with_dwim = dwim_new_local_branch_ok;
>  
> -		int could_be_checkout_paths = !has_dash_dash &&
> +		int could_be_checkout_paths = !expect_commit_only &&
>  			check_filename(opts->prefix, arg);
>  
> -		if (!has_dash_dash && !no_wildcard(arg))
> +		if (!expect_commit_only && !no_wildcard(arg))
>  			recover_with_dwim = 0;
>  
>  		/*
> @@ -1242,7 +1245,7 @@ static int parse_branchname_arg(int argc, const char **argv,
>  		}
>  
>  		if (!recover_with_dwim) {
> -			if (has_dash_dash)
> +			if (expect_commit_only)
>  				die(_("invalid reference: %s"), arg);
>  			return 0;
>  		}
> @@ -1253,7 +1256,7 @@ static int parse_branchname_arg(int argc, const char **argv,
>  	if (!opts->source_tree)                   /* case (1): want a tree */
>  		die(_("reference is not a tree: %s"), arg);
>  
> -	if (!has_dash_dash) {	/* case (3).(d) -> (1) */
> +	if (!expect_commit_only) {	/* case (3).(d) -> (1) */
>  		/*
>  		 * Do not complain the most common case
>  		 *	git checkout branch

Re: [PATCH 1/5] parse_branchname_arg(): fix dash_dash_pos, drop argcount

[Alexandr Miloslavskiy]

Thanks for having a look at my patches!

On 18.12.2019 19:52, Junio C Hamano wrote:
>> Simplify the code by dropping `argcount` and useless `argc` / `argv`
>> manipulations.
> 
> I am not sure if this is a good change, though.  It goes against the
> reasoning that makes the above "dash-dash-pos" change is a good one,
> doesn't it?  When the code changes over time, wouldn't it make the
> code more clear to keep track of the count of args it saw in a
> separate variable, than relying on the invariant that currently
> happens to hold which is "if dash-dash is after the first argument,
> return 2 and otherwise return 1"?

My bad that I forgot to write a detailed explanation for that. In V3 in 
other topic [2] I have separated this change as a separate commit with a 
good message.

>> +	if (opts->accept_pathspec) {
>> +	    if (dash_dash_pos == 0)
>> +		    return 1; /* case (2) */
>> +	    else if (dash_dash_pos == 1)
>> +		    has_dash_dash = 1; /* case (3) or (1) */
>> +	    else if (dash_dash_pos >= 2)
>> +		    die(_("only one reference expected, %d given."), dash_dash_pos);
> 
> Non-standard indentation?  In our code, a level of indent is another
> horizontal tab.

Sorry, my Visual Studio was acting up again. It's doing pretty weird 
things when I have tab size 4 (due to other projects), which is 
overridden by git repo settings to 8.

Fixed it in V3 in other topic [2]

----
[1] Commit 09ebad6f ("checkout: split off a function to peel away 
branchname arg", 2011-02-08)
[2] 
https://lore.kernel.org/git/pull.490.v3.git.1576778515.gitgitgadget@gmail.com/

Re: [PATCH 2/5] parse_branchname_arg(): introduce expect_commit_only

[Alexandr Miloslavskiy]

On 18.12.2019 20:18, Junio C Hamano wrote:

>> `has_dash_dash` unexpectedly takes `opts->accept_pathspec` into account.
> 
> You also touched the code that depends on opts->accept_pathspec in
> the earlier step 1/5; these two steps seem harder to reason about
> than necessary---I wonder if it is easier to explain and understand
> if these two steps are merged into one and explained together?

Yes, that sounds like a good idea! In V3 in other topic [1] I have 
shuffled the changes between commits for easier understanding.

>> +	if (has_dash_dash)
>> +	    expect_commit_only = 1;
> 
> Non-standard indentation here.

Sorry!

>> +	opts->count_checkout_paths = !opts->quiet && !expect_commit_only;
> 
> OK.  count_checkout_paths is relevant only when checking out paths
> out of a tree-ish, so "expect-commit-only" would be false in such a
> situation.  On the other hand, if we were checking out a branch (or
> detaching), we must have a commit and a tree-ish is insufficient,
> so expect_commit_only would be true in such a case.
> 
> Makes sense.  I am wondering if we still need has_dash_dash, and
> also if expect_commit_only is the best name for the variable.

It seems that indeed, the variable could use an even better name. It's 
<commit> as opposed to <pathspec>, and not as opposed to <tree-ish>. I 
have renamed the variable again in V3 in other topic [1].

----
[1] 
https://lore.kernel.org/git/pull.490.v3.git.1576778515.gitgitgadget@gmail.com/