{"thread":{"id":"18851","subject":"[PATCH] builtin-reset.c: Extend hard reset error message when using paths.","startedAt":"2009-04-13T18:14:04Z","lastAt":"2009-04-14T05:35:29Z","messageCount":4,"participants":["Tim Retout","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"111209","messageId":"1239646444-11653-1-git-send-email-tim@retout.co.uk","threadId":"18851","inReplyTo":null,"subject":"[PATCH] builtin-reset.c: Extend hard reset error message when using paths.","fromName":"Tim Retout","fromEmail":"tim@retout.co.uk","sentAt":"2009-04-13T18:14:04Z","receivedAt":"2009-04-13T18:14:04Z","isPatch":true,"sender":{"key":"tim@retout.co.uk","avatar":null},"body":"Users who invoke 'git reset --hard <paths>' are probably trying to\nupdate paths in their working directory.  The error message should\npoint them in the direction of git-checkout(1).\n\nSigned-off-by: Tim Retout <tim@retout.co.uk>\n---\n builtin-reset.c |    4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-reset.c b/builtin-reset.c\nindex c0cb915..885ca9a 100644\n--- a/builtin-reset.c\n+++ b/builtin-reset.c\n@@ -257,6 +257,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \tif (i < argc) {\n \t\tif (reset_type == MIXED)\n \t\t\twarning(\"--mixed option is deprecated with paths.\");\n+\t\telse if (reset_type == HARD)\n+\t\t\tdie(\"Cannot do %s reset with paths.\\n\"\n+\t\t\t    \"See git-checkout(1) to update paths in the working tree.\",\n+\t\t\t\t\treset_type_names[reset_type]);\n \t\telse if (reset_type != NONE)\n \t\t\tdie(\"Cannot do %s reset with paths.\",\n \t\t\t\t\treset_type_names[reset_type]);\n-- \n1.6.2.2\n"},{"id":"111228","messageId":"7vr5zwe3ie.fsf@gitster.siamese.dyndns.org","threadId":"18851","inReplyTo":"1239646444-11653-1-git-send-email-tim@retout.co.uk","subject":"Re: [PATCH] builtin-reset.c: Extend hard reset error message when using paths.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-13T19:51:21Z","receivedAt":"2009-04-13T19:51:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Retout <tim@retout.co.uk> writes:\n\n> Users who invoke 'git reset --hard <paths>' are probably trying to\n> update paths in their working directory.  The error message should\n> point them in the direction of git-checkout(1).\n\nThat is one possibility.  Another is:\n\n    git reset --hard mester\n\n(and you have ./mester file in the work tree) and in that case the user\ndefinitely didn't want to do any checkout.\n\nI wonder if you can tell these cases apart, and also if this (not just\ntelling these apart, but what your patch adds) is worth additional\ncluttering in the running program.  I certainly wouldn't mind addition to\ngit-reset manual page if new people are often confused between \"checking\nout paths from the index or from the named commit\" and \"resetting the HEAD\nto a different commit while nuking the index and the work tree state\",\nthough.\n\n> Signed-off-by: Tim Retout <tim@retout.co.uk>\n> ---\n>  builtin-reset.c |    4 ++++\n>  1 files changed, 4 insertions(+), 0 deletions(-)\n>\n> diff --git a/builtin-reset.c b/builtin-reset.c\n> index c0cb915..885ca9a 100644\n> --- a/builtin-reset.c\n> +++ b/builtin-reset.c\n> @@ -257,6 +257,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n>  \tif (i < argc) {\n>  \t\tif (reset_type == MIXED)\n>  \t\t\twarning(\"--mixed option is deprecated with paths.\");\n> +\t\telse if (reset_type == HARD)\n> +\t\t\tdie(\"Cannot do %s reset with paths.\\n\"\n> +\t\t\t    \"See git-checkout(1) to update paths in the working tree.\",\n> +\t\t\t\t\treset_type_names[reset_type]);\n>  \t\telse if (reset_type != NONE)\n>  \t\t\tdie(\"Cannot do %s reset with paths.\",\n>  \t\t\t\t\treset_type_names[reset_type]);\n> -- \n> 1.6.2.2\n"},{"id":"111237","messageId":"1239657595.4641.50.camel@arneb.retout.co.uk","threadId":"18851","inReplyTo":"7vr5zwe3ie.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-reset.c: Extend hard reset error message when using paths.","fromName":"Tim Retout","fromEmail":"tim@retout.co.uk","sentAt":"2009-04-13T21:19:55Z","receivedAt":"2009-04-13T21:19:55Z","isPatch":true,"sender":{"key":"tim@retout.co.uk","avatar":null},"body":"On Mon, 2009-04-13 at 12:51 -0700, Junio C Hamano wrote:\n> Tim Retout <tim@retout.co.uk> writes:\n> \n> > Users who invoke 'git reset --hard <paths>' are probably trying to\n> > update paths in their working directory.  The error message should\n> > point them in the direction of git-checkout(1).\n> \n> That is one possibility.  Another is:\n> \n>     git reset --hard mester\n> \n> (and you have ./mester file in the work tree) and in that case the user\n> definitely didn't want to do any checkout.\n\nI would think that this sort of typo is sufficiently rare that it should\nnot cause confusion - at least, no more than 'Cannot do hard reset with\npaths.', which would be the current message.\n\n> I wonder if you can tell these cases apart, and also if this (not just\n> telling these apart, but what your patch adds) is worth additional\n> cluttering in the running program.\n\nWell, in my opinion the cluttering is minimal, and a line similar to\nthis would be useful to help these users find the right documentation...\n\n> I certainly wouldn't mind addition to\n> git-reset manual page if new people are often confused between \"checking\n> out paths from the index or from the named commit\" and \"resetting the HEAD\n> to a different commit while nuking the index and the work tree state\",\n> though.\n\n...and a note in the man page would be helpful as well, similar to that\nin git-revert(1).  I'm happy to come up with something.\n\nI'm aiming to reduce the frequency with which I get asked for help with\nreverting a file.  Perhaps adding a <paths> form to 'revert' is what I\nreally want to do.\n\n-- \nTim Retout <tim@retout.co.uk>\n"},{"id":"111262","messageId":"20090414053529.GB19027@coredump.intra.peff.net","threadId":"18851","inReplyTo":"7vr5zwe3ie.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-reset.c: Extend hard reset error message when using paths.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-14T05:35:29Z","receivedAt":"2009-04-14T05:35:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 13, 2009 at 12:51:21PM -0700, Junio C Hamano wrote:\n\n> That is one possibility.  Another is:\n> \n>     git reset --hard mester\n> \n> (and you have ./mester file in the work tree) and in that case the user\n> definitely didn't want to do any checkout.\n> \n> I wonder if you can tell these cases apart, and also if this (not just\n> telling these apart, but what your patch adds) is worth additional\n\nYou could always guess based on the existing refs:\n\n---\ndiff --git a/builtin-reset.c b/builtin-reset.c\nindex c0cb915..803957d 100644\n--- a/builtin-reset.c\n+++ b/builtin-reset.c\n@@ -243,8 +243,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (get_sha1(rev, sha1))\n-\t\tdie(\"Failed to resolve '%s' as a valid ref.\", rev);\n+\tif (get_sha1(rev, sha1)) {\n+\t\terror(\"Failed to resolve '%s' as a valid ref.\", rev);\n+\t\tprobable_refs_print(rev);\n+\t\treturn 1;\n+\t}\n \n \tcommit = lookup_commit_reference(sha1);\n \tif (!commit)\n@@ -257,6 +260,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \tif (i < argc) {\n \t\tif (reset_type == MIXED)\n \t\t\twarning(\"--mixed option is deprecated with paths.\");\n+\t\telse if (reset_type == HARD) {\n+\t\t\terror(\"Cannot do hard reset with paths.\");\n+\t\t\tprobable_refs_print(argv[i]);\n+\t\t\treturn 1;\n+\t\t}\n \t\telse if (reset_type != NONE)\n \t\t\tdie(\"Cannot do %s reset with paths.\",\n \t\t\t\t\treset_type_names[reset_type]);\ndiff --git a/refs.c b/refs.c\nindex 82afb87..bad7e2f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3,6 +3,8 @@\n #include \"object.h\"\n #include \"tag.h\"\n #include \"dir.h\"\n+#include \"levenshtein.h\"\n+#include \"string-list.h\"\n \n /* ISSYMREF=01 and ISPACKED=02 are public interfaces */\n #define REF_KNOWS_PEELED 04\n@@ -750,9 +752,8 @@ int check_ref_format(const char *ref)\n \t}\n }\n \n-const char *prettify_ref(const struct ref *ref)\n+static const char *prettify_refname(const char *name)\n {\n-\tconst char *name = ref->name;\n \treturn name + (\n \t\t!prefixcmp(name, \"refs/heads/\") ? 11 :\n \t\t!prefixcmp(name, \"refs/tags/\") ? 10 :\n@@ -760,6 +761,11 @@ const char *prettify_ref(const struct ref *ref)\n \t\t0);\n }\n \n+const char *prettify_ref(const struct ref *ref)\n+{\n+\treturn prettify_refname(ref->name);\n+}\n+\n const char *ref_rev_parse_rules[] = {\n \t\"%.*s\",\n \t\"refs/%.*s\",\n@@ -1756,3 +1762,74 @@ char *shorten_unambiguous_ref(const char *ref)\n \tfree(short_name);\n \treturn xstrdup(ref);\n }\n+\n+struct probable_refs_data {\n+\tconst char *s;\n+\tstruct string_list *out;\n+};\n+\n+static void probable_refs_append(struct probable_refs_data *d, const char *s)\n+{\n+\tstruct string_list_item *i = string_list_append(s, d->out);\n+\ti->util = (void *)levenshtein(d->s, s, 0, 2, 1, 4);\n+}\n+\n+static int probable_refs_cb(const char *refname, const unsigned char *sha1,\n+\t\tint flag, void *data)\n+{\n+\tconst char *p = prettify_refname(refname);\n+\tprobable_refs_append(data, refname);\n+\tif (p != refname)\n+\t\tprobable_refs_append(data, p);\n+\treturn 1;\n+}\n+\n+static int util_compare(const void *va, const void *vb)\n+{\n+\tconst struct string_list_item *a = va, *b = vb;\n+\treturn a->util - b->util;\n+}\n+\n+void probable_refs(const char *s, struct string_list *out)\n+{\n+\tstruct probable_refs_data d;\n+\tint i, j;\n+\n+\tmemset(out, 0, sizeof(*out));\n+\tout->strdup_strings = 1;\n+\td.s = s;\n+\td.out = out;\n+\tfor_each_ref(probable_refs_cb, &d);\n+\n+\tif (!out->nr)\n+\t\treturn;\n+\n+\tqsort(out->items, out->nr, sizeof(*out->items), util_compare);\n+\t/* Arbitrary cutoff at distance '5'. The first is the best match, but\n+\t * there may be others which are equally good */\n+\tfor (i = 0; i < out->nr; i++)\n+\t\tif ((int)out->items[i].util > 5 ||\n+\t\t    out->items[i].util > out->items[0].util)\n+\t\t\tbreak;\n+\t/* truncate the list to i items */\n+\tfor (j = i; j < out->nr; j++)\n+\t\tfree(out->items[j].string);\n+\tout->nr = i;\n+}\n+\n+void probable_refs_print(const char *name)\n+{\n+\tstruct string_list s;\n+\tint i;\n+\n+\tprobable_refs(name, &s);\n+\tif (!s.nr)\n+\t\treturn;\n+\n+\tfprintf(stderr, \"\\nDid you mean %s?\\n\",\n+\t\ts.nr < 2 ? \"this\" : \"one of these\");\n+\tfor (i = 0; i < s.nr; i++)\n+\t\tfprintf(stderr, \"\\t%s\\n\", s.items[i].string);\n+\n+\tstring_list_clear(&s, 0);\n+}\ndiff --git a/refs.h b/refs.h\nindex 50abbbb..586bfab 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -95,4 +95,6 @@ int update_ref(const char *action, const char *refname,\n \t\tconst unsigned char *sha1, const unsigned char *oldval,\n \t\tint flags, enum action_on_err onerr);\n \n+void probable_refs_print(const char *name);\n+\n #endif /* REFS_H */\n\n\n\nAnd yes, I am joking.\n\n-Peff\n"}]}