[PATCH] builtin-reset.c: Extend hard reset error message when using paths.

[PATCH] builtin-reset.c: Extend hard reset error message when using paths.

[Tim Retout]

Users who invoke 'git reset --hard <paths>' are probably trying to
update paths in their working directory.  The error message should
point them in the direction of git-checkout(1).

Signed-off-by: Tim Retout <tim@retout.co.uk>
---
 builtin-reset.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/builtin-reset.c b/builtin-reset.c
index c0cb915..885ca9a 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -257,6 +257,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (i < argc) {
 		if (reset_type == MIXED)
 			warning("--mixed option is deprecated with paths.");
+		else if (reset_type == HARD)
+			die("Cannot do %s reset with paths.\n"
+			    "See git-checkout(1) to update paths in the working tree.",
+					reset_type_names[reset_type]);
 		else if (reset_type != NONE)
 			die("Cannot do %s reset with paths.",
 					reset_type_names[reset_type]);
-- 
1.6.2.2

Re: [PATCH] builtin-reset.c: Extend hard reset error message when using paths.

[Junio C Hamano]

Tim Retout <tim@retout.co.uk> writes:

> Users who invoke 'git reset --hard <paths>' are probably trying to
> update paths in their working directory.  The error message should
> point them in the direction of git-checkout(1).

That is one possibility.  Another is:

    git reset --hard mester

(and you have ./mester file in the work tree) and in that case the user
definitely didn't want to do any checkout.

I wonder if you can tell these cases apart, and also if this (not just
telling these apart, but what your patch adds) is worth additional
cluttering in the running program.  I certainly wouldn't mind addition to
git-reset manual page if new people are often confused between "checking
out paths from the index or from the named commit" and "resetting the HEAD
to a different commit while nuking the index and the work tree state",
though.

> Signed-off-by: Tim Retout <tim@retout.co.uk>
> ---
>  builtin-reset.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/builtin-reset.c b/builtin-reset.c
> index c0cb915..885ca9a 100644
> --- a/builtin-reset.c
> +++ b/builtin-reset.c
> @@ -257,6 +257,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>  	if (i < argc) {
>  		if (reset_type == MIXED)
>  			warning("--mixed option is deprecated with paths.");
> +		else if (reset_type == HARD)
> +			die("Cannot do %s reset with paths.\n"
> +			    "See git-checkout(1) to update paths in the working tree.",
> +					reset_type_names[reset_type]);
>  		else if (reset_type != NONE)
>  			die("Cannot do %s reset with paths.",
>  					reset_type_names[reset_type]);
> -- 
> 1.6.2.2

Re: [PATCH] builtin-reset.c: Extend hard reset error message when using paths.

[Tim Retout]

On Mon, 2009-04-13 at 12:51 -0700, Junio C Hamano wrote:
> Tim Retout <tim@retout.co.uk> writes:
> 
> > Users who invoke 'git reset --hard <paths>' are probably trying to
> > update paths in their working directory.  The error message should
> > point them in the direction of git-checkout(1).
> 
> That is one possibility.  Another is:
> 
>     git reset --hard mester
> 
> (and you have ./mester file in the work tree) and in that case the user
> definitely didn't want to do any checkout.

I would think that this sort of typo is sufficiently rare that it should
not cause confusion - at least, no more than 'Cannot do hard reset with
paths.', which would be the current message.

> I wonder if you can tell these cases apart, and also if this (not just
> telling these apart, but what your patch adds) is worth additional
> cluttering in the running program.

Well, in my opinion the cluttering is minimal, and a line similar to
this would be useful to help these users find the right documentation...

> I certainly wouldn't mind addition to
> git-reset manual page if new people are often confused between "checking
> out paths from the index or from the named commit" and "resetting the HEAD
> to a different commit while nuking the index and the work tree state",
> though.

...and a note in the man page would be helpful as well, similar to that
in git-revert(1).  I'm happy to come up with something.

I'm aiming to reduce the frequency with which I get asked for help with
reverting a file.  Perhaps adding a <paths> form to 'revert' is what I
really want to do.

-- 
Tim Retout <tim@retout.co.uk>

Re: [PATCH] builtin-reset.c: Extend hard reset error message when using paths.

[Jeff King]

On Mon, Apr 13, 2009 at 12:51:21PM -0700, Junio C Hamano wrote:

> That is one possibility.  Another is:
> 
>     git reset --hard mester
> 
> (and you have ./mester file in the work tree) and in that case the user
> definitely didn't want to do any checkout.
> 
> I wonder if you can tell these cases apart, and also if this (not just
> telling these apart, but what your patch adds) is worth additional

You could always guess based on the existing refs:

---
diff --git a/builtin-reset.c b/builtin-reset.c
index c0cb915..803957d 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -243,8 +243,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	if (get_sha1(rev, sha1))
-		die("Failed to resolve '%s' as a valid ref.", rev);
+	if (get_sha1(rev, sha1)) {
+		error("Failed to resolve '%s' as a valid ref.", rev);
+		probable_refs_print(rev);
+		return 1;
+	}
 
 	commit = lookup_commit_reference(sha1);
 	if (!commit)
@@ -257,6 +260,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (i < argc) {
 		if (reset_type == MIXED)
 			warning("--mixed option is deprecated with paths.");
+		else if (reset_type == HARD) {
+			error("Cannot do hard reset with paths.");
+			probable_refs_print(argv[i]);
+			return 1;
+		}
 		else if (reset_type != NONE)
 			die("Cannot do %s reset with paths.",
 					reset_type_names[reset_type]);
diff --git a/refs.c b/refs.c
index 82afb87..bad7e2f 100644
--- a/refs.c
+++ b/refs.c
@@ -3,6 +3,8 @@
 #include "object.h"
 #include "tag.h"
 #include "dir.h"
+#include "levenshtein.h"
+#include "string-list.h"
 
 /* ISSYMREF=01 and ISPACKED=02 are public interfaces */
 #define REF_KNOWS_PEELED 04
@@ -750,9 +752,8 @@ int check_ref_format(const char *ref)
 	}
 }
 
-const char *prettify_ref(const struct ref *ref)
+static const char *prettify_refname(const char *name)
 {
-	const char *name = ref->name;
 	return name + (
 		!prefixcmp(name, "refs/heads/") ? 11 :
 		!prefixcmp(name, "refs/tags/") ? 10 :
@@ -760,6 +761,11 @@ const char *prettify_ref(const struct ref *ref)
 		0);
 }
 
+const char *prettify_ref(const struct ref *ref)
+{
+	return prettify_refname(ref->name);
+}
+
 const char *ref_rev_parse_rules[] = {
 	"%.*s",
 	"refs/%.*s",
@@ -1756,3 +1762,74 @@ char *shorten_unambiguous_ref(const char *ref)
 	free(short_name);
 	return xstrdup(ref);
 }
+
+struct probable_refs_data {
+	const char *s;
+	struct string_list *out;
+};
+
+static void probable_refs_append(struct probable_refs_data *d, const char *s)
+{
+	struct string_list_item *i = string_list_append(s, d->out);
+	i->util = (void *)levenshtein(d->s, s, 0, 2, 1, 4);
+}
+
+static int probable_refs_cb(const char *refname, const unsigned char *sha1,
+		int flag, void *data)
+{
+	const char *p = prettify_refname(refname);
+	probable_refs_append(data, refname);
+	if (p != refname)
+		probable_refs_append(data, p);
+	return 1;
+}
+
+static int util_compare(const void *va, const void *vb)
+{
+	const struct string_list_item *a = va, *b = vb;
+	return a->util - b->util;
+}
+
+void probable_refs(const char *s, struct string_list *out)
+{
+	struct probable_refs_data d;
+	int i, j;
+
+	memset(out, 0, sizeof(*out));
+	out->strdup_strings = 1;
+	d.s = s;
+	d.out = out;
+	for_each_ref(probable_refs_cb, &d);
+
+	if (!out->nr)
+		return;
+
+	qsort(out->items, out->nr, sizeof(*out->items), util_compare);
+	/* Arbitrary cutoff at distance '5'. The first is the best match, but
+	 * there may be others which are equally good */
+	for (i = 0; i < out->nr; i++)
+		if ((int)out->items[i].util > 5 ||
+		    out->items[i].util > out->items[0].util)
+			break;
+	/* truncate the list to i items */
+	for (j = i; j < out->nr; j++)
+		free(out->items[j].string);
+	out->nr = i;
+}
+
+void probable_refs_print(const char *name)
+{
+	struct string_list s;
+	int i;
+
+	probable_refs(name, &s);
+	if (!s.nr)
+		return;
+
+	fprintf(stderr, "\nDid you mean %s?\n",
+		s.nr < 2 ? "this" : "one of these");
+	for (i = 0; i < s.nr; i++)
+		fprintf(stderr, "\t%s\n", s.items[i].string);
+
+	string_list_clear(&s, 0);
+}
diff --git a/refs.h b/refs.h
index 50abbbb..586bfab 100644
--- a/refs.h
+++ b/refs.h
@@ -95,4 +95,6 @@ int update_ref(const char *action, const char *refname,
 		const unsigned char *sha1, const unsigned char *oldval,
 		int flags, enum action_on_err onerr);
 
+void probable_refs_print(const char *name);
+
 #endif /* REFS_H */



And yes, I am joking.

-Peff