{"thread":{"id":"33720","subject":"[PATCH v2 0/2] Show suggested refs when ref unknown","startedAt":"2013-05-04T00:04:18Z","lastAt":"2013-05-09T21:04:22Z","messageCount":5,"participants":["Vikrant Varma","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"216384","messageId":"1367625860-20746-1-git-send-email-vikrant.varma94@gmail.com","threadId":"33720","inReplyTo":null,"subject":"[PATCH v2 0/2] Show suggested refs when ref unknown","fromName":"Vikrant Varma","fromEmail":"vikrant.varma94@gmail.com","sentAt":"2013-05-04T00:04:18Z","receivedAt":"2013-05-04T00:04:18Z","isPatch":true,"sender":{"key":"vikrant.varma94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1186303?v=4"},"body":"If origin/foo exists, but foo doesn't:\n\n $ git merge foo\n fatal: foo - not something we can merge\n\nThis patch series improves the error message. If a remote branch exists with the\nsame name, it now says:\n\n $ git merge foo\n fatal: foo - not something we can merge\n\n Did you mean this?\n \t origin/foo\n\nIt does this by adding a new help function, help_unknown_ref, that takes care of\nprinting the more friendly error message, and modifies builtin/merge.c to use it.\n\nThis function can easily be used by other operations involving refs that don't\nexist instead of providing blanket failure error messages (eg. git checkout foo).\n\nVikrant Varma (2):\n help: add help_unknown_ref\n merge: use help_unknown_ref\n\n builtin/merge.c | 3 ++-\n help.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++\n help.h | 5 +++++\n 3 files changed, 57 insertions(+), 1 deletion(-)\n\n-- \n1.7.10.4\n"},{"id":"216385","messageId":"1367625860-20746-2-git-send-email-vikrant.varma94@gmail.com","threadId":"33720","inReplyTo":"1367625860-20746-1-git-send-email-vikrant.varma94@gmail.com","subject":"[PATCH v2 1/2] help: add help_unknown_ref","fromName":"Vikrant Varma","fromEmail":"vikrant.varma94@gmail.com","sentAt":"2013-05-04T00:04:19Z","receivedAt":"2013-05-04T00:04:19Z","isPatch":true,"sender":{"key":"vikrant.varma94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1186303?v=4"},"body":"When a ref is not known, currently functions call die() with an error message.\nAdd helper function help_unknown_ref to take care of displaying an error\nmessage along with a list of suggested refs the user might have meant.\n\nExample:\n\t$ git merge foo\n\tmerge: foo - not something we can merge\n\n\tDid you mean one of these?\n\t origin/foo\n\t upstream/foo\n\nSigned-off-by: Vikrant Varma \n---\n help.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++\n help.h | 5 +++++\n 2 files changed, 55 insertions(+)\n\ndiff --git a/help.c b/help.c\nindex 02ba043..fe5fe67 100644\n--- a/help.c\n+++ b/help.c\n@@ -7,6 +7,7 @@\n #include \"string-list.h\"\n #include \"column.h\"\n #include \"version.h\"\n+#include \"refs.h\"\n \n void add_cmdname(struct cmdnames *cmds, const char *name, int len)\n {\n@@ -404,3 +405,52 @@ int cmd_version(int argc, const char **argv, const char *prefix)\n \tprintf(\"git version %s\\n\", git_version_string);\n \treturn 0;\n }\n+\n+struct similar_ref_cb {\n+\tconst char *base_ref;\n+\tstruct string_list *similar_refs;\n+};\n+\n+static int append_similar_ref(const char *refname, const unsigned char *sha1,\n+\t\t\t int flags, void *cb_data)\n+{\n+\tstruct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data);\n+\tchar *branch = strrchr(refname, '/') + 1;\n+\t/* A remote branch of the same name is deemed similar */\n+\tif (!prefixcmp(refname, \"refs/remotes/\") &&\n+\t !strcmp(branch, cb->base_ref))\n+\t\tstring_list_append(cb->similar_refs,\n+\t\t\t\t refname + strlen(\"refs/remotes/\"));\n+\treturn 0;\n+}\n+\n+struct string_list guess_refs(const char *ref)\n+{\n+\tstruct similar_ref_cb ref_cb;\n+\tstruct string_list similar_refs = STRING_LIST_INIT_NODUP;\n+\n+\tref_cb.base_ref = ref;\n+\tref_cb.similar_refs = &similar_refs;\n+\tfor_each_ref(append_similar_ref, &ref_cb);\n+\treturn similar_refs;\n+}\n+\n+void help_unknown_ref(const char *ref, const char *cmd, const char *error)\n+{\n+\tint i;\n+\tstruct string_list suggested_refs = guess_refs(ref);\n+\n+\tfprintf_ln(stderr, _(\"%s: %s - %s\"), cmd, ref, error);\n+\n+\tif (suggested_refs.nr > 0) {\n+\t\tfprintf_ln(stderr,\n+\t\t\t Q_(\"\\nDid you mean this?\",\n+\t\t\t \"\\nDid you mean one of these?\",\n+\t\t\t suggested_refs.nr));\n+\t\tfor (i = 0; i < suggested_refs.nr; i++)\n+\t\t\tfprintf(stderr, \"\\t%s\\n\", suggested_refs.items[i].string);\n+\t}\n+\n+\tstring_list_clear(&suggested_refs, 0);\n+\texit(1);\n+}\ndiff --git a/help.h b/help.h\nindex 0ae5a12..5003423 100644\n--- a/help.h\n+++ b/help.h\n@@ -27,4 +27,9 @@ extern void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);\n extern int is_in_cmdlist(struct cmdnames *cmds, const char *name);\n extern void list_commands(unsigned int colopts, struct cmdnames *main_cmds, struct cmdnames *other_cmds);\n \n+/*\n+ * This function has been called because ref is not known.\n+ * Print a list of refs that the user might have meant, and exit.\n+ */\n+extern void help_unknown_ref(const char *ref, const char *cmd, const char *error);\n #endif /* HELP_H */\n-- \n1.7.10.4\n"},{"id":"216386","messageId":"1367625860-20746-3-git-send-email-vikrant.varma94@gmail.com","threadId":"33720","inReplyTo":"1367625860-20746-1-git-send-email-vikrant.varma94@gmail.com","subject":"[PATCH v2 2/2] merge: use help_unknown_ref","fromName":"Vikrant Varma","fromEmail":"vikrant.varma94@gmail.com","sentAt":"2013-05-04T00:04:20Z","receivedAt":"2013-05-04T00:04:20Z","isPatch":true,"sender":{"key":"vikrant.varma94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1186303?v=4"},"body":"Use help.c:help_unknown_ref instead of die to provide a friendlier error\nmessage before exiting, when one of the refs specified in a merge is unknown.\n\nSigned-off-by: Vikrant Varma \n---\n builtin/merge.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 3e2daa3..2ebe732 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1054,7 +1054,8 @@ static struct commit_list *collect_parents(struct commit *head_commit,\n \tfor (i = 0; i < argc; i++) {\n \t\tstruct commit *commit = get_merge_parent(argv[i]);\n \t\tif (!commit)\n-\t\t\tdie(_(\"%s - not something we can merge\"), argv[i]);\n+\t\t\thelp_unknown_ref(argv[i], \"merge\",\n+\t\t\t\t\t \"not something we can merge\");\n \t\tremotes = &commit_list_insert(commit, remotes)->next;\n \t}\n \t*remotes = NULL;\n-- \n1.7.10.4\n"},{"id":"216731","messageId":"7vzjw5axzk.fsf@alter.siamese.dyndns.org","threadId":"33720","inReplyTo":"1367625860-20746-2-git-send-email-vikrant.varma94@gmail.com","subject":"Re: [PATCH v2 1/2] help: add help_unknown_ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-08T22:49:19Z","receivedAt":"2013-05-08T22:49:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vikrant Varma writes:\n\n> When a ref is not known, currently functions call die() with an\n> error message.\n\nThe first part read somewhat awkward, so I started rewriting the\nabove like so:\n\n When the user gives an unknown string to a command that expects\n to see a ref, we could be more helpful than just saying \"that's\n not a ref\" and die.\n\nwhich in turn made me realize that some commands may not even know\nif the user mistyped a ref. It is not an objection to this patch\nper-se, but a useful future enhancement may be to allow the callers\ncall guess_mistyped_ref() directly and let them decide what to do\nwhen they suspect the string they did not understand is not a\nmistyped ref but something else, i.e. not let help_unknown_ref() die\nunconditionally but allow it to return. Then the caller can do:\n\n commit = get_commit_from_string(argv[i]);\n if (!commit) {\n ... I do not understand argv[i], but ...\n ... it may be a mistyped ref ...\n help_unknown_ref(argv[i], \"expected a revision\");\n ... it is not likely to be a typo ...\n ... perhaps it was meant to be a filename? ...\n if (file_exists(argv[i])) {\n ... yes! ...\n ... do the \"file\" thing instead ...\n }\n }\n\n> Add helper function help_unknown_ref to take care of displaying an\n> error message along with a list of suggested refs the user might\n> have meant.\n\nOK.\n\n> Example:\n> \t$ git merge foo\n> \tmerge: foo - not something we can merge\n\nThat leading \"merge: \" looks somewhat strange, especially when it\nimmediately follows the command line to invoke \"merge\", making it\nappear to waste space by stating the obvious.\n\nOur messages are generally marked with \"error:\", \"fatal:\",\n\"warning:\", etc. at the beginning.\n\n>\n> \tDid you mean one of these?\n> \t origin/foo\n> \t upstream/foo\n>\n> Signed-off-by: Vikrant Varma \n\n> ...\n> +struct string_list guess_refs(const char *ref)\n> +{\n> +\tstruct similar_ref_cb ref_cb;\n> +\tstruct string_list similar_refs = STRING_LIST_INIT_NODUP;\n> +\n> +\tref_cb.base_ref = ref;\n> +\tref_cb.similar_refs = &similar_refs;\n> +\tfor_each_ref(append_similar_ref, &ref_cb);\n> +\treturn similar_refs;\n> +}\n> +\n> +void help_unknown_ref(const char *ref, const char *cmd, const char *error)\n> +{\n> +\tint i;\n> +\tstruct string_list suggested_refs = guess_refs(ref);\n> +\n> +\tfprintf_ln(stderr, _(\"%s: %s - %s\"), cmd, ref, error);\n> +\n> +\tif (suggested_refs.nr > 0) {\n> +\t\tfprintf_ln(stderr,\n> +\t\t\t Q_(\"\\nDid you mean this?\",\n> +\t\t\t \"\\nDid you mean one of these?\",\n> +\t\t\t suggested_refs.nr));\n> +\t\tfor (i = 0; i < suggested_refs.nr; i++)\n> +\t\t\tfprintf(stderr, \"\\t%s\\n\", suggested_refs.items[i].string);\n> +\t}\n> +\n> +\tstring_list_clear(&suggested_refs, 0);\n> +\texit(1);\n> +}\n\nLooks sensible.\n\n> diff --git a/help.h b/help.h\n> index 0ae5a12..5003423 100644\n> --- a/help.h\n> +++ b/help.h\n> @@ -27,4 +27,9 @@ extern void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);\n> extern int is_in_cmdlist(struct cmdnames *cmds, const char *name);\n> extern void list_commands(unsigned int colopts, struct cmdnames *main_cmds, struct cmdnames *other_cmds);\n> \n> +/*\n> + * This function has been called because ref is not known.\n> + * Print a list of refs that the user might have meant, and exit.\n> + */\n\nThe wording is a bit funny; I'll amend it somehow before queuing.\n\n> +extern void help_unknown_ref(const char *ref, const char *cmd, const char *error);\n> #endif /* HELP_H */\n"},{"id":"216817","messageId":"518C0F56.7070600@gmail.com","threadId":"33720","inReplyTo":"7vzjw5axzk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/2] help: add help_unknown_ref","fromName":"Vikrant Varma","fromEmail":"vikrant.varma94@gmail.com","sentAt":"2013-05-09T21:04:22Z","receivedAt":"2013-05-09T21:04:22Z","isPatch":true,"sender":{"key":"vikrant.varma94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1186303?v=4"},"body":"On Thursday 09 May 2013 04:19 AM, Junio C Hamano wrote:\n > [...] which in turn made me realize that some commands may not even know\n > if the user mistyped a ref. It is not an objection to this patch\n > per-se, but a useful future enhancement may be to allow the callers\n > call guess_mistyped_ref() directly and let them decide what to do\n > when they suspect the string they did not understand is not a\n > mistyped ref but something else, i.e. not let help_unknown_ref() die\n > unconditionally but allow it to return. Then the caller can do:\n >\n > commit = get_commit_from_string(argv[i]);\n > if (!commit) {\n > ... I do not understand argv[i], but ...\n > ... it may be a mistyped ref ...\n > help_unknown_ref(argv[i], \"expected a revision\");\n > ... it is not likely to be a typo ...\n > ... perhaps it was meant to be a filename? ...\n > if (file_exists(argv[i])) {\n > ... yes! ...\n > ... do the \"file\" thing instead ...\n > }\n > }\n >\n\nI'm apprehensive about calling guess_mistyped_ref() (or it's equivalent, \nwhich happens to be guess_refs()) directly, because it doesn't seem like \na clean enough separation. When the caller thinks it's got a bad \nrefname, it should just hand it over to help_unknown_ref, for further \nprocessing.\n\nIf autocorrect is enabled, it can get back a single corrected refname \n(that is what my next patch will include - is it okay to base it on pu?).\n\nIf the need ever did arise to get that kind of information from \nhelp_unknown_ref, it could always be done using callback data?\n\n\tcommit = get_commit_from_string(argv[i]);\n\tif (!commit) {\n\t\t... maybe mistyped ref, maybe something else ...\n\t\tstruct unknown_ref_cb data;\n\t\thelp_unknown_ref(argv[i], \"expected something else\",\n\t\t\t\t &data);\n\t\tif (data.autocorrect)\n\t\t\tcommit = get_commit_from_string(\n\t\t\t\t\tdata.corrected_ref);\n\t\telse if (data.is_file)\n\t\t\t... do the file thing instead ...\n \t}\n\nI didn't see the need for this right away.\n\n >> Example:\n >> \t$ git merge foo\n >> \tmerge: foo - not something we can merge\n >\n > That leading \"merge: \" looks somewhat strange, especially when it\n > immediately follows the command line to invoke \"merge\", making it\n > appear to waste space by stating the obvious.\n >\n > Our messages are generally marked with \"error:\", \"fatal:\",\n > \"warning:\", etc. at the beginning.\n\nI agree, it looks strange. However the alternatives seem to be:\n\n1) hard code 'fatal' into the error message\n2) print the corrections before using die()\n3) create and store the corrections string beforehand, and then call die()\n\n1 and 3 are not elegant, and 2's output seems harder to read. I haven't \nbeen able to figure out a way to do this well.\n"}]}