{"thread":{"id":"33694","subject":"[PATCH 0/2] Better advice on merge","startedAt":"2013-05-01T11:22:05Z","lastAt":"2013-05-01T23:07:33Z","messageCount":16,"participants":["Vikrant Varma","Ramkumar Ramachandra","Matthieu Moy","Jonathan Nieder","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"216139","messageId":"1367407327-5216-1-git-send-email-vikrant.varma94@gmail.com","threadId":"33694","inReplyTo":null,"subject":"[PATCH 0/2] Better advice on merge","fromName":"Vikrant Varma","fromEmail":"vikrant.varma94@gmail.com","sentAt":"2013-05-01T11:22:05Z","receivedAt":"2013-05-01T11:22:05Z","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 \nwith the same 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 \ncare of printing the more friendly error message, and modifies builtin/merge.c\nto use it. In the future, this could also be extended to other operations\ninvolving branches that don't exist locally, instead of providing blanket\nfailure error messages (eg. git checkout foo).\n\nVikrant Varma (2):\n help: add help_unknown_ref\n merge: use help_unknown_ref instead of die\n\n builtin/merge.c | 4 ++--\n help.c | 44 ++++++++++++++++++++++++++++++++++++++++++++\n help.h | 6 ++++++\n 3 files changed, 52 insertions(+), 2 deletions(-)\n\n-- \n1.8.3-rc0\n"},{"id":"216140","messageId":"1367407327-5216-2-git-send-email-vikrant.varma94@gmail.com","threadId":"33694","inReplyTo":"1367407327-5216-1-git-send-email-vikrant.varma94@gmail.com","subject":"[PATCH 1/2] help: add help_unknown_ref","fromName":"Vikrant Varma","fromEmail":"vikrant.varma94@gmail.com","sentAt":"2013-05-01T11:22:06Z","receivedAt":"2013-05-01T11:22:06Z","isPatch":true,"sender":{"key":"vikrant.varma94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1186303?v=4"},"body":"Give better advice when trying to merge a branch that doesn't exist. If\nthe branch exists in any remotes, display a list of suggestions.\n\nExample:\n\t$ git merge foo\n\tfatal: foo - not something we can merge\n\n\tDid you mean this?\n\t bar/foo\n\nSigned-off-by: Vikrant Varma \n---\n help.c | 44 ++++++++++++++++++++++++++++++++++++++++++++\n help.h | 6 ++++++\n 2 files changed, 50 insertions(+)\n\ndiff --git a/help.c b/help.c\nindex 02ba043..faf18b9 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,46 @@ 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+ const char *base_ref;\n+ struct string_list similar_refs;\n+};\n+\n+static int append_similar_ref(const char* refname, const unsigned char *sha1, int flags, void *cb_data)\n+{\n+ int i;\n+ struct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data);\n+ for (i = strlen(refname); refname[i] != '/'; i--)\n+\t ;\n+ /* A remote branch of the same name is deemed similar */\n+ if (!prefixcmp(refname, \"refs/remotes/\") && !strcmp(refname + i + 1, cb->base_ref))\n+\t \tstring_list_append(&(cb->similar_refs), refname + 13);\n+ return 0;\n+}\n+\n+void help_unknown_ref(const char* ref) {\n+ int i;\n+ struct similar_ref_cb ref_cb;\n+ ref_cb.similar_refs = (struct string_list)STRING_LIST_INIT_NODUP;\n+ ref_cb.base_ref = ref;\n+\n+ for_each_ref(append_similar_ref, &ref_cb);\n+\n+ fprintf_ln(stderr, _(\"fatal: %s - not something we can merge\"), ref);\n+\n+ if (ref_cb.similar_refs.nr > 0) {\n+\t fprintf_ln(stderr,\n+\t\t Q_(\"\\nDid you mean this?\",\n+\t\t \"\\nDid you mean one of these?\",\n+\t\t ref_cb.similar_refs.nr));\n+\n+\t for (i = 0; i < ref_cb.similar_refs.nr; i++)\n+\t\t fprintf(stderr, \"\\t%s\\n\", ref_cb.similar_refs.items[i].string);\n+ }\n+ exit(1);\n+}\n+\t\n+\t\t\t\n+\n+\ndiff --git a/help.h b/help.h\nindex 0ae5a12..613cb45 100644\n--- a/help.h\n+++ b/help.h\n@@ -27,4 +27,10 @@ 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+ * ref is not something that can be merged. Print a list of remote branches of the\n+ * same name that the user might have meant. \n+ */\n+extern void help_unknown_ref(const char* ref);\n+\n #endif /* HELP_H */\n-- \n1.8.3-rc0\n"},{"id":"216141","messageId":"1367407327-5216-3-git-send-email-vikrant.varma94@gmail.com","threadId":"33694","inReplyTo":"1367407327-5216-1-git-send-email-vikrant.varma94@gmail.com","subject":"[PATCH 2/2] merge: use help_unknown_ref instead of die","fromName":"Vikrant Varma","fromEmail":"vikrant.varma94@gmail.com","sentAt":"2013-05-01T11:22:07Z","receivedAt":"2013-05-01T11:22:07Z","isPatch":true,"sender":{"key":"vikrant.varma94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1186303?v=4"},"body":"The previous patch added help_unknown_ref to print a more helpful error \nmessage when trying to merge a branch that doesn't exist, by printing a \nlist of remote branches the user might have meant. Use it.\n\nSigned-off-by: Vikrant Varma \n---\n builtin/merge.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 3e2daa3..0f1f39b 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1053,8 +1053,8 @@ static struct commit_list *collect_parents(struct commit *head_commit,\n \t\tremotes = &commit_list_insert(head_commit, remotes)->next;\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\tif (!commit) \n+\t\t help_unknown_ref(argv[i]);\t\t \n \t\tremotes = &commit_list_insert(commit, remotes)->next;\n \t}\n \t*remotes = NULL;\n-- \n1.8.3-rc0\n"},{"id":"216142","messageId":"CALkWK0nMMi-nmAMUGXCaJDCV29G3dOzYTosKqSw+bFzc0osiaA@mail.gmail.com","threadId":"33694","inReplyTo":"1367407327-5216-2-git-send-email-vikrant.varma94@gmail.com","subject":"Re: [PATCH 1/2] help: add help_unknown_ref","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-01T12:23:35Z","receivedAt":"2013-05-01T12:23:35Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Vikrant Varma wrote:\n> Give better advice when trying to merge a branch that doesn't exist. If\n> the branch exists in any remotes, display a list of suggestions.\n\nInteresting. Thanks for working on this.\n\nYou say advice, but you're not invoking advise() or guarding the\nadvice with an advice.* -- the advice is undoubtedly helpful, but not\neveryone wants to see it.\n\n> diff --git a/help.c b/help.c\n> index 02ba043..faf18b9 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,46 @@ int cmd_version(int argc, const char **argv, const char *prefix)\n> printf(\"git version %s\\n\", git_version_string);\n> return 0;\n> }\n> +\n> +struct similar_ref_cb {\n\nI see that there are other structs in our codebase suffixing _cb, to\nindicate \"callback data\". I normally reserve _cb for callback\nfunctions.\n\n> + const char *base_ref;\n> + struct string_list similar_refs;\n\nOkay, so you might have more than one matching candidate.\n\n> +static int append_similar_ref(const char* refname, const unsigned char *sha1, int flags, void *cb_data)\n> +{\n> + int i;\n> + struct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data);\n> + for (i = strlen(refname); refname[i] != '/'; i--)\n> + ;\n\nEr, what is this? A re-implementation of strrchr()?\n\n> + /* A remote branch of the same name is deemed similar */\n> + if (!prefixcmp(refname, \"refs/remotes/\") && !strcmp(refname + i + 1, cb->base_ref))\n> + string_list_append(&(cb->similar_refs), refname + 13);\n\nWhat is 13? Please use strlen(\"refs/remotes/\") for readability.\n\nI don't like the + i + 1 thing, but you should be able to get rid of\nit with strrchr().\n\n> +void help_unknown_ref(const char* ref) {\n> + int i;\n> + struct similar_ref_cb ref_cb;\n> + ref_cb.similar_refs = (struct string_list)STRING_LIST_INIT_NODUP;\n\nWhy are you casting STRING_LIST_INIT_NODUP?\n\n> + ref_cb.base_ref = ref;\n> +\n> + for_each_ref(append_similar_ref, &ref_cb);\n> +\n> + fprintf_ln(stderr, _(\"fatal: %s - not something we can merge\"), ref);\n\nHm, \"fatal: \" was emitted by die() earlier. I wonder if something\nlike error() will be nicer than hard-coding \"fatal: \".\n\n> + if (ref_cb.similar_refs.nr > 0) {\n> + fprintf_ln(stderr,\n> + Q_(\"\\nDid you mean this?\",\n> + \"\\nDid you mean one of these?\",\n> + ref_cb.similar_refs.nr));\n\nHm, why did you use Q_?\n\n> + for (i = 0; i < ref_cb.similar_refs.nr; i++)\n> + fprintf(stderr, \"\\t%s\\n\", ref_cb.similar_refs.items[i].string);\n> + }\n> + exit(1);\n\ndie() exits with 128, no? Why are you exiting with 1 now?\n"},{"id":"216143","messageId":"vpqmwsen6b1.fsf@grenoble-inp.fr","threadId":"33694","inReplyTo":"CALkWK0nMMi-nmAMUGXCaJDCV29G3dOzYTosKqSw+bFzc0osiaA@mail.gmail.com","subject":"Re: [PATCH 1/2] help: add help_unknown_ref","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-05-01T14:06:10Z","receivedAt":"2013-05-01T14:06:10Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Ramkumar Ramachandra writes:\n\n> You say advice, but you're not invoking advise() or guarding the\n> advice with an advice.* -- the advice is undoubtedly helpful, but not\n> everyone wants to see it.\n\nQuite franckly, I don't see why anybody would need to disable it.\nadvice.* is very nice to disable messages that are shown in normal Git\nusage (e.g. running \"git status\" or attempting a non-fast forward push),\nbut this is clearly a user-error, that an advanced user is not supposed\nto see anyway.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"216165","messageId":"20130501182726.GL24467@google.com","threadId":"33694","inReplyTo":"1367407327-5216-1-git-send-email-vikrant.varma94@gmail.com","subject":"Re: [PATCH 0/2] Better advice on merge","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-05-01T18:27:26Z","receivedAt":"2013-05-01T18:27:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Vikrant Varma wrote:\n\n> If origin/foo exists, but foo doesn't:\n>\n> $ git merge foo\n> fatal: foo - not something we can merge\n>\n> This patch series improves the error message. If a remote branch exists \n> with the same 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\nFun. :)\n\nI haven't looked at the patches closely. My only immediate thoughts\nare:\n\n - It would be nice to add a test under t/, so we can be sure without\n manually testing it that this new feature doesn't break in the\n future. See t/README if interested. I guess it would be t7613.\n\n - Since the first patch isn't useful without or logically separate\n from the second, this would probably be easier to read as a single\n patch.\n\nThanks for your work, and hope that helps.\n\nSincerely,\nJonathan\n"},{"id":"216168","messageId":"7vppxa5z01.fsf@alter.siamese.dyndns.org","threadId":"33694","inReplyTo":"1367407327-5216-2-git-send-email-vikrant.varma94@gmail.com","subject":"Re: [PATCH 1/2] help: add help_unknown_ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-01T18:35:58Z","receivedAt":"2013-05-01T18:35:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vikrant Varma writes:\n\n> Give better advice when trying to merge a branch that doesn't exist. If\n> the branch exists in any remotes, display a list of suggestions.\n>\n> Example:\n> \t$ git merge foo\n> \tfatal: foo - not something we can merge\n>\n> \tDid you mean this?\n> \t bar/foo\n>\n> Signed-off-by: Vikrant Varma \n> ---\n\nNicely explained.\n\nIf you step back a bit, you would notice two things.\n\n (1) Saying 'foo' when the user means 'origin/foo' is hardly the\n only (or even the most common) kind of mistake that the code\n you need to add to 'git merge' would encounter and could help\n the user with. \"git merge origin/mastre\" and \"orign/master\"\n may benefit from a typofix as well, and the mechanism to come\n up with the suggestion is likely to hook to the same codepath\n you are modifying with this patch, even though the logic to\n come up with the suggested alternatives may be different.\n\n (2) \"merge\" is not the single command that user may make this kind\n of mistake the command could help and use the same helper.\n \"git branch myfoo foo\" may want to suggest \"origin/foo\", for\n example. I just typed \"git checkout mater\", which could have\n been easily corrected to \"git checkout master\" with a mechanism\n like this.\n\n> help.c | 44 ++++++++++++++++++++++++++++++++++++++++++++\n> help.h | 6 ++++++\n> 2 files changed, 50 insertions(+)\n>\n> diff --git a/help.c b/help.c\n> index 02ba043..faf18b9 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,46 @@ 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> + const char *base_ref;\n> + struct string_list similar_refs;\n> +};\n> +\n> +static int append_similar_ref(const char* refname, const unsigned char *sha1, int flags, void *cb_data)\n\nAn asterisk sticks to the parameter name, not type, like this:\n\n\t..._ref(const char *refname, ...\n\nThere are other places with the same style problem in this patch.\n\n> +{\n> + int i;\n> + struct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data);\n> + for (i = strlen(refname); refname[i] != '/'; i--)\n> +\t ;\n\nIndent with two HT, not HT followed by a run of SPs.\n\n> + /* A remote branch of the same name is deemed similar */\n> + if (!prefixcmp(refname, \"refs/remotes/\") && !strcmp(refname + i + 1, cb->base_ref))\n\nAn overlong line can and should be split, perhaps like this:\n\n\tif (!prefixcmp(... very long parameter list ...) &&\n !strcmp(... another very long parameter list ...))\n\n> +\t \tstring_list_append(&(cb->similar_refs), refname + 13);\n\nTo suggest \"orign/foo\" => \"origin/foo\", \"foz\" => \"origin/foo\", and\n\"mastre\" => \"master\", using levenshtein.c would help here.\n\nYou would special case the distance between \"foo\" and \"origin/foo\"\nas \"very low\", e.g. 0, and compute levenshtein distance with refname\nand cb->base_ref, store the result in the .util field of the string\nlist, and sort it at the end after you finish iterating using the\ncomputed distance to come up with the list of suggestions.\n\n> + return 0;\n> +}\n> +\n> +void help_unknown_ref(const char* ref) {\n> + int i;\n> + struct similar_ref_cb ref_cb;\n> + ref_cb.similar_refs = (struct string_list)STRING_LIST_INIT_NODUP;\n> + ref_cb.base_ref = ref;\n> +\n> + for_each_ref(append_similar_ref, &ref_cb);\n> +\n> + fprintf_ln(stderr, _(\"fatal: %s - not something we can merge\"), ref);\n\nWhen you consider the point (2) above, it becomes clear why this\nmessage does not belong to a helper function with a bland and\ngeneric name \"help unknown ref\".\n\nThis API is misdesigned. A possible alternative that may be better\nreusable would be to have a helper that is used to come up with a\nlist of suggestions and make the caller responsible for emitting the\nerror message.\n\n> + if (ref_cb.similar_refs.nr > 0) {\n> +\t fprintf_ln(stderr,\n> +\t\t Q_(\"\\nDid you mean this?\",\n> +\t\t \"\\nDid you mean one of these?\",\n> +\t\t ref_cb.similar_refs.nr));\n> +\n> +\t for (i = 0; i < ref_cb.similar_refs.nr; i++)\n> +\t\t fprintf(stderr, \"\\t%s\\n\", ref_cb.similar_refs.items[i].string);\n> + }\n> + exit(1);\n> +}\n> +\t\n> +\t\t\t\n> +\n> +\n\nDo not add trailing blank lines.\n\n> diff --git a/help.h b/help.h\n> index 0ae5a12..613cb45 100644\n> --- a/help.h\n> +++ b/help.h\n> @@ -27,4 +27,10 @@ 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> + * ref is not something that can be merged. Print a list of remote branches of the\n> + * same name that the user might have meant. \n> + */\n> +extern void help_unknown_ref(const char* ref);\n> +\n> #endif /* HELP_H */\n"},{"id":"216169","messageId":"7vip325yzq.fsf@alter.siamese.dyndns.org","threadId":"33694","inReplyTo":"1367407327-5216-3-git-send-email-vikrant.varma94@gmail.com","subject":"Re: [PATCH 2/2] merge: use help_unknown_ref instead of die","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-01T18:36:09Z","receivedAt":"2013-05-01T18:36:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vikrant Varma writes:\n\n> The previous patch added help_unknown_ref to print a more helpful error \n> message when trying to merge a branch that doesn't exist, by printing a \n> list of remote branches the user might have meant. Use it.\n>\n> Signed-off-by: Vikrant Varma \n> ---\n> builtin/merge.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 3e2daa3..0f1f39b 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -1053,8 +1053,8 @@ static struct commit_list *collect_parents(struct commit *head_commit,\n> \t\tremotes = &commit_list_insert(head_commit, remotes)->next;\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\tif (!commit) \n> +\t\t help_unknown_ref(argv[i]);\t\t \n\nThis calling site may become something like:\n\n\tif (!commit) {\n \tchar *suggestion;\n suggestion = guess_misspelled_ref(argv[i]);\n die(suggestion == NULL\n ? _(\"%s - not something we can merge\")\n : _(\"%s - not something we can merge\\n\"\n \"Perhaps you meant one of these?\\n\"\n \"%s\"), argv[i], suggestion);\n\t}\n\nif you really want to keep \"not something we can merge\" at the top.\n\nI however suspect that this might be easier for the reader.\n\n\tif (!commit) {\n \tstruct string_list *suggestion;\n suggestion = guess_misspelled_ref(argv[i]);\n if (suggestion)\n\t\t\tprint_string_list(suggestion,\n \t_(\"Perhaps you meant one of these?\"));\n \tdie(_(\"%s - not something we can merge\"), argv[i]);\n\t}\n\nNote that print_string_list() needs to be enhanced so that the\ncaller can tell it not to show the .util field if you go in this\ndirection.\n\n> \t\tremotes = &commit_list_insert(commit, remotes)->next;\n> \t}\n> \t*remotes = NULL;\n\nThanks.\n"},{"id":"216187","messageId":"51817319.6060201@gmail.com","threadId":"33694","inReplyTo":"CALkWK0nMMi-nmAMUGXCaJDCV29G3dOzYTosKqSw+bFzc0osiaA@mail.gmail.com","subject":"Re: [PATCH 1/2] help: add help_unknown_ref","fromName":"Vikrant Varma","fromEmail":"vikrant.varma94@gmail.com","sentAt":"2013-05-01T19:55:05Z","receivedAt":"2013-05-01T19:55:05Z","isPatch":true,"sender":{"key":"vikrant.varma94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1186303?v=4"},"body":"On 01-05-2013 17:53, Ramkumar Ramachandra wrote:\n> Vikrant Varma wrote:\n>> Give better advice when trying to merge a branch that doesn't exist. If\n>> the branch exists in any remotes, display a list of suggestions.\n>\n> Interesting. Thanks for working on this.\n>\n> You say advice, but you're not invoking advise() or guarding the\n> advice with an advice.* -- the advice is undoubtedly helpful, but not\n> everyone wants to see it.\n>\n\nI agree with Matthieu, the people who don't want to see this advice \nnever will, because they won't make that mistake. Maybe advice is the \nwrong word, corrections might be more appropriate.\n\n>> diff --git a/help.c b/help.c\n>> index 02ba043..faf18b9 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,46 @@ int cmd_version(int argc, const char **argv, const char *prefix)\n>> printf(\"git version %s\\n\", git_version_string);\n>> return 0;\n>> }\n>> +\n>> +struct similar_ref_cb {\n>\n> I see that there are other structs in our codebase suffixing _cb, to\n> indicate \"callback data\". I normally reserve _cb for callback\n> functions.\n\nI'm following the convention (builtin/merge.c: struct append_ref_cb). If \nthere's a better way to name it, I'll use that.\n\n>> +static int append_similar_ref(const char* refname, const unsigned char *sha1, int flags, void *cb_data)\n>> +{\n>> + int i;\n>> + struct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data);\n>> + for (i = strlen(refname); refname[i] != '/'; i--)\n>> + ;\n>\n> Er, what is this? A re-implementation of strrchr()?\n>\n\nOh so that's what it's called. Apologies, will fix this.\n\n>> + /* A remote branch of the same name is deemed similar */\n>> + if (!prefixcmp(refname, \"refs/remotes/\") && !strcmp(refname + i + 1, cb->base_ref))\n>> + string_list_append(&(cb->similar_refs), refname + 13);\n>\n> What is 13? Please use strlen(\"refs/remotes/\") for readability.\n>\n\nYes, will fix this too.\n\n>> +void help_unknown_ref(const char* ref) {\n>> + int i;\n>> + struct similar_ref_cb ref_cb;\n>> + ref_cb.similar_refs = (struct string_list)STRING_LIST_INIT_NODUP;\n>\n> Why are you casting STRING_LIST_INIT_NODUP?\n>\n>> + ref_cb.base_ref = ref;\n\n\nref_cb.similar_refs has already been defined. The compiler won't let me \nassign to it unless I cast first. However, I think compound literals are \na C99/gcc feature. Is this better?\n\n\tstruct similar_ref_cb ref_cb = {ref, STRING_LIST_INIT_NODUP};\n\n\n>> + if (ref_cb.similar_refs.nr > 0) {\n>> + fprintf_ln(stderr,\n>> + Q_(\"\\nDid you mean this?\",\n>> + \"\\nDid you mean one of these?\",\n>> + ref_cb.similar_refs.nr));\n>\n> Hm, why did you use Q_?\n>\n\nQ_ is a pluralization helper that picks one of the two strings based on \nref_cb.similar_refs.nr. It's used in help.c:help_unknown_cmd for the \nsame reason.\n\n>> + for (i = 0; i < ref_cb.similar_refs.nr; i++)\n>> + fprintf(stderr, \"\\t%s\\n\", ref_cb.similar_refs.items[i].string);\n>> + }\n>> + exit(1);\n>\n> die() exits with 128, no? Why are you exiting with 1 now?\n>\n\nAgain, because help_unknown_cmd exited with 1. I've tried to follow the \nconvention as laid down there. What's the significance of the error code \nfor die()? When is it correct to use die(), and when to use error() \nfollowed by exit(128)?\n"},{"id":"216189","messageId":"518179DD.8050009@kdbg.org","threadId":"33694","inReplyTo":"51817319.6060201@gmail.com","subject":"Re: [PATCH 1/2] help: add help_unknown_ref","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-05-01T20:23:57Z","receivedAt":"2013-05-01T20:23:57Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 01.05.2013 21:55, schrieb Vikrant Varma:\n> On 01-05-2013 17:53, Ramkumar Ramachandra wrote:\n>> Vikrant Varma wrote:\n>>> +void help_unknown_ref(const char* ref) {\n>>> + int i;\n>>> + struct similar_ref_cb ref_cb;\n>>> + ref_cb.similar_refs = (struct string_list)STRING_LIST_INIT_NODUP;\n>>\n>> Why are you casting STRING_LIST_INIT_NODUP?\n>>\n>>> + ref_cb.base_ref = ref;\n> \n> \n> ref_cb.similar_refs has already been defined. The compiler won't let me\n> assign to it unless I cast first. However, I think compound literals are\n> a C99/gcc feature. Is this better?\n> \n> struct similar_ref_cb ref_cb = {ref, STRING_LIST_INIT_NODUP};\n\nNo. There are compilers that can initialize a struct only with constant\ndata, but ref is not a constant.\n\n-- Hannes\n"},{"id":"216191","messageId":"CALkWK0nJSoBoAXR=ViGan6keFzXjmDGkfKqqWkTHqF89mHDDOQ@mail.gmail.com","threadId":"33694","inReplyTo":"51817319.6060201@gmail.com","subject":"Re: [PATCH 1/2] help: add help_unknown_ref","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-01T20:32:43Z","receivedAt":"2013-05-01T20:32:43Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Vikrant Varma wrote:\n> I agree with Matthieu, the people who don't want to see this advice never\n> will, because they won't make that mistake. Maybe advice is the wrong word,\n> corrections might be more appropriate.\n\nMakes sense.\n\nPerhaps it would make sense to hook into help.autocorrect. I would\ndefinitely like that.\n\n>> I see that there are other structs in our codebase suffixing _cb, to\n>> indicate \"callback data\". I normally reserve _cb for callback\n>> functions.\n>\n>\n> I'm following the convention (builtin/merge.c: struct append_ref_cb). If\n> there's a better way to name it, I'll use that.\n\nFine leaving it as it is.\n\n> ref_cb.similar_refs has already been defined. The compiler won't let me\n> assign to it unless I cast first. However, I think compound literals are a\n> C99/gcc feature. Is this better?\n>\n> struct similar_ref_cb ref_cb = {ref, STRING_LIST_INIT_NODUP};\n\nAs Johannes pointed out, ref is a variable and that is problematic.\nLeave the cast on: I didn't notice the compiler warning in my head.\n\n> Q_ is a pluralization helper that picks one of the two strings based on\n> ref_cb.similar_refs.nr. It's used in help.c:help_unknown_cmd for the same\n> reason.\n\nThanks.\n\n> Again, because help_unknown_cmd exited with 1. I've tried to follow the\n> convention as laid down there.\n\nAh, I didn't notice that.\n\n> What's the significance of the error code for\n> die()?\n\nWhen something _really_ bad happens, exit() with 128, without\nbothering to go up the callstack. Return error() is a common idiom to\nprint an error message immediately, and propagate the return status -1\nup the callstack.\n\n> When is it correct to use die(), and when to use error() followed by\n> exit(128)?\n\nexit(128) is a bit rare [grep for it yourself]. It's for when you\nwant to die(), but exit() only after doing an important task in the\ncaller.\n"},{"id":"216200","messageId":"51818CFA.9030305@gmail.com","threadId":"33694","inReplyTo":"CALkWK0nJSoBoAXR=ViGan6keFzXjmDGkfKqqWkTHqF89mHDDOQ@mail.gmail.com","subject":"Re: [PATCH 1/2] help: add help_unknown_ref","fromName":"Vikrant Varma","fromEmail":"vikrant.varma94@gmail.com","sentAt":"2013-05-01T21:45:30Z","receivedAt":"2013-05-01T21:45:30Z","isPatch":true,"sender":{"key":"vikrant.varma94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1186303?v=4"},"body":"On 02-05-2013 02:02, Ramkumar Ramachandra wrote:\n>> ref_cb.similar_refs has already been defined. The compiler won't let me\n>> assign to it unless I cast first. However, I think compound literals are a\n>> C99/gcc feature. Is this better?\n>>\n>> struct similar_ref_cb ref_cb = {ref, STRING_LIST_INIT_NODUP};\n>\n> As Johannes pointed out, ref is a variable and that is problematic.\n> Leave the cast on: I didn't notice the compiler warning in my head.\n>\nIs it okay to use a compound literal? It's not supported in C89.\n"},{"id":"216207","messageId":"7vobcu4af6.fsf@alter.siamese.dyndns.org","threadId":"33694","inReplyTo":"51818CFA.9030305@gmail.com","subject":"Re: [PATCH 1/2] help: add help_unknown_ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-01T22:12:13Z","receivedAt":"2013-05-01T22:12:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vikrant Varma writes:\n\n> On 02-05-2013 02:02, Ramkumar Ramachandra wrote:\n>>> ref_cb.similar_refs has already been defined. The compiler won't let me\n>>> assign to it unless I cast first. However, I think compound literals are a\n>>> C99/gcc feature. Is this better?\n>>>\n>>> struct similar_ref_cb ref_cb = {ref, STRING_LIST_INIT_NODUP};\n>>\n>> As Johannes pointed out, ref is a variable and that is problematic.\n>> Leave the cast on: I didn't notice the compiler warning in my head.\n>>\n> Is it okay to use a compound literal? It's not supported in C89.\n\nBuilding on top of what was suggested in the other message, the\nhelper could be made more reusable by doing something like this:\n\n\tint suggest_misspelt_ref(const char *ref, struct string_list *suggested);\n\nand the caller can do\n\n\tif (!commit) {\n\t\tstruct string_list suggested = STRING_LIST_INIT;\n if (suggest_misspelt_ref(argv[1], &suggested)) {\n \t... Did you mean one of these??? ...\n\t\t\tstring_list_clear(&suggested);\n\t\t}\n die(_(\"'%s' is not something we can merge'), argv[1]);\n\t}\n\nSo I think this point is moot. Of course, similar_ref_cb needs to\nbe updated to keep a pointer to an existing string_list, not an\ninstance of its own string_list.\n"},{"id":"216210","messageId":"5181967E.9020601@gmail.com","threadId":"33694","inReplyTo":"7vppxa5z01.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] help: add help_unknown_ref","fromName":"Vikrant Varma","fromEmail":"vikrant.varma94@gmail.com","sentAt":"2013-05-01T22:26:06Z","receivedAt":"2013-05-01T22:26:06Z","isPatch":true,"sender":{"key":"vikrant.varma94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1186303?v=4"},"body":"On 02-05-2013 00:05, Junio C Hamano wrote:\n> If you step back a bit, you would notice two things.\n>\n> (1) Saying 'foo' when the user means 'origin/foo' is hardly the\n> only (or even the most common) kind of mistake that the code\n> you need to add to 'git merge' would encounter and could help\n> the user with.\n\nYes. I like your suggestion of using levenshtein.c, similar to what's \nbeen done in help.c:help_unknown_cmd. However, where do you draw the \nline? Do you also suggest 'remotes/origin/foo' for 'remotes/foo'? Also, \nwhich would you then prioritize for 'foo': 'fob' (this is local) or \n'origin/foo'? In other words, what kind of mistakes are you looking to \ncorrect - typos, or forgetful omissions, or both and something more?\n\n> (2) \"merge\" is not the single command that user may make this kind\n> of mistake the command could help and use the same helper.\n> \"git branch myfoo foo\" may want to suggest \"origin/foo\", for\n> example. I just typed \"git checkout mater\", which could have\n> been easily corrected to \"git checkout master\" with a mechanism\n> like this.\n>\n\nOf course, once the suggestion mechanism is in place, it can be used to \nreplace unfriendly die()s in every command that takes a ref.\n\n>\n> An asterisk sticks to the parameter name, not type\n>\n> Indent with two HT, not HT followed by a run of SPs.\n>\n> An overlong line can and should be split\n >\n > Do not add trailing blank lines.\n >\n\nThanks, will fix this.\n\n> When you consider the point (2) above, it becomes clear why this\n> message does not belong to a helper function with a bland and\n> generic name \"help unknown ref\".\n>\n> This API is misdesigned. A possible alternative that may be better\n> reusable would be to have a helper that is used to come up with a\n> list of suggestions and make the caller responsible for emitting the\n> error message.\n>\n\nYes, I think a better name is needed, I was trying to follow along the \nlines of help_unknown_cmd.\n\nHowever, making the caller responsible for printing the suggestions may \nnot be the best alternative. Borrowing from the way help_unknown_cmd \nworks, in help_unknown_ref we could:\n1) check if autocorrect is on, returning the corrected refname to the \ncalling function, otherwise\n2) print an error message, a list of suggestions, and exit()\n\nI think this makes for a clean and reusable API, and requires changing \none line of code in every function that currently calls die().\n"},{"id":"216217","messageId":"5181A006.9050107@gmail.com","threadId":"33694","inReplyTo":"20130501182726.GL24467@google.com","subject":"Re: [PATCH 0/2] Better advice on merge","fromName":"Vikrant Varma","fromEmail":"vikrant.varma94@gmail.com","sentAt":"2013-05-01T23:06:46Z","receivedAt":"2013-05-01T23:06:46Z","isPatch":true,"sender":{"key":"vikrant.varma94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1186303?v=4"},"body":"On 01-05-2013 23:57, Jonathan Nieder wrote:\n> - It would be nice to add a test under t/\n>\n\nThanks, I'll do that.\n\n> - Since the first patch isn't useful without or logically separate\n> from the second, this would probably be easier to read as a single\n> patch.\n>\n\nThey are logically separate, even if the first isn't useful without the \nsecond. I wanted to segregate the task of defining a helper function \nthat corrects the ref name, and changing the parts of the code that \nshould use it. The reason the second is separate is that once the first \nis in place, even commands like 'checkout' can use the helper function.\n"},{"id":"216218","messageId":"7vzjwe2tai.fsf@alter.siamese.dyndns.org","threadId":"33694","inReplyTo":"5181967E.9020601@gmail.com","subject":"Re: [PATCH 1/2] help: add help_unknown_ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-01T23:07:33Z","receivedAt":"2013-05-01T23:07:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vikrant Varma writes:\n\n> On 02-05-2013 00:05, Junio C Hamano wrote:\n>> If you step back a bit, you would notice two things.\n>>\n>> (1) Saying 'foo' when the user means 'origin/foo' is hardly the\n>> only (or even the most common) kind of mistake that the code\n>> you need to add to 'git merge' would encounter and could help\n>> the user with.\n>\n> Yes. I like your suggestion of using levenshtein.c, similar to what's\n> been done in help.c:help_unknown_cmd. However, where do you draw the\n> line? Do you also suggest 'remotes/origin/foo' for 'remotes/foo'?\n> Also, which would you then prioritize for 'foo': 'fob' (this is local)\n> or 'origin/foo'? In other words, what kind of mistakes are you looking\n> to correct - typos, or forgetful omissions, or both and something\n> more?\n\nThis is your itch, so you get to choose what you would propose to\nthe list and have people tweak it ;-)\n\nI personally think the first version should not use levenshtein _at\nall_ (hence not solving my 'git checkout mastr'). I however want to\nmake sure the code is structured in such a way that allows other\npeople to build on top later.\n\n> However, making the caller responsible for printing the suggestions\n> may not be the best alternative.\n\nThat \"responsibility\" could be encapsulated in a separate helper\nfunction the callers call instead of die().\n\nThe lower level machinery to come up with a suggested list of refs,\nwhen the caller suspects that there is a typo in refs, is the same\nacross different commands, and it is insufficient to have a single\nhelper function that hardcodes \"not something we can _MERGE_\", and\ncopy the function to create many other helpers that do the same\nthing but with different error messages.\n\nYou would need two layers, one that collects suggested list of refs,\nand another that is directly called by the top-level caller that\ntakes the refname and the die message, and the latter would use the\nformer. What I was showing was the lower level one, without the\nhigher level one that takes die message and a suspicious user input\n(the higher level one would be trivial and do not need hand-holding\nwhile designing).\n"}]}