[PATCH] git-revert is one of the most misunderstood command in git, help users out.

When git-revert has a file argument then redirect the user to what he
probably meant.

Signed-off-by: Pierre Habouzit <redacted>
---
 builtin-revert.c |   24 +++++++++++++++++-------
 gitk             |    2 +-
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/builtin-revert.c b/builtin-revert.c
index 62ab1fa..9660048 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c

@@ -38,7 +38,7 @@ static const char *me;
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
-static void parse_args(int argc, const char **argv)
+static void parse_args(int argc, const char **argv, const char *prefix)
 {
 	const char * const * usage_str =
 		action == REVERT ?  revert_usage : cherry_pick_usage;

@@ -58,8 +58,18 @@ static void parse_args(int argc, const char **argv)
 		usage_with_options(usage_str, options);
 	arg = argv[0];
 
-	if (get_sha1(arg, sha1))
-		die ("Cannot find '%s'", arg);
+	if (get_sha1(arg, sha1)) {
+		struct stat st;
+		const char *name;
+
+		name = prefix ? prefix_filename(prefix, strlen(prefix), arg) : arg;
+		if (!lstat(name, &st)) {
+			die("Cannot find commit '%s', did you meant: "
+				"git checkout HEAD -- '%s'", arg, arg);
+		} else {
+			die("Cannot find commit '%s'", arg);
+		}
+	}
 	commit = (struct commit *)parse_object(sha1);
 	if (!commit)
 		die ("Could not find %s", sha1_to_hex(sha1));

@@ -225,7 +235,7 @@ static int merge_recursive(const char *base_sha1,
 	return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD);
 }
 
-static int revert_or_cherry_pick(int argc, const char **argv)
+static int revert_or_cherry_pick(int argc, const char **argv, const char *prefix)
 {
 	unsigned char head[20];
 	struct commit *base, *next, *parent;

@@ -237,7 +247,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	git_config(git_default_config);
 	me = action == REVERT ? "revert" : "cherry-pick";
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	parse_args(argc, argv);
+	parse_args(argc, argv, prefix);
 
 	/* this is copied from the shell script, but it's never triggered... */
 	if (action == REVERT && !no_replay)

@@ -405,12 +415,12 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 		edit = 1;
 	no_replay = 1;
 	action = REVERT;
-	return revert_or_cherry_pick(argc, argv);
+	return revert_or_cherry_pick(argc, argv, prefix);
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
 	no_replay = 0;
 	action = CHERRY_PICK;
-	return revert_or_cherry_pick(argc, argv);
+	return revert_or_cherry_pick(argc, argv, prefix);
 }

diff --git a/gitk b/gitk
index 1da0b0a..ab8bab2 100755
--- a/gitk
+++ b/gitk

@@ -1,6 +1,6 @@
 #!/bin/sh
 # Tcl ignores the next line -*- tcl -*- \
-exec wish "$0" -- "$@"
+exec wish8.5 "$0" -- "$@"
 
 # Copyright (C) 2005-2006 Paul Mackerras.  All rights reserved.
 # This program is free software; it may be used, copied, modified

-- 
1.5.3.5.1541.gd2b5c-dirty

On Mon, Nov 05, 2007 at 07:01:41PM +0000, Pierre Habouzit wrote:

  F*CK this chunk is obviously spurious.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

On Mon, Nov 05, 2007 at 08:04:11PM +0100, Pierre Habouzit wrote:

On Mon, Nov 05, 2007 at 07:01:41PM +0000, Pierre Habouzit wrote:

quoted

+		if (!lstat(name, &st)) {
+			die("Cannot find commit '%s', did you meant: "

s/meant/mean/

quoted

+				"git checkout HEAD -- '%s'", arg, arg);
+		} else {
+			die("Cannot find commit '%s'", arg);
+		}
+	}

--b.

On Mon, Nov 05, 2007 at 07:05:56PM +0000, J. Bruce Fields wrote:

On Mon, Nov 05, 2007 at 08:04:11PM +0100, Pierre Habouzit wrote:

quoted

On Mon, Nov 05, 2007 at 07:01:41PM +0000, Pierre Habouzit wrote:

quoted

+		if (!lstat(name, &st)) {
+			die("Cannot find commit '%s', did you meant: "

s/meant/mean/

Yeah I wrote that out of the iritation of the 192812948th question about
that on #git, I should have read my patch it seems :)

Though, if there will be a new maint release, I do believe that this
patch is maint material.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

On Nov 5, 2007, at 11:01 AM, Pierre Habouzit wrote:

When git-revert has a file argument then redirect the user to what he
probably meant.

That's a big improvement. Basically everyone I show git to gets  
"revert" wrong at first.

+			die("Cannot find commit '%s', did you meant: "
+				"git checkout HEAD -- '%s'", arg, arg);

But that suggested command is not going to convince anyone they were  
wrong about git being hard to learn. I wonder if instead of saying, "I  
know what you meant, but I'm going to make you type a different  
command," we should make git revert just do what the user meant.

There is already precedent for that kind of mixed-mode UI:

git checkout my-branch
vs.
git checkout my/source/file.c

-Steve

On Mon, Nov 05, 2007 at 07:28:03PM +0000, Steven Grimm wrote:

On Nov 5, 2007, at 11:01 AM, Pierre Habouzit wrote:

quoted

When git-revert has a file argument then redirect the user to what he
probably meant.

That's a big improvement. Basically everyone I show git to gets "revert" 
wrong at first.

quoted

+			die("Cannot find commit '%s', did you meant: "
+				"git checkout HEAD -- '%s'", arg, arg);

But that suggested command is not going to convince anyone they were 
wrong about git being hard to learn. I wonder if instead of saying, "I 
know what you meant, but I'm going to make you type a different command," 
we should make git revert just do what the user meant.

  That's an option, but it wouldn't be maint material then :)

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

On Mon 05 Nov 2007, 11:28, Steven Grimm wrote:

But that suggested command is not going to convince anyone they were wrong 
about git being hard to learn. I wonder if instead of saying, "I know what 
you meant, but I'm going to make you type a different command," we should 
make git revert just do what the user meant.

I think that would just add to confusion.  "revert" applies to full
changesets, not single files, plus it creates a new commit, which is
probably not what the user wants.  Most of them just want to revert some
local changes to some random files, so teach them what they need, if
anything.

There is already precedent for that kind of mixed-mode UI:

git checkout my-branch
vs.
git checkout my/source/file.c

This is a different case: you're basically performing the same
operation, with the second line applying just to a subset of files.

- Alex