login
Search
Note that it also fix a bug, git checkout -- <path> would be badly
understood as git checkout <branch> if <path> is ambiguous with a branch.
Testcases included.
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
A user on #git happened to have issues that made me realize that
builtin-checkout is badly broken wrt argument parseing.
This clearly needs to be applied to master, probably to maint too.
The patch is against next though, but should probably apply to other
branches just fine.
builtin-checkout.c | 9 +++++++--
t/t2010-checkout-ambiguous.sh | 27 +++++++++++++++++++++++++++
2 files changed, 34 insertions(+), 2 deletions(-)
create mode 100755 t/t2010-checkout-ambiguous.sh
diff --git a/builtin-checkout.c b/builtin-checkout.c
index fbd5105..1490e8e 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -438,9 +438,14 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
opts.track = git_branch_track;
- argc = parse_options(argc, argv, options, checkout_usage, 0);
- if (argc) {
+ argc = parse_options(argc, argv, options, checkout_usage,
+ PARSE_OPT_KEEP_DASHDASH);
+
+ if (argc && strcmp(argv[0], "--")) {
arg = argv[0];
+
+ if (argc == 1 || strcmp(argv[1], "--"))
+ verify_non_filename(NULL, arg);
if (get_sha1(arg, rev))
;
else if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh
new file mode 100755
index 0000000..c1a86a2
--- /dev/null
+++ b/t/t2010-checkout-ambiguous.sh
@@ -0,0 +1,26 @@
+#!/bin/sh
+
+test_description='checkout and pathspecs/refspecs ambiguities'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ echo hello >world &&
+ git add world &&
+ git commit -m initial &&
+ git branch world
+'
+
+test_expect_success 'branch switching' '
+ git checkout world --
+'
+
+test_expect_success 'checkout world from the index' '
+ git checkout -- ...Okay those two tests are stupid in the sense that they don't check git-checkout does what it's supposed to do. One should check the first one outputs 'Switched to branch "world"' and the second should rather be: ' echo "bye bye" > world && git checkout -- world && git diff --quiet --exit-code '
Hi, Why "argc == 1"? Should "git checkout <path>" really fail? That would be a change in behavior that _would_ hit me. However, you may want to verify_non_filename() when argc == 1 _and_ get_sha1() succeeded. Because then, <path> is ambiguous. Ciao, Dscho --
No I was mistaken about what verify_non_filename did, actually I should not code when I'm so obviously tired, and I wanted verify_non_filename to do what I meant instead of checking what it does ;P Yes and the reverse when we have sucessfully parsed something that looks like a path as a path. Anyways, someone should carefully proofread my resent patch, it's likely that errors slipped through given my sleep deprivation atm.
Note that it also fix a bug, git checkout -- <path> would be badly
understood as git checkout <branch> if <path> is ambiguous with a branch.
Testcases included.
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
This is a resend with proper patches that made me realize that my
previous patch had silly mistakes and wasn't testing thigns properly.
builtin-checkout.c | 23 +++++++++++++++++------
t/t2010-checkout-ambiguous.sh | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 52 insertions(+), 6 deletions(-)
create mode 100755 t/t2010-checkout-ambiguous.sh
diff --git a/builtin-checkout.c b/builtin-checkout.c
index fbd5105..97321e6 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -438,12 +438,17 @@ int cmd_checkout(int argc, const char **argv, const c=
har *prefix)
=20
opts.track =3D git_branch_track;
=20
- argc =3D parse_options(argc, argv, options, checkout_usage, 0);
- if (argc) {
+ argc =3D parse_options(argc, argv, options, checkout_usage,
+ PARSE_OPT_KEEP_DASHDASH);
+
+ if (argc && strcmp(argv[0], "--")) {
+ int may_be_ambiguous =3D argc =3D=3D 1 || strcmp(argv[1], "--");
+
arg =3D argv[0];
- if (get_sha1(arg, rev))
- ;
- else if ((new.commit =3D lookup_commit_reference_gently(rev, 1))) {
+ if (get_sha1(arg, rev)) {
+ if (may_be_ambiguous)
+ verify_filename(NULL, arg);
+ } else if ((new.commit =3D lookup_commit_reference_gently(rev, 1))) {
new.name =3D arg;
setup_branch_path(&new);
if (resolve_ref(new.path, rev, 1, NULL))
@@ -454,10 +459,16 @@ int cmd_checkout(int argc, const char **argv, const c=
har *prefix)
source_tree =3D new.commit->tree;
argv++;
argc--;
+ if (may_be_ambiguous)
+ verify_non_filename(NULL, arg);
} else if ((source_tree =3D parse_tree_indirect(rev))) {
argv++;
argc--;
- }
+ if (may_be_ambiguous)
+ verify_non_filename(NULL, arg);
+ } else
+ if (may_be_ambiguous)
+ verify_filename(NULL, arg);
}
=20
if ...Dscho mentionned on irc that this bit is superfluous, we already know that 'arg' cannot be a refspec here, so in the worse case, if it's not a file at all, this will generate a curious error message about "ambiguous" arguments, whereas it should fail with an "unknown file" error or sth similar. This two lines should probably remain ';' like it previously was.
Hi,
Instead of the "Testcases included", I would have expected something like
When calling "git checkout <name>" and <name> is both a path and a
branch, Git would silently assume that you meant the branch.
Worse, "git checkout -- <name>" would behave the same.
Still you allow "git checkout <path> --" to succeed, right? It should
not.
IMHO you need "must_be_branch" and "must_not_be_path" instead of
"may_be_ambiguous".
I.e. something like
int must_be_branch = argc > 1 && !strcmp(argv[1], "--");
int must_not_be_path = argc > 1 && strcmp(argv[1], "--");
arg = argv[0];
if (get_sha1(arg, rev)) {
if (must_be_branch)
die ("Not a branch: '%s'", arg);
}
else {
if (must_not_be_path)
verify_non_filename(NULL, arg);
if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
[...]
And maybe you want to add a test for "git checkout <path> --" to fail,
too.
Ciao,
Dscho
--
Okay, so after a (too short) night sleep, here is a way better series. The first patch corrects a real bug, and should be applied to maint. It's a one liner, the comment is self explanatory. The second one is a twofold UI improvement wrt error messages that git-checkout (if we abstract the issues from patch 1) already caught as errors but with dreadful errors. And with respect to the case where there is no '--' and that there is an ambiguity. We used to always favor the interpretation where the first argument is understood as a path (which is really horrible because it's the destructive choice: at least if we favored the reference, user would never loose local modifications), it now barfs. I reckon I've not checked what git-checkout did when it wasn't a builtin, so I don't know if the second part of this UI improvement comes as fixing a regression (in which case the patch could be considered for maint) or if it's an ambiguity that was here like forever, in which case it's less urgent, but should IMHO be addressed because git-checkout is porcelain: see http://gist.github.com/1402 for the user issue that made me discover that, you'll see we want a desambiguation one way or the other. --
This fixes an issue when you use:
$ git checkout -- <path1> [<paths>...]
and that <path1> can also be understood as a reference. git-checkout
mistakenly understands this as the same as:
$ git checkout <path1> -- [<paths>...]
because parse-options was eating the '--' and the argument parser thought
he was parsing:
$ git checkout <path1> [<paths>...]
Where there indeed is an ambiguity
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
builtin-checkout.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/builtin-checkout.c b/builtin-checkout.c
index fbd5105..9cadf9c 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -438,7 +438,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
opts.track = git_branch_track;
- argc = parse_options(argc, argv, options, checkout_usage, 0);
+ argc = parse_options(argc, argv, options, checkout_usage,
+ PARSE_OPT_KEEP_DASHDASH);
if (argc) {
arg = argv[0];
if (get_sha1(arg, rev))
--
1.6.0.rc0.155.g8e50b
--
The patch is twofold: it moves the option consistency checks just under
the parse_options call so that it doesn't get in the way of the tree
reference vs. pathspecs desambiguation.
The other part rewrites the way to understand arguments so that when
git-checkout fails it does with an understandable message. Compared to the
previous behavior we now have:
- a better error message when doing:
git checkout <blob reference> --
now complains about the reference not pointing to a tree, instead of
things like:
error: pathspec <blob reference> did not match any file(s) known to git.
error: pathspec '--' did not match any file(s) known to git.
you what it spitted before.
- a better error message when doing:
git checkout <path> --
It now complains about <path> not being a reference instead of the
completely obscure:
error: pathspec '--' did not match any file(s) known to git.
- an error when -- wasn't used and the first argument is ambiguous.
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
builtin-checkout.c | 71 +++++++++++++++++++++++++++++++----------
t/t2010-checkout-ambiguous.sh | 39 ++++++++++++++++++++++
2 files changed, 93 insertions(+), 17 deletions(-)
create mode 100755 t/t2010-checkout-ambiguous.sh
diff --git a/builtin-checkout.c b/builtin-checkout.c
index 9cadf9c..e9ae834 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -430,6 +430,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
OPT_BOOLEAN('m', NULL, &opts.merge, "merge"),
OPT_END(),
};
+ int has_dash_dash;
memset(&opts, 0, sizeof(opts));
memset(&new, 0, sizeof(new));
@@ -440,11 +441,52 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, options, checkout_usage,
PARSE_OPT_KEEP_DASHDASH);
+
+ if (!opts.new_branch && (opts.track != git_branch_track))
+ die("git checkout: --track and --no-track require ...I think this goes a bit too far.
Even if you have a file called 'master' tracked in your project, when you
say:
$ git checkout master
that's almost always branch switching. Forcing "git checkout master --"
disambiguation for such a common case is simply a wrong thing to do from
the usability point of view.
So how about (obviously we are interested only in the case without
disambiguating '--' here):
(3-1) if there is only one token left and if it is a rev, that's the
branch to check out or commit to detach to.
(3-2) otherwise the user might have mistyped one of the paths, so help
avoiding by making sure the first token is unambiguously either
a rev or a path (but not both).
--
So something like this on top of your patch.
builtin-checkout.c | 19 +++++++++++++++----
t/t2010-checkout-ambiguous.sh | 15 +++++++++++++--
2 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/builtin-checkout.c b/builtin-checkout.c
index d99c1c0..411cc51 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -460,7 +460,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
*
* case 3: git checkout <something> [<paths>]
*
- * <something> shall not be ambiguous.
+ * With no paths, if <something> is a commit, that is to
+ * switch to the branch or detach HEAD at it.
+ *
+ * Otherwise <something> shall 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.
@@ -474,7 +477,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
}
arg = argv[0];
- has_dash_dash = argc > 1 && !strcmp(argv[1], "--");
+ has_dash_dash = (argc > 1) && !strcmp(argv[1], "--");
if (get_sha1(arg, rev)) {
if (has_dash_dash) /* case (1) */
@@ -500,8 +503,16 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
if (!source_tree) /* case (1): want a tree */
die("reference is not a tree: %s", arg);
- if (!has_dash_dash) /* case (3 -> 1) */
- verify_non_filename(NULL, arg);
+ if (!has_dash_dash) {/* case (3 -> 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)
+ verify_non_filename(NULL, arg);
+ }
else {
argv++;
argc--;
diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh
index 50d1f43..7cc0a35 100755
--- a/t/t2010-checkout-ambiguous.sh
+++ b/t/t2010-checkout-ambiguous.sh
@@ -32,8 +32,19 @@ test_expect_success 'non ambiguous call' ...It sounds really reasonable, and your patch seems really fine. --=20 =C2=B7O=C2=B7 Pierre Habouzit =C2=B7=C2=B7O madcoder@debia= n.org OOO http://www.madism.org
'git checkout' uses '--' to separate options from paths, but it was not mentioned in the documentation Signed-off-by: SZEDER Gábor <szeder@ira.uka.de> --- And here are two small changes related to 'git checkout --'. Documentation/git-checkout.txt | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt index 2abfbda..5aa69c0 100644 --- a/Documentation/git-checkout.txt +++ b/Documentation/git-checkout.txt @@ -9,7 +9,7 @@ SYNOPSIS -------- [verse] 'git checkout' [-q] [-f] [[--track | --no-track] -b <new_branch> [-l]] [-m] [<branch>] -'git checkout' [<tree-ish>] <paths>... +'git checkout' [<tree-ish>] [--] <paths>... DESCRIPTION ----------- -- 1.6.0.rc0.35.gb83a7 --
Commit d773c631 (bash: offer only paths after '--', 2008-07-08) did the
same for several other git commands, but 'git checkout' went unnoticed.
Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
contrib/completion/git-completion.bash | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 2edb341..9b51fda 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -626,6 +626,8 @@ _git_bundle ()
_git_checkout ()
{
+ __git_has_doubledash && return
+
__gitcomp "$(__git_refs)"
}
--
1.6.0.rc0.35.gb83a7
--
