* [PATCH 1/5] parse_branchname_arg(): fix dash_dash_pos, drop argcount
2019-11-28 19:32 [PATCH 0/5] parse_branchname_arg(): make code easier to understand Alexandr Miloslavskiy via GitGitGadget
@ 2019-11-28 19:32 ` Alexandr Miloslavskiy via GitGitGadget
2019-12-18 18:52 ` Junio C Hamano
2019-11-28 19:32 ` [PATCH 2/5] parse_branchname_arg(): introduce expect_commit_only Alexandr Miloslavskiy via GitGitGadget
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-11-28 19:32 UTC (permalink / raw)
To: git; +Cc: Alexandr Miloslavskiy, Junio C Hamano, Alexandr Miloslavskiy
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] parse_branchname_arg(): fix dash_dash_pos, drop argcount
2019-11-28 19:32 ` [PATCH 1/5] parse_branchname_arg(): fix dash_dash_pos, drop argcount Alexandr Miloslavskiy via GitGitGadget
@ 2019-12-18 18:52 ` Junio C Hamano
2019-12-19 18:03 ` Alexandr Miloslavskiy
0 siblings, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2019-12-18 18:52 UTC (permalink / raw)
To: Alexandr Miloslavskiy via GitGitGadget; +Cc: git, Alexandr Miloslavskiy
"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.
> + }
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] parse_branchname_arg(): fix dash_dash_pos, drop argcount
2019-12-18 18:52 ` Junio C Hamano
@ 2019-12-19 18:03 ` Alexandr Miloslavskiy
0 siblings, 0 replies; 10+ messages in thread
From: Alexandr Miloslavskiy @ 2019-12-19 18:03 UTC (permalink / raw)
To: Junio C Hamano, Alexandr Miloslavskiy via GitGitGadget; +Cc: git
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/
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/5] parse_branchname_arg(): introduce expect_commit_only
2019-11-28 19:32 [PATCH 0/5] parse_branchname_arg(): make code easier to understand Alexandr Miloslavskiy via GitGitGadget
2019-11-28 19:32 ` [PATCH 1/5] parse_branchname_arg(): fix dash_dash_pos, drop argcount Alexandr Miloslavskiy via GitGitGadget
@ 2019-11-28 19:32 ` Alexandr Miloslavskiy via GitGitGadget
2019-12-18 19:18 ` Junio C Hamano
2019-11-28 19:32 ` [PATCH 3/5] parse_branchname_arg(): update code comments Alexandr Miloslavskiy via GitGitGadget
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-11-28 19:32 UTC (permalink / raw)
To: git; +Cc: Alexandr Miloslavskiy, Junio C Hamano, Alexandr Miloslavskiy
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/5] parse_branchname_arg(): introduce expect_commit_only
2019-11-28 19:32 ` [PATCH 2/5] parse_branchname_arg(): introduce expect_commit_only Alexandr Miloslavskiy via GitGitGadget
@ 2019-12-18 19:18 ` Junio C Hamano
2019-12-19 18:03 ` Alexandr Miloslavskiy
0 siblings, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2019-12-18 19:18 UTC (permalink / raw)
To: Alexandr Miloslavskiy via GitGitGadget; +Cc: git, Alexandr Miloslavskiy
"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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/5] parse_branchname_arg(): introduce expect_commit_only
2019-12-18 19:18 ` Junio C Hamano
@ 2019-12-19 18:03 ` Alexandr Miloslavskiy
0 siblings, 0 replies; 10+ messages in thread
From: Alexandr Miloslavskiy @ 2019-12-19 18:03 UTC (permalink / raw)
To: Junio C Hamano, Alexandr Miloslavskiy via GitGitGadget; +Cc: git
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/
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/5] parse_branchname_arg(): update code comments
2019-11-28 19:32 [PATCH 0/5] parse_branchname_arg(): make code easier to understand Alexandr Miloslavskiy via GitGitGadget
2019-11-28 19:32 ` [PATCH 1/5] parse_branchname_arg(): fix dash_dash_pos, drop argcount Alexandr Miloslavskiy via GitGitGadget
2019-11-28 19:32 ` [PATCH 2/5] parse_branchname_arg(): introduce expect_commit_only Alexandr Miloslavskiy via GitGitGadget
@ 2019-11-28 19:32 ` Alexandr Miloslavskiy via GitGitGadget
2019-11-28 19:32 ` [PATCH 4/5] parse_branchname_arg(): refactor the decision making Alexandr Miloslavskiy via GitGitGadget
2019-11-28 19:32 ` [PATCH 5/5] t2024: cover more cases Alexandr Miloslavskiy via GitGitGadget
4 siblings, 0 replies; 10+ messages in thread
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-11-28 19:32 UTC (permalink / raw)
To: git; +Cc: Alexandr Miloslavskiy, Junio C Hamano, Alexandr Miloslavskiy
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/5] parse_branchname_arg(): refactor the decision making
2019-11-28 19:32 [PATCH 0/5] parse_branchname_arg(): make code easier to understand Alexandr Miloslavskiy via GitGitGadget
` (2 preceding siblings ...)
2019-11-28 19:32 ` [PATCH 3/5] parse_branchname_arg(): update code comments Alexandr Miloslavskiy via GitGitGadget
@ 2019-11-28 19:32 ` Alexandr Miloslavskiy via GitGitGadget
2019-11-28 19:32 ` [PATCH 5/5] t2024: cover more cases Alexandr Miloslavskiy via GitGitGadget
4 siblings, 0 replies; 10+ messages in thread
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-11-28 19:32 UTC (permalink / raw)
To: git; +Cc: Alexandr Miloslavskiy, Junio C Hamano, Alexandr Miloslavskiy
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 5/5] t2024: cover more cases
2019-11-28 19:32 [PATCH 0/5] parse_branchname_arg(): make code easier to understand Alexandr Miloslavskiy via GitGitGadget
` (3 preceding siblings ...)
2019-11-28 19:32 ` [PATCH 4/5] parse_branchname_arg(): refactor the decision making Alexandr Miloslavskiy via GitGitGadget
@ 2019-11-28 19:32 ` Alexandr Miloslavskiy via GitGitGadget
4 siblings, 0 replies; 10+ messages in thread
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-11-28 19:32 UTC (permalink / raw)
To: git; +Cc: Alexandr Miloslavskiy, Junio C Hamano, Alexandr Miloslavskiy
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
^ permalink raw reply related [flat|nested] 10+ messages in thread