{"thread":{"id":"31397","subject":"[PATCH] checkout: warn users if action is taken in ambiguous situation","startedAt":"2012-08-31T14:38:54Z","lastAt":"2012-08-31T18:27:42Z","messageCount":2,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"198146","messageId":"1346423934-7003-1-git-send-email-pclouds@gmail.com","threadId":"31397","inReplyTo":null,"subject":"[PATCH] checkout: warn users if action is taken in ambiguous situation","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-08-31T14:38:54Z","receivedAt":"2012-08-31T14:38:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\"git checkout foo\" (no more arguments) always checks out existing\nbranch \"foo\" even if \"foo\" exists on working directory. To avoid\nconfusion to users who do not know this exception, say something along\nthe action.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Personally I disagree with the comment above the top chunk about\n stopping annoying users in this case. I think the annoyance is a good\n sign for them to rename their branches to something more sensible.\n\n builtin/checkout.c |  6 ++++--\n cache.h            |  2 ++\n setup.c            | 21 +++++++++++++++++----\n 3 files changed, 23 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 7d922c6..7bc4e8a 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -899,8 +899,10 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\t * even if there happen to be a file called 'branch';\n \t\t * it would be extremely annoying.\n \t\t */\n-\t\tif (argc)\n-\t\t\tverify_non_filename(NULL, arg);\n+\t\tconst char *action = NULL;\n+\t\tif (!argc)\n+\t\t\taction = _(\"Checked out the new branch.\");\n+\t\tverify_non_filename_gently(NULL, arg, action);\n \t} else {\n \t\targcount++;\n \t\targv++;\ndiff --git a/cache.h b/cache.h\nindex 95daa69..95f11df 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -404,6 +404,8 @@ extern void verify_filename(const char *prefix,\n \t\t\t    const char *name,\n \t\t\t    int diagnose_misspelt_rev);\n extern void verify_non_filename(const char *prefix, const char *name);\n+extern void verify_non_filename_gently(const char *prefix, const char *name,\n+\t\t\t\t       const char *preferred_action);\n extern int path_inside_repo(const char *prefix, const char *path);\n \n #define INIT_DB_QUIET 0x0001\ndiff --git a/setup.c b/setup.c\nindex 3a1b2fd..4b776cf 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -133,7 +133,8 @@ void verify_filename(const char *prefix,\n  * and we parsed the arg as a refname.  It should not be interpretable\n  * as a filename.\n  */\n-void verify_non_filename(const char *prefix, const char *arg)\n+void verify_non_filename_gently(const char *prefix, const char *arg,\n+\t\t\t\tconst char *preferred_action)\n {\n \tif (!is_inside_work_tree() || is_inside_git_dir())\n \t\treturn;\n@@ -141,9 +142,21 @@ void verify_non_filename(const char *prefix, const char *arg)\n \t\treturn; /* flag */\n \tif (!check_filename(prefix, arg))\n \t\treturn;\n-\tdie(\"ambiguous argument '%s': both revision and filename\\n\"\n-\t    \"Use '--' to separate paths from revisions, like this:\\n\"\n-\t    \"'git <command> [<revision>...] -- [<file>...]'\", arg);\n+\tif (!preferred_action)\n+\t\tdie(_(\"ambiguous argument '%s': both revision and filename\\n\"\n+\t\t      \"Use '--' to separate paths from revisions, like this:\\n\"\n+\t\t      \"'git <command> [<revision>...] -- [<file>...]'\"), arg);\n+\telse {\n+\t\twarning(_(\"ambiguous argument '%s': both revision and filename\\n\"\n+\t\t\t  \"Use '--' to separate paths from revisions, like this:\\n\"\n+\t\t\t  \"'git <command> [<revision>...] -- [<file>...]'\"), arg);\n+\t\twarning(\"%s\", preferred_action);\n+\t}\n+}\n+\n+void verify_non_filename(const char *prefix, const char *arg)\n+{\n+\tverify_non_filename_gently(prefix, arg, NULL);\n }\n \n /*\n-- \n1.7.12.rc2.18.g61b472e\n"},{"id":"198162","messageId":"7vd326or1d.fsf@alter.siamese.dyndns.org","threadId":"31397","inReplyTo":"1346423934-7003-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] checkout: warn users if action is taken in ambiguous situation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-31T18:27:42Z","receivedAt":"2012-08-31T18:27:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> \"git checkout foo\" (no more arguments) always checks out existing\n> branch \"foo\" even if \"foo\" exists on working directory. To avoid\n> confusion to users who do not know this exception, say something along\n> the action.\n\nI do not think the extra noise \"Checked out the new branch.\" your\npatch adds is necessary:\n\n    $ git checkout master\n    Already on 'master'\n    $ >next\n    $ git checkout next\n    Switched to branch 'next'\n\nIf the user wanted to grab the contents from the index for path\n'next' and deposit it to the file 'next' in the working tree, we are\nalready telling the user that that is not what happened.\n\nEven though I agree with you that it is a good goal to help users\nwho want to disambiguate between branch and path differently from\nwhat the annoyance avoidance does, I do not think butchering the\nannoyance avoidance in parse_branchname_arg() is the right way to do\nso.\n\nI wouldn't mind if the above interaction went like this:\n\n    $ git checkout next\n    Switched to branch 'next'\n    hint: if you meant to check out the contents of 'next' out of\n    hint: the index to the file 'next' in your working tree, say\n    hint: \"git checkout -- next\" instead.  To squelch this advice,\n    hint: \"git config set advice.checkoutWarnFileAndBranchName false\".\n\nThis should trigger only when the advice is not set (or set to\ntrue), and only when you have 'next' in the index.  Presense or\nabsense of 'next' in the working tree should not make a difference.\n\nAnd doing it that way covers anoter valid case in which the user\nwould want the exact same help from Git that your patch would not\ncover:\n\n    $ git branch Makefile\n    $ git checkout master\n    Already on 'master'\n    $ rm Makefile\n    $ git checkout Makefile\n    D\tMakefile\n    Switched to branch 'Makefile'\n\nAnother thing to look out for is this case:\n\n    $ git branch Makefile master\n    $ git checkout next\n    Already on 'next'\n    $ rm Makefile\n    $ echo >>archive.c ;# different between master and next\n    $ git checkout Makefile\n    error: Your local changes to the following files would be overwritten by checkout:\n        archive.c\n    Please, commit your changes or stash them before you can switch branches.\n    Aborting\n\nWe would want the exact same hint (with the obvious s/next/Makefile/) after\nthe above output.\n\nJust for a reference, with your patch, I think the output would like\nthis (I didn't apply nor run it):\n\n    $ git checkout Makefile\n    warning: ambiguous argument 'Makefile': both revision and filename\n    Use '--' to separate paths from revisions, like this:\n    'git <command> [<reviosion>....] -- [<file>...]'\n    warning: checked out the new branch.\n    error: Your local changes...\n        archive.c\n    Please, commit your ...\n    Aborting\n\nwhich would be far worse and confusing.\n"}]}