* [PATCH v3 1/2] checkout: allow dwim for branch creation for "git checkout $branch --"
@ 2013-09-26 9:08 Matthieu Moy
2013-09-26 9:08 ` [PATCH v3 2/2] checkout: proper error message on 'git checkout foo bar --' Matthieu Moy
2013-10-17 18:09 ` [PATCH v3 1/2] checkout: allow dwim for branch creation for "git checkout $branch --" Junio C Hamano
0 siblings, 2 replies; 3+ messages in thread
From: Matthieu Moy @ 2013-09-26 9:08 UTC (permalink / raw
To: git, gitster; +Cc: pclouds, jc, jrnieder, Matthieu Moy
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.
A possible scenario is then:
git checkout $branch
=> fails if $branch is both a ref and a file, and suggests --
git checkout $branch --
=> refuses to create the $branch
This patch allows the second form to create $branch, and since the -- is
provided, it does not look for file named $branch.
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Since v2: essentially Jonathan's comments.
builtin/checkout.c | 70 ++++++++++++++++++++++++++++++++++--------------
t/t2024-checkout-dwim.sh | 21 +++++++++++++++
2 files changed, 71 insertions(+), 20 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0f57397..9edd9c3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -863,6 +863,13 @@ static const char *unique_tracking_name(const char *name, unsigned char *sha1)
return NULL;
}
+static int error_invalid_ref(const char *arg, int has_dash_dash, int argcount)
+{
+ if (has_dash_dash)
+ die(_("invalid reference: %s"), arg);
+ return argcount;
+}
+
static int parse_branchname_arg(int argc, const char **argv,
int dwim_new_local_branch_ok,
struct branch_info *new,
@@ -885,20 +892,30 @@ static int parse_branchname_arg(int argc, const char **argv,
*
* everything after the '--' must be paths.
*
- * case 3: git checkout <something> [<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 nor -b was given, and
+ * and there is a tracking branch whose name is <something>
+ * in one and only one remote, 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).
*
- * With no paths, 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.
+ * (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.
*
- * With no paths, if <something> is _not_ a commit, no -t nor -b
- * was given, and there is a tracking branch whose name is
- * <something> in one and only one remote, then this is a short-hand
- * to fork local <something> from that remote-tracking branch.
+ * case 4: git checkout <something> <paths>
*
- * Otherwise <something> shall not be ambiguous.
+ * 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.
@@ -917,19 +934,32 @@ static int parse_branchname_arg(int argc, const char **argv,
arg = "@{-1}";
if (get_sha1_mb(arg, rev)) {
- if (has_dash_dash) /* case (1) */
- die(_("invalid reference: %s"), arg);
- if (dwim_new_local_branch_ok &&
- !check_filename(NULL, arg) &&
- argc == 1) {
+ /*
+ * Either case (3) or (4), with <something> not being
+ * a commit, or an attempt to use case (1) with an
+ * invalid ref.
+ */
+ 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 argcount;
+ return error_invalid_ref(arg, has_dash_dash, argcount);
*new_branch = arg;
arg = remote;
- /* DWIMmed to create local branch */
+ /* DWIMmed to create local branch, case (3).(b) */
} else {
- return argcount;
+ return error_invalid_ref(arg, has_dash_dash, argcount);
}
}
@@ -958,7 +988,7 @@ static int parse_branchname_arg(int argc, const char **argv,
if (!*source_tree) /* case (1): want a tree */
die(_("reference is not a tree: %s"), arg);
- if (!has_dash_dash) {/* case (3 -> 1) */
+ if (!has_dash_dash) {/* case (3).(d) -> (1) */
/*
* Do not complain the most common case
* git checkout branch
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 094b92e..6ecb559 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -164,4 +164,25 @@ 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 rev-parse --verify refs/heads/spam &&
+ test_branch master
+'
+
+test_expect_success 'checkout <branch> -- succeeds, even if a file with the same name exists' '
+ git checkout -B master &&
+ test_might_fail git branch -D spam &&
+
+ >spam &&
+ git checkout spam -- &&
+ test_branch spam &&
+ test_cmp_rev refs/remotes/extra_dir/repo_c/extra_dir/spam HEAD &&
+ test_branch_upstream spam repo_c spam
+'
+
test_done
--
1.8.4.474.g128a96c
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH v3 2/2] checkout: proper error message on 'git checkout foo bar --'
2013-09-26 9:08 [PATCH v3 1/2] checkout: allow dwim for branch creation for "git checkout $branch --" Matthieu Moy
@ 2013-09-26 9:08 ` Matthieu Moy
2013-10-17 18:09 ` [PATCH v3 1/2] checkout: allow dwim for branch creation for "git checkout $branch --" Junio C Hamano
1 sibling, 0 replies; 3+ messages in thread
From: Matthieu Moy @ 2013-09-26 9:08 UTC (permalink / raw
To: git, gitster; +Cc: pclouds, jc, jrnieder, Matthieu Moy
The previous code was detecting the presence of "--" by looking only at
argument 1. As a result, "git checkout foo bar --" was interpreted as an
ambiguous file/revision list, and errored out with:
error: pathspec 'foo' did not match any file(s) known to git.
error: pathspec 'bar' did not match any file(s) known to git.
error: pathspec '--' did not match any file(s) known to git.
This patch fixes it by walking through the argument list to find the
"--", and now complains about the number of references given.
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Again, Jonathan's comments (partly) applied since v2.
builtin/checkout.c | 21 ++++++++++++++++-----
t/t2010-checkout-ambiguous.sh | 6 ++++++
2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9edd9c3..ef6b56a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -880,7 +880,9 @@ static int parse_branchname_arg(int argc, const char **argv,
int argcount = 0;
unsigned char branch_rev[20];
const char *arg;
- int has_dash_dash;
+ int dash_dash_pos;
+ int has_dash_dash = 0;
+ int i;
/*
* case 1: git checkout <ref> -- [<paths>]
@@ -924,11 +926,20 @@ static int parse_branchname_arg(int argc, const char **argv,
if (!argc)
return 0;
- if (!strcmp(argv[0], "--")) /* case (2) */
- return 1;
-
arg = argv[0];
- has_dash_dash = (argc > 1) && !strcmp(argv[1], "--");
+ dash_dash_pos = -1;
+ for (i = 0; i < argc; 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 (!strcmp(arg, "-"))
arg = "@{-1}";
diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh
index 7cc0a35..87bdf9c 100755
--- a/t/t2010-checkout-ambiguous.sh
+++ b/t/t2010-checkout-ambiguous.sh
@@ -47,4 +47,10 @@ test_expect_success 'disambiguate checking out from a tree-ish' '
git diff --exit-code --quiet
'
+test_expect_success 'accurate error message with more than one ref' '
+ test_must_fail git checkout HEAD master -- 2>actual &&
+ grep 2 actual &&
+ test_i18ngrep "one reference expected, 2 given" actual
+'
+
test_done
--
1.8.4.474.g128a96c
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3 1/2] checkout: allow dwim for branch creation for "git checkout $branch --"
2013-09-26 9:08 [PATCH v3 1/2] checkout: allow dwim for branch creation for "git checkout $branch --" Matthieu Moy
2013-09-26 9:08 ` [PATCH v3 2/2] checkout: proper error message on 'git checkout foo bar --' Matthieu Moy
@ 2013-10-17 18:09 ` Junio C Hamano
1 sibling, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2013-10-17 18:09 UTC (permalink / raw
To: Matthieu Moy; +Cc: git, pclouds, jc, jrnieder
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 0f57397..9edd9c3 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -863,6 +863,13 @@ static const char *unique_tracking_name(const char *name, unsigned char *sha1)
> return NULL;
> }
>
> +static int error_invalid_ref(const char *arg, int has_dash_dash, int argcount)
> +{
> + if (has_dash_dash)
> + die(_("invalid reference: %s"), arg);
> + return argcount;
> +}
This is somewhat unfortunate; it pretends to be a reusable helper by
being a separate function, but it is not very reusable (see below).
> @@ -917,19 +934,32 @@ static int parse_branchname_arg(int argc, const char **argv,
> arg = "@{-1}";
>
> if (get_sha1_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.
> + */
> + 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);
Up to this point, the updated code makes very good sense.
> if (!remote)
> - return argcount;
> + return error_invalid_ref(arg, has_dash_dash, argcount);
The original that returned "argcount" from here were unnecessarily
misleading in the first place. It saw "git checkout foo" where "foo"
does not refer to an object nor a filesystem entity and there was no
unique "refs/remotes/*/foo"; it wanted to return 0 to tell the
caller that it consumed zero arguments as branch names.
And the updated code is even more obscure. This calling site makes
it look as if it is an error to have no unique "refs/remotes/*/foo"
at this point of the code by naming the helper function "error_*()",
but it is an error in some case and not in others.
if (!remote) {
if (has_dash_dash)
die(_("..."));
return 0;
}
would be a lot more understandable.
The only reason you have conditional die() here (and on the "else"
side of this "if" statement) is because you delayed the die that was
at a much earlier point in the original. And the only reason you
created the unfortunate helper function is because you need to deal
with that delayed decision to die now in two places.
So it may be even cleaner to read if you did it this way:
if (get_sha1_mb(...)) {
/*
* The first token is not a valid rev; we should
* ordinarily error out if "git checkout foo --"
* if foo is not a valid rev, but first see if
* we can auto-create foo to continue...
*/
int recover_with_dwim = dwim_new_local_branch_ok;
... decide if we want to recover_with_dwim ...
if (recover_with_dwim) {
const char *remote = unique_tracking_name(arg, rev);
if (remote) {
*new_branch = arg;
arg = remote;
} else {
/* no; arg cannot be salvaged */
recover_with_dwim = 0;
}
}
if (!recover_with_dwim) {
if (has_dash_dash)
die(_("invalid ref %s", arg);
return 0; /* we saw no branch/commit */
}
/* otherwise we made a successful recovery */
}
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-10-17 18:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-26 9:08 [PATCH v3 1/2] checkout: allow dwim for branch creation for "git checkout $branch --" Matthieu Moy
2013-09-26 9:08 ` [PATCH v3 2/2] checkout: proper error message on 'git checkout foo bar --' Matthieu Moy
2013-10-17 18:09 ` [PATCH v3 1/2] checkout: allow dwim for branch creation for "git checkout $branch --" Junio C Hamano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.