[PATCH] checkout: warn users if action is taken in ambiguous situation

[Nguyễn Thái Ngọc Duy]

git checkout foo (no more arguments) always checks out existing
branch foo even if foo exists on working directory. To avoid
confusion to users who do not know this exception, say something along
the action.

Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
 Personally I disagree with the comment above the top chunk about
 stopping annoying users in this case. I think the annoyance is a good
 sign for them to rename their branches to something more sensible.

 builtin/checkout.c |  6 --
 cache.h|  2 ++
 setup.c| 21 +
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 7d922c6..7bc4e8a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -899,8 +899,10 @@ 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)
-   verify_non_filename(NULL, arg);
+   const char *action = NULL;
+   if (!argc)
+   action = _(Checked out the new branch.);
+   verify_non_filename_gently(NULL, arg, action);
} else {
argcount++;
argv++;
diff --git a/cache.h b/cache.h
index 95daa69..95f11df 100644
--- a/cache.h
+++ b/cache.h
@@ -404,6 +404,8 @@ extern void verify_filename(const char *prefix,
const char *name,
int diagnose_misspelt_rev);
 extern void verify_non_filename(const char *prefix, const char *name);
+extern void verify_non_filename_gently(const char *prefix, const char *name,
+  const char *preferred_action);
 extern int path_inside_repo(const char *prefix, const char *path);
 
 #define INIT_DB_QUIET 0x0001
diff --git a/setup.c b/setup.c
index 3a1b2fd..4b776cf 100644
--- a/setup.c
+++ b/setup.c
@@ -133,7 +133,8 @@ void verify_filename(const char *prefix,
  * and we parsed the arg as a refname.  It should not be interpretable
  * as a filename.
  */
-void verify_non_filename(const char *prefix, const char *arg)
+void verify_non_filename_gently(const char *prefix, const char *arg,
+   const char *preferred_action)
 {
if (!is_inside_work_tree() || is_inside_git_dir())
return;
@@ -141,9 +142,21 @@ void verify_non_filename(const char *prefix, const char 
*arg)
return; /* flag */
if (!check_filename(prefix, arg))
return;
-   die(ambiguous argument '%s': both revision and filename\n
-   Use '--' to separate paths from revisions, like this:\n
-   'git command [revision...] -- [file...]', arg);
+   if (!preferred_action)
+   die(_(ambiguous argument '%s': both revision and filename\n
+ Use '--' to separate paths from revisions, like this:\n
+ 'git command [revision...] -- [file...]'), arg);
+   else {
+   warning(_(ambiguous argument '%s': both revision and 
filename\n
+ Use '--' to separate paths from revisions, like 
this:\n
+ 'git command [revision...] -- [file...]'), 
arg);
+   warning(%s, preferred_action);
+   }
+}
+
+void verify_non_filename(const char *prefix, const char *arg)
+{
+   verify_non_filename_gently(prefix, arg, NULL);
 }
 
 /*
-- 
1.7.12.rc2.18.g61b472e

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] checkout: warn users if action is taken in ambiguous situation

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 git checkout foo (no more arguments) always checks out existing
 branch foo even if foo exists on working directory. To avoid
 confusion to users who do not know this exception, say something along
 the action.

I do not think the extra noise Checked out the new branch. your
patch adds is necessary:

$ git checkout master
Already on 'master'
$ next
$ git checkout next
Switched to branch 'next'

If the user wanted to grab the contents from the index for path
'next' and deposit it to the file 'next' in the working tree, we are
already telling the user that that is not what happened.

Even though I agree with you that it is a good goal to help users
who want to disambiguate between branch and path differently from
what the annoyance avoidance does, I do not think butchering the
annoyance avoidance in parse_branchname_arg() is the right way to do
so.

I wouldn't mind if the above interaction went like this:

$ git checkout next
Switched to branch 'next'
hint: if you meant to check out the contents of 'next' out of
hint: the index to the file 'next' in your working tree, say
hint: git checkout -- next instead.  To squelch this advice,
hint: git config set advice.checkoutWarnFileAndBranchName false.

This should trigger only when the advice is not set (or set to
true), and only when you have 'next' in the index.  Presense or
absense of 'next' in the working tree should not make a difference.

And doing it that way covers anoter valid case in which the user
would want the exact same help from Git that your patch would not
cover:

$ git branch Makefile
$ git checkout master
Already on 'master'
$ rm Makefile
$ git checkout Makefile
D   Makefile
Switched to branch 'Makefile'

Another thing to look out for is this case:

$ git branch Makefile master
$ git checkout next
Already on 'next'
$ rm Makefile
$ echo archive.c ;# different between master and next
$ git checkout Makefile
error: Your local changes to the following files would be overwritten by 
checkout:
archive.c
Please, commit your changes or stash them before you can switch branches.
Aborting

We would want the exact same hint (with the obvious s/next/Makefile/) after
the above output.

Just for a reference, with your patch, I think the output would like
this (I didn't apply nor run it):

$ git checkout Makefile
warning: ambiguous argument 'Makefile': both revision and filename
Use '--' to separate paths from revisions, like this:
'git command [reviosion] -- [file...]'
warning: checked out the new branch.
error: Your local changes...
archive.c
Please, commit your ...
Aborting

which would be far worse and confusing.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html