threads / patch / 10672

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

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

## tl;dr

39 messages between Nov 5, 2007 and Nov 7, 2007. Diffs are folded; open one to read it.

replies: 38people: 12as markdown or json

Pierre Habouzit· Nov 5, 2007, 19:01 UTC · lore

[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 <madcoder@debian.org>
---
 builtin-revert.c |   24 +++++++++++++++++-------
 gitk             |    2 +-
 2 files changed, 18 insertions(+), 8 deletions(-)
Show changes to 2 files +18 −8

builtin-revert.c, gitk

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
Pierre Habouzit· Nov 5, 2007, 19:04 UTC · re: Pierre Habouzit · lore
On Mon, Nov 05, 2007 at 07:01:41PM +0000, Pierre Habouzit wrote:
Show 76 quoted lines
> When git-revert has a file argument then redirect the user to what he
> probably meant.
> 
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>  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);
>  }
Show 14 quoted lines
> 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
  F*CK this chunk is obviously spurious.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
J. Bruce Fields· Nov 5, 2007, 19:05 UTC · re: Pierre Habouzit · lore
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:
> > +		if (!lstat(name, &st)) {
> > +			die("Cannot find commit '%s', did you meant: "
s/meant/mean/
Show 5 quoted lines
> > +				"git checkout HEAD -- '%s'", arg, arg);
> > +		} else {
> > +			die("Cannot find commit '%s'", arg);
> > +		}
> > +	}
--b.
Pierre Habouzit· Nov 5, 2007, 19:10 UTC · re: J. Bruce Fields · lore
On Mon, Nov 05, 2007 at 07:05:56PM +0000, J. Bruce Fields wrote:
Show 6 quoted lines
> 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:
> > > +		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
Steven Grimm· Nov 5, 2007, 19:28 UTC · re: Pierre Habouzit · lore
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
Pierre Habouzit· Nov 5, 2007, 19:50 UTC · re: Steven Grimm · lore
On Mon, Nov 05, 2007 at 07:28:03PM +0000, Steven Grimm wrote:
Show 14 quoted lines
> 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.
  That's an option, but it wouldn't be maint material then :)
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Alejandro Martinez Ruiz· Nov 5, 2007, 21:54 UTC · re: Steven Grimm · lore
On Mon 05 Nov 2007, 11:28, Steven Grimm wrote:
Show 5 quoted lines
>
> 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.

Show 5 quoted lines
> 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
David Kastrup· Nov 5, 2007, 22:06 UTC · re: Alejandro Martinez Ruiz · lore
Alejandro Martinez Ruiz <alex@flawedcode.org> writes:
Show 21 quoted lines
> 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.
Huh?  The first one moves HEAD.  The second one doesn't.
-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum
Alejandro Martinez Ruiz· Nov 5, 2007, 23:41 UTC · re: David Kastrup · lore
On Mon 05 Nov 2007, 23:06, David Kastrup wrote:
Show 13 quoted lines
> Alejandro Martinez Ruiz <alex@flawedcode.org> writes:
> > On Mon 05 Nov 2007, 11:28, Steven Grimm wrote:
> >
> >> 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.
> 
> Huh?  The first one moves HEAD.  The second one doesn't.

That's why I wrote "basically". Anyway, the point is that this doesn't seem to be a valid precedent.

- Alex
Junio C Hamano· Nov 5, 2007, 22:21 UTC · re: Steven Grimm · lore
Steven Grimm <koreth@midwinter.com> writes:
Show 10 quoted lines
> 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

That's an example of mixed-mode UI, but what you are suggesting is quite different, isn't it?

There is no other officially supported single-command-way to checkout paths out of the index. "git checkout paths..." does not introduce a confusion because of that. The user learns the way git supports that concept and that's the end of the story. The same thing can be said about "git checkout <commit> paths...". That's _the_ way to checkout paths out of an arbitrary commit.

In the case being discussed, we already have the concept of checking out paths from the index, which has an officially supported way to express.

You are proposing to give it a synonym "git revert paths...", which superfitially sounds friendlier. But I actually think allowing a mistaken

	git revert path...

to be burned to users' fingers and brains is doing the user a great disservice.

The next person would say "Why doesn't 'git revert HEAD path...' work?", and you would add the synonym to do 'git checkout HEAD path...'. Up to that point it is sort-of Ok (but not quite). You already have "git checkout" that let's you do so, but you introduced new concepts that are "revert paths to the index" and "revert paths to the last commit".

Which may make you feel good, but you just introduced a narrower synonym the user needs to learn, than a more established and wider concept that we already have: "checkout paths out of X", where X are either the index or an arbitrary commit.

The reason I think the narrower synonym is bad and will lead to more user confusion is because after that point you will have a few issues.

Another newcomer would say "I like the fact that 'git revert HEAD path...' works but why doesn't 'git revert HEAD~12 path...' work?".

 - You may further allow "git revert <arbitrary-commit>
   path...".  But what does that _mean_?  "revert the path to
   the twelfth commit"?  You may implement that _anyway_.
   Then, the user would say "eh, why do you have both 'git
   checkout path...' and 'git revert path...' that seem to do
   the same thing?  There's no difference?  Why Why Why, git is
   so hard to learn".
 - You may instead not to do so, and explain that the "arbitrary
   commit" form is not supported and tell the user to use "git
   checkout <commit> paths...".
   The user will say: "but you earlier told me to use revert --
   you could have taught me to use checkout from the beginning
   and saved me from great confusion instead".

Giving the same concept two different names is bad unless there is a compelling reason to do so. Labelling an initially narrower subset of an existing concept with a different name, and having to extended that 'new concept' ending up with the same as the existing concept is even worse.

Johannes Schindelin· Nov 5, 2007, 23:40 UTC · re: Junio C Hamano · lore
Hi,
On Mon, 5 Nov 2007, Junio C Hamano wrote:
Show 18 quoted lines
> Steven Grimm <koreth@midwinter.com> writes:
> 
> > 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
> 
> That's an example of mixed-mode UI, but what you are suggesting is quite 
> different, isn't it?
> 
> There is no other officially supported single-command-way to
> checkout paths out of the index.
Okay, let's step back a bit.

We taught "git show" to show other objects than commits, by doing the obvious things. So there _is_ a precendent to changing a commands behaviour to accept more than just commits. And there was already another command for the same purpose, cat-file, which was never meant as porcelain however.

Now, what does "revert" _mean_? At the moment, it wants a commit, and will undo the changes that commit introduced, _and commits it_ (asking for a message).

What would I expect "git revert -- file" to do? It would undo the changes to that file -- and since no commit was specified, I would expect it to look at the changes against the index. (IOW exactly what Steven proposed.)

To continue the analogy, it would have to commit the undoing of the change. But since that change never was committed, I think it is more natural to _not_ commit it.

In the same way, I would expect "git revert <commit> -- file" to undo the changes in that commit to _that_ file (something like "git merge-file file <commit>:file <commit>^:file"), but this time commit it, since it was committed at one stage.

IMHO this would be a consistent behaviour _and_ help new git users.

After all, we are not Python, supposedly narrowing users down to one-way-to-do-things only.

Ciao, Dscho

Pierre Habouzit· Nov 6, 2007, 00:08 UTC · re: Johannes Schindelin · lore
On Mon, Nov 05, 2007 at 11:40:46PM +0000, Johannes Schindelin wrote:
Show 48 quoted lines
> Hi,
> 
> On Mon, 5 Nov 2007, Junio C Hamano wrote:
> 
> > Steven Grimm <koreth@midwinter.com> writes:
> > 
> > > 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
> > 
> > That's an example of mixed-mode UI, but what you are suggesting is quite 
> > different, isn't it?
> > 
> > There is no other officially supported single-command-way to
> > checkout paths out of the index.
> 
> Okay, let's step back a bit.
> 
> We taught "git show" to show other objects than commits, by doing the 
> obvious things.  So there _is_ a precendent to changing a commands 
> behaviour to accept more than just commits.  And there was already another 
> command for the same purpose, cat-file, which was never meant as 
> porcelain however.
> 
> Now, what does "revert" _mean_?  At the moment, it wants a commit, and 
> will undo the changes that commit introduced, _and commits it_ (asking 
> for a message).
> 
> What would I expect "git revert -- file" to do?  It would undo the changes 
> to that file -- and since no commit was specified, I would expect it to 
> look at the changes against the index.  (IOW exactly what Steven 
> proposed.)
> 
> To continue the analogy, it would have to commit the undoing of the 
> change.  But since that change never was committed, I think it is more 
> natural to _not_ commit it.
> 
> In the same way, I would expect "git revert <commit> -- file" to undo the 
> changes in that commit to _that_ file (something like "git merge-file 
> file <commit>:file <commit>^:file"), but this time commit it, since it 
> was committed at one stage.
  I agree that this is something that really makes sense to me, and does
not looks like a perversion of the UI, and quite a nice extension in
fact. And it would _finally_ solve the issue that for _ANYTHING_ on the
planet that I've used for more than 10 seconds, $SCM revert path/to/file
reverts local changes :)
  When you look at how git-revert is implemented, you'll see that it
uses the very same code as git cherry-pick does, and in fact, I've
wanted a git cherry-pick <commit> -- path1 path2 path3 for a _very_ long
time, and your proposal would just gracefully give it as a bonus I
believe (of course uncommited changes have no sense for a cherry-pick).
  I like it a lot.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Junio C Hamano· Nov 6, 2007, 02:51 UTC · re: Johannes Schindelin · lore
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> In the same way, I would expect "git revert <commit> -- file" to undo the 
> changes in that commit to _that_ file (something like "git merge-file 
> file <commit>:file <commit>^:file"), but this time commit it, since it 
> was committed at one stage.

Allowing people to revert or cherry pick partially by using paths limiter is a very good idea; the whole "it comes from a commit so we also commit" feels an utter nonsense, though.

Johannes Schindelin· Nov 6, 2007, 03:18 UTC · re: Junio C Hamano · lore
Hi,
On Mon, 5 Nov 2007, Junio C Hamano wrote:
Show 10 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > In the same way, I would expect "git revert <commit> -- file" to undo 
> > the changes in that commit to _that_ file (something like "git 
> > merge-file file <commit>:file <commit>^:file"), but this time commit 
> > it, since it was committed at one stage.
> 
> Allowing people to revert or cherry pick partially by using paths 
> limiter is a very good idea; the whole "it comes from a commit so we 
> also commit" feels an utter nonsense, though.
No.

When "git revert <commit>" commits the result, "git revert <commit> -- <file>" should, too.

You can always add the "-n" option.

Ciao, Dscho

Junio C Hamano· Nov 6, 2007, 04:54 UTC · re: Johannes Schindelin · lore
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 10 quoted lines
> On Mon, 5 Nov 2007, Junio C Hamano wrote:
>
>> Allowing people to revert or cherry pick partially by using paths 
>> limiter is a very good idea; the whole "it comes from a commit so we 
>> also commit" feels an utter nonsense, though.
>
> No.
>
> When "git revert <commit>" commits the result, "git revert <commit> -- 
> <file>" should, too.

I was not questioning about that part. "If 'git revert <some other form> foo' does not talk about commit, it should not commit" was what I was referring to.

Pierre Habouzit· Nov 6, 2007, 08:49 UTC · re: Junio C Hamano · lore
On Tue, Nov 06, 2007 at 04:54:02AM +0000, Junio C Hamano wrote:
Show 16 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Mon, 5 Nov 2007, Junio C Hamano wrote:
> >
> >> Allowing people to revert or cherry pick partially by using paths 
> >> limiter is a very good idea; the whole "it comes from a commit so we 
> >> also commit" feels an utter nonsense, though.
> >
> > No.
> >
> > When "git revert <commit>" commits the result, "git revert <commit> -- 
> > <file>" should, too.
> 
> I was not questioning about that part.  "If 'git revert <some
> other form> foo' does not talk about commit, it should not
> commit" was what I was referring to.
  Well, I don't really know how closely you read #git, but I'd say that
"how do I undo my local changes in a git repository" is among the top 3
questions. There _IS_ an UI issue for that.

If git revert <commitish> -- path1 path2 path3 is going to work at some point, I see no harm in saying that git revert HEAD -- path1 path2 path3 work. We can also in that case spit an error message:

error: this works as a courtesy but you really meant git checkout -- path/to/file
  On some other issues I'm all about educating people and learning to
them how to "think different". But here it's a pure interface problem,
and git is the sole $scm with a revert commands that doesn't reverts
local changes wrt HEAD.
  The next release of master will have tons of UI improvements (terse
output, better options parsing, more builtins hence faster commands …),
I believe it's stopping halfway not thinking about issues like this from
a newcomer point of view.
  On the pure theoretical basis I believe you're right, it's a bit
mixing apples and oranges. On the pragmatic usability side I'm quite
sure you're wrong, because everyone is used to that:
    $ hg revert --help | head -3 | tail -1
    revert files or dirs to their states as of some revision
    $ bzr help revert | head -1
    Purpose: Revert files to a previous revision.
    $ svn help revert | head -1
    revert: Restore pristine working copy file (undo most local edits).
    $ darcs help revert | head -3 | tail -1
    Revert to the recorded version (safe the first time only).
    <put your favorite non-git scm with a revert command here>
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Mike Hommey· Nov 6, 2007, 09:29 UTC · re: Pierre Habouzit · lore
On Tue, Nov 06, 2007 at 09:49:25AM +0100, Pierre Habouzit <madcoder@debian.org> wrote:
Show 25 quoted lines
> On Tue, Nov 06, 2007 at 04:54:02AM +0000, Junio C Hamano wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> > > On Mon, 5 Nov 2007, Junio C Hamano wrote:
> > >
> > >> Allowing people to revert or cherry pick partially by using paths 
> > >> limiter is a very good idea; the whole "it comes from a commit so we 
> > >> also commit" feels an utter nonsense, though.
> > >
> > > No.
> > >
> > > When "git revert <commit>" commits the result, "git revert <commit> -- 
> > > <file>" should, too.
> > 
> > I was not questioning about that part.  "If 'git revert <some
> > other form> foo' does not talk about commit, it should not
> > commit" was what I was referring to.
> 
>   Well, I don't really know how closely you read #git, but I'd say that
> "how do I undo my local changes in a git repository" is among the top 3
> questions. There _IS_ an UI issue for that.
> 
> If git revert <commitish> -- path1 path2 path3 is going to work at some
> point, I see no harm in saying that git revert HEAD -- path1 path2 path3
> work. We can also in that case spit an error message:

It seems to me git revert HEAD -- path1 path2 path3 should revert the changes made in the commit pointed to by HEAD, not revert the changes in the working tree or the index...

Mike
Pierre Habouzit· Nov 6, 2007, 09:37 UTC · re: Mike Hommey · lore
On Tue, Nov 06, 2007 at 09:29:42AM +0000, Mike Hommey wrote:
Show 30 quoted lines
> On Tue, Nov 06, 2007 at 09:49:25AM +0100, Pierre Habouzit <madcoder@debian.org> wrote:
> > On Tue, Nov 06, 2007 at 04:54:02AM +0000, Junio C Hamano wrote:
> > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > > 
> > > > On Mon, 5 Nov 2007, Junio C Hamano wrote:
> > > >
> > > >> Allowing people to revert or cherry pick partially by using paths 
> > > >> limiter is a very good idea; the whole "it comes from a commit so we 
> > > >> also commit" feels an utter nonsense, though.
> > > >
> > > > No.
> > > >
> > > > When "git revert <commit>" commits the result, "git revert <commit> -- 
> > > > <file>" should, too.
> > > 
> > > I was not questioning about that part.  "If 'git revert <some
> > > other form> foo' does not talk about commit, it should not
> > > commit" was what I was referring to.
> > 
> >   Well, I don't really know how closely you read #git, but I'd say that
> > "how do I undo my local changes in a git repository" is among the top 3
> > questions. There _IS_ an UI issue for that.
> > 
> > If git revert <commitish> -- path1 path2 path3 is going to work at some
> > point, I see no harm in saying that git revert HEAD -- path1 path2 path3
> > work. We can also in that case spit an error message:
> 
> It seems to me git revert HEAD -- path1 path2 path3 should revert the changes
> made in the commit pointed to by HEAD, not revert the changes in the working
> tree or the index...
  Yes, sorry, the `HEAD` in my sentence was spurious.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Johannes Schindelin· Nov 6, 2007, 12:32 UTC · re: Junio C Hamano · lore
Hi,
On Mon, 5 Nov 2007, Junio C Hamano wrote:
Show 16 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Mon, 5 Nov 2007, Junio C Hamano wrote:
> >
> >> Allowing people to revert or cherry pick partially by using paths 
> >> limiter is a very good idea; the whole "it comes from a commit so we 
> >> also commit" feels an utter nonsense, though.
> >
> > No.
> >
> > When "git revert <commit>" commits the result, "git revert <commit> -- 
> > <file>" should, too.
> 
> I was not questioning about that part.  "If 'git revert <some
> other form> foo' does not talk about commit, it should not
> commit" was what I was referring to.

Well, I think that _if_ we allow "git revert <path>" to mean "revert the changes to <path>, relative to the index" (which would be the same as "git checkout <path>"), then committing that change just does not make sense.

And it is this behaviour that people are seeking, not "git revert <commit> <path>".

Ciao, Dscho

Junio C Hamano· Nov 6, 2007, 18:06 UTC · re: Johannes Schindelin · lore
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 6 quoted lines
> Well, I think that _if_ we allow "git revert <path>" to mean "revert the 
> changes to <path>, relative to the index" (which would be the same as "git 
> checkout <path>"), then committing that change just does not make sense.
>
> And it is this behaviour that people are seeking, not "git revert <commit> 
> <path>".
Heh, I found this in the recent log somewhere.
<gitte> Really, I wonder how difficult git is for people who are not
	brainwashed by cvs/svn, and unfortunately enough, partly by bzr and hg.
<gitte> From a user perspective, you might be correct.  But then we have to
	add 1000 commands to reflect the English language.
<gitte> Not what I want.						[06:46]
I am wondering who said it ;-).

But anyway, I am inclined to agree that accepting "$scm revert paths.." as a synonym for "git checkout -- paths..."

Johannes Schindelin· Nov 6, 2007, 18:27 UTC · re: Junio C Hamano · lore
Hi,
On Tue, 6 Nov 2007, Junio C Hamano wrote:
Show 18 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Well, I think that _if_ we allow "git revert <path>" to mean "revert the 
> > changes to <path>, relative to the index" (which would be the same as "git 
> > checkout <path>"), then committing that change just does not make sense.
> >
> > And it is this behaviour that people are seeking, not "git revert <commit> 
> > <path>".
> 
> Heh, I found this in the recent log somewhere.
> 
> <gitte> Really, I wonder how difficult git is for people who are not
> 	brainwashed by cvs/svn, and unfortunately enough, partly by bzr and hg.
> <gitte> From a user perspective, you might be correct.  But then we have to
> 	add 1000 commands to reflect the English language.
> <gitte> Not what I want.						[06:46]
> 
> I am wondering who said it ;-).
Now, that is not fair, using my own words against me ;-)

Ciao, Dscho

Pierre Habouzit· Nov 6, 2007, 19:39 UTC · re: Johannes Schindelin · lore
On Tue, Nov 06, 2007 at 06:27:54PM +0000, Johannes Schindelin wrote:
Show 24 quoted lines
> Hi,
> 
> On Tue, 6 Nov 2007, Junio C Hamano wrote:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> > > Well, I think that _if_ we allow "git revert <path>" to mean "revert the 
> > > changes to <path>, relative to the index" (which would be the same as "git 
> > > checkout <path>"), then committing that change just does not make sense.
> > >
> > > And it is this behaviour that people are seeking, not "git revert <commit> 
> > > <path>".
> > 
> > Heh, I found this in the recent log somewhere.
> > 
> > <gitte> Really, I wonder how difficult git is for people who are not
> > 	brainwashed by cvs/svn, and unfortunately enough, partly by bzr and hg.
> > <gitte> From a user perspective, you might be correct.  But then we have to
> > 	add 1000 commands to reflect the English language.
> > <gitte> Not what I want.						[06:46]
> > 
> > I am wondering who said it ;-).
> 
> Now, that is not fair, using my own words against me ;-)
  That's very funny actually :]
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Junio C Hamano· Nov 6, 2007, 19:42 UTC · re: Pierre Habouzit · lore
Pierre Habouzit <madcoder@debian.org> writes:
Show 27 quoted lines
> On Tue, Nov 06, 2007 at 06:27:54PM +0000, Johannes Schindelin wrote:
>> Hi,
>> 
>> On Tue, 6 Nov 2007, Junio C Hamano wrote:
>> 
>> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > 
>> > > Well, I think that _if_ we allow "git revert <path>" to mean "revert the 
>> > > changes to <path>, relative to the index" (which would be the same as "git 
>> > > checkout <path>"), then committing that change just does not make sense.
>> > >
>> > > And it is this behaviour that people are seeking, not "git revert <commit> 
>> > > <path>".
>> > 
>> > Heh, I found this in the recent log somewhere.
>> > 
>> > <gitte> Really, I wonder how difficult git is for people who are not
>> > 	brainwashed by cvs/svn, and unfortunately enough, partly by bzr and hg.
>> > <gitte> From a user perspective, you might be correct.  But then we have to
>> > 	add 1000 commands to reflect the English language.
>> > <gitte> Not what I want.						[06:46]
>> > 
>> > I am wondering who said it ;-).
>> 
>> Now, that is not fair, using my own words against me ;-)
>
>   That's very funny actually :]

Yeah, it was doubly funny after I saw you posted a list of "$scm revert" and Dscho still sided with you in that thread.

Johannes Schindelin· Nov 6, 2007, 22:21 UTC · re: Junio C Hamano · lore
Hi,
On Tue, 6 Nov 2007, Junio C Hamano wrote:
Show 33 quoted lines
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > On Tue, Nov 06, 2007 at 06:27:54PM +0000, Johannes Schindelin wrote:
> > 
> >> On Tue, 6 Nov 2007, Junio C Hamano wrote:
> >> 
> >> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >> > 
> >> > > Well, I think that _if_ we allow "git revert <path>" to mean 
> >> > > "revert the changes to <path>, relative to the index" (which 
> >> > > would be the same as "git checkout <path>"), then committing that 
> >> > > change just does not make sense.
> >> > >
> >> > > And it is this behaviour that people are seeking, not "git revert 
> >> > > <commit> <path>".
> >> > 
> >> > Heh, I found this in the recent log somewhere.
> >> > 
> >> > <gitte> Really, I wonder how difficult git is for people who are not
> >> > 	brainwashed by cvs/svn, and unfortunately enough, partly by bzr 
> >> >  and hg.
> >> > <gitte> From a user perspective, you might be correct.  But then we 
> >> >  have to add 1000 commands to reflect the English language.
> >> > <gitte> Not what I want.						[06:46]
> >> > 
> >> > I am wondering who said it ;-).
> >> 
> >> Now, that is not fair, using my own words against me ;-)
> >
> >   That's very funny actually :]
> 
> Yeah, it was doubly funny after I saw you posted a list of "$scm revert" 
> and Dscho still sided with you in that thread.
Hey, I had my nice 5 minutes for the day, so give me a break!
;-)

Ciao, Dscho

Robin Rosenberg· Nov 6, 2007, 20:06 UTC · re: Johannes Schindelin · lore
tisdag 06 november 2007 skrev Johannes Schindelin:
Show 27 quoted lines
> Hi,
> 
> On Mon, 5 Nov 2007, Junio C Hamano wrote:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> > > On Mon, 5 Nov 2007, Junio C Hamano wrote:
> > >
> > >> Allowing people to revert or cherry pick partially by using paths 
> > >> limiter is a very good idea; the whole "it comes from a commit so we 
> > >> also commit" feels an utter nonsense, though.
> > >
> > > No.
> > >
> > > When "git revert <commit>" commits the result, "git revert <commit> -- 
> > > <file>" should, too.
> > 
> > I was not questioning about that part.  "If 'git revert <some
> > other form> foo' does not talk about commit, it should not
> > commit" was what I was referring to.
> 
> Well, I think that _if_ we allow "git revert <path>" to mean "revert the 
> changes to <path>, relative to the index" (which would be the same as "git 
> checkout <path>"), then committing that change just does not make sense.
> 
> And it is this behaviour that people are seeking, not "git revert <commit> 
> <path>".

I'm not convince making every command perform enitrely all kinds of actions just because other SCMs interpret a name differently. git revert today creates a *new* commit. Keep it simple. I think its ok that it mentions another comnand when it detects arguments that does not make sense. There is no right or wrong with interepreting reset either way, but not both ways please. The confusion with checkout and reset is enough.

-- robin
Mike Hommey· Nov 6, 2007, 20:13 UTC · re: Robin Rosenberg · lore
On Tue, Nov 06, 2007 at 09:06:56PM +0100, Robin Rosenberg wrote:
Show 35 quoted lines
> tisdag 06 november 2007 skrev Johannes Schindelin:
> > Hi,
> > 
> > On Mon, 5 Nov 2007, Junio C Hamano wrote:
> > 
> > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > > 
> > > > On Mon, 5 Nov 2007, Junio C Hamano wrote:
> > > >
> > > >> Allowing people to revert or cherry pick partially by using paths 
> > > >> limiter is a very good idea; the whole "it comes from a commit so we 
> > > >> also commit" feels an utter nonsense, though.
> > > >
> > > > No.
> > > >
> > > > When "git revert <commit>" commits the result, "git revert <commit> -- 
> > > > <file>" should, too.
> > > 
> > > I was not questioning about that part.  "If 'git revert <some
> > > other form> foo' does not talk about commit, it should not
> > > commit" was what I was referring to.
> > 
> > Well, I think that _if_ we allow "git revert <path>" to mean "revert the 
> > changes to <path>, relative to the index" (which would be the same as "git 
> > checkout <path>"), then committing that change just does not make sense.
> > 
> > And it is this behaviour that people are seeking, not "git revert <commit> 
> > <path>".
> 
> I'm not convince making every command perform enitrely all kinds of actions 
> just because other SCMs interpret a name differently. git revert today 
> creates a *new* commit. Keep it simple. I think its ok that it mentions 
> another comnand when it detects arguments that does not make sense. There is 
> no right or wrong with interepreting reset either way, but not both ways 
> please. The confusion with checkout and reset is enough.

Maybe the documentation could emphasise on how to undo things when the user makes mistakes. Sometimes, saving your repo can be as simple as git reset --hard HEAD@{1}. This is not, unfortunately, a works-for-all-cases command.

Mike
Robin Rosenberg· Nov 6, 2007, 21:21 UTC · re: Mike Hommey · lore
tisdag 06 november 2007 skrev Mike Hommey:
> Maybe the documentation could emphasise on how to undo things when the
> user makes mistakes.
> Sometimes, saving your repo can be as simple as git reset --hard HEAD@{1}.
> This is not, unfortunately, a works-for-all-cases command.
Yea, git-undo(7). 
-- robin
Johannes Schindelin· Nov 6, 2007, 22:25 UTC · re: Robin Rosenberg · lore
Hi,
On Tue, 6 Nov 2007, Robin Rosenberg wrote:
Show 7 quoted lines
> tisdag 06 november 2007 skrev Mike Hommey:
> > Maybe the documentation could emphasise on how to undo things when the
> > user makes mistakes.
> > Sometimes, saving your repo can be as simple as git reset --hard HEAD@{1}.
> > This is not, unfortunately, a works-for-all-cases command.
> 
> Yea, git-undo(7). 
In related news, I know a few users who need an un-rm-rf.  Anyone?

Ciao, Dscho

Mike Hommey· Nov 7, 2007, 08:16 UTC · re: Johannes Schindelin · lore
On Tue, Nov 06, 2007 at 10:25:48PM +0000, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 13 quoted lines
> Hi,
> 
> On Tue, 6 Nov 2007, Robin Rosenberg wrote:
> 
> > tisdag 06 november 2007 skrev Mike Hommey:
> > > Maybe the documentation could emphasise on how to undo things when the
> > > user makes mistakes.
> > > Sometimes, saving your repo can be as simple as git reset --hard HEAD@{1}.
> > > This is not, unfortunately, a works-for-all-cases command.
> > 
> > Yea, git-undo(7). 
> 
> In related news, I know a few users who need an un-rm-rf.  Anyone?

The fact is you can do harm to your repo with things you wouldn't expect to break things, except maybe you gave bad arguments or so. It's quite easy to fuck up with git-rebase, or to merge the wrong commits, etc.

Mike
Johannes Schindelin· Nov 7, 2007, 11:08 UTC · re: Mike Hommey · lore
Hi,
On Wed, 7 Nov 2007, Mike Hommey wrote:
Show 17 quoted lines
> On Tue, Nov 06, 2007 at 10:25:48PM +0000, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> > On Tue, 6 Nov 2007, Robin Rosenberg wrote:
> > 
> > > tisdag 06 november 2007 skrev Mike Hommey:
> > > > Maybe the documentation could emphasise on how to undo things when 
> > > > the user makes mistakes. Sometimes, saving your repo can be as 
> > > > simple as git reset --hard HEAD@{1}. This is not, unfortunately, a 
> > > > works-for-all-cases command.
> > > 
> > > Yea, git-undo(7). 
> > 
> > In related news, I know a few users who need an un-rm-rf.  Anyone?
> 
> The fact is you can do harm to your repo with things you wouldn't expect 
> to break things, except maybe you gave bad arguments or so. It's quite 
> easy to fuck up with git-rebase, or to merge the wrong commits, etc.

I don't see how these commands are dangerous. Usually you just look into the reflog, pick the one commit you started with, and reset --hard.

The _only_ commands I find dangerous are "git stash clear" and "git reflog --expire=0". Funnily, people want to do that all the time.

Like recently, on the IRC channel, where somebody lost patches "during a rebase", by "rm -rf .dotest".

There will be a point where nobody can help. But before that, reflogs are your friend. But you must not do "reset --hard HEAD@{1}" blindly. You have to look first what the reflogs are.

Ciao, Dscho

Robin Rosenberg· Nov 7, 2007, 19:32 UTC · re: Johannes Schindelin · lore
onsdag 07 november 2007 skrev Johannes Schindelin:
Show 24 quoted lines
> Hi,
> 
> On Wed, 7 Nov 2007, Mike Hommey wrote:
> 
> > On Tue, Nov 06, 2007 at 10:25:48PM +0000, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > 
> > > On Tue, 6 Nov 2007, Robin Rosenberg wrote:
> > > 
> > > > tisdag 06 november 2007 skrev Mike Hommey:
> > > > > Maybe the documentation could emphasise on how to undo things when 
> > > > > the user makes mistakes. Sometimes, saving your repo can be as 
> > > > > simple as git reset --hard HEAD@{1}. This is not, unfortunately, a 
> > > > > works-for-all-cases command.
> > > > 
> > > > Yea, git-undo(7). 
> > > 
> > > In related news, I know a few users who need an un-rm-rf.  Anyone?
> > 
> > The fact is you can do harm to your repo with things you wouldn't expect 
> > to break things, except maybe you gave bad arguments or so. It's quite 
> > easy to fuck up with git-rebase, or to merge the wrong commits, etc.
> 
> I don't see how these commands are dangerous.  Usually you just look into 
> the reflog, pick the one commit you started with, and reset --hard.
Indeed, but you must *know* that and you must know that you *can* do it.
As for undo rm -rf, it's not part of git and outside the scope of git.
-- robin
Jakub Narebski· Nov 7, 2007, 20:01 UTC · re: Robin Rosenberg · lore
Robin Rosenberg wrote:
> As for undo rm -rf, it's not part of git and outside the scope of git.

Unless Mnemosyne or some other automatic backup solution is based on git as engine...

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
David Kastrup· Nov 7, 2007, 09:03 UTC · re: Johannes Schindelin · lore
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 11 quoted lines
> On Tue, 6 Nov 2007, Robin Rosenberg wrote:
>
>> tisdag 06 november 2007 skrev Mike Hommey:
>> > Maybe the documentation could emphasise on how to undo things when the
>> > user makes mistakes.
>> > Sometimes, saving your repo can be as simple as git reset --hard HEAD@{1}.
>> > This is not, unfortunately, a works-for-all-cases command.
>> 
>> Yea, git-undo(7). 
>
> In related news, I know a few users who need an un-rm-rf.  Anyone?

Most file systems don't have a reflog or other ways to recover from shooting yourself in the foot. git has, and for good reason.

There is no sense in hiding that facility away because of feeling macho. Since git already keeps the file space around needed for recovery (and you really have to exert yourself to make it let go for good), there is no point in not making it as convenient as feasible to recover.

-- 
David Kastrup
Junio C Hamano· Nov 6, 2007, 11:08 UTC · re: Junio C Hamano · lore
Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> In the same way, I would expect "git revert <commit> -- file" to undo the 
>> changes in that commit to _that_ file (something like "git merge-file 
>> file <commit>:file <commit>^:file"), but this time commit it, since it 
>> was committed at one stage.
>
> Allowing people to revert or cherry pick partially by using
> paths limiter is a very good idea; ...

As Pierre said earlier, a partial revert via "revert <commit> -- <paths>" and a partial cherry-pick would make quite a lot of sense, and in addition, it should not be too hard to add.

Reusing the 'merge-recursive' part should be almost trivial. The only tricky part is coming up with a fake tree using base and next commit in revert_or_cherry_pick() for this purpose.

When replaying the change from A->B (when cherry-picking, A is the parent and B is what was named from the command line; when reverting, they are the other way around), instead of doing the three-way merge using:

	merge-recursive A HEAD B

you would first come up with a modified tree B' that has the identical contents to A _except_ the parts the path limiters specify which are taken from B. Then running

	merge-recursive A HEAD B'

would replay the revert or cherry-pick of change from A->B, limited by the path, on top of the current HEAD.

As to "reverting to the index" case, if somebody is interested in doing a builtin-checkout.c, please keep in mind that major parts of that work should be made available to the implementation of "git revert [--] <paths>", as it appears that it will be exactly the same as "git checkout" with the same set of options.

I am wondering what "git cherry-pick -- <paths>" should do. My current thinking is that it would not make any sense at all.

Johannes Sixt· Nov 6, 2007, 11:51 UTC · re: Junio C Hamano · lore
Junio C Hamano schrieb:
> I am wondering what "git cherry-pick -- <paths>" should do.  My
> current thinking is that it would not make any sense at all.
IMO, at least "git cherry-pick -n -- <paths>" makes tons of sense.
-- Hannes
Johannes Schindelin· Nov 6, 2007, 12:16 UTC · re: Johannes Sixt · lore
Hi,
On Tue, 6 Nov 2007, Johannes Sixt wrote:
Show 5 quoted lines
> Junio C Hamano schrieb:
> > I am wondering what "git cherry-pick -- <paths>" should do.  My
> > current thinking is that it would not make any sense at all.
> 
> IMO, at least "git cherry-pick -n -- <paths>" makes tons of sense.

I guess you missed that Junio did not specify any commit. With a commit, I agree, it makes tons of sense.

Without a commit, it would default to... uhm... HEAD? And applying the changes to a given file, which are already in HEAD, no, that does not make sense.

Ciao, Dscho

Johannes Schindelin· Nov 6, 2007, 12:25 UTC · re: Junio C Hamano · lore
Hi,
On Tue, 6 Nov 2007, Junio C Hamano wrote:
Show 15 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> >> In the same way, I would expect "git revert <commit> -- file" to undo the 
> >> changes in that commit to _that_ file (something like "git merge-file 
> >> file <commit>:file <commit>^:file"), but this time commit it, since it 
> >> was committed at one stage.
> >
> > Allowing people to revert or cherry pick partially by using
> > paths limiter is a very good idea; ...
> 
> As Pierre said earlier, a partial revert via "revert <commit> --
> <paths>" and a partial cherry-pick would make quite a lot of
> sense, and in addition, it should not be too hard to add.

Yes, but Pierre also said earlier that people want to revert their local changes. And the logical thing to try that really is

	git revert <path>

Now, if you read that out in English, it does not make too much sense: "revert the path" (not "revert the _changes_ to that file"). But it is what people try to do.

However, IIUC another thing Pierre mentioned is that
	$scm revert <commit> <path>

commonly means "revert the file _to the version_ stored in <commit>". This is just different enough from "revert the _changes_ to that file stored in <commit>" to bite people, no?

> Reusing the 'merge-recursive' part should be almost trivial. The only 
> tricky part is coming up with a fake tree using base and next commit in 
> revert_or_cherry_pick() for this purpose.

FWIW I really wanted to use the merge-file machinery, not the merge-recursive one. But since "<path>" can be a directory, too, I was mistaken, and you are correct, as always.

Show 5 quoted lines
> As to "reverting to the index" case, if somebody is interested in doing 
> a builtin-checkout.c, please keep in mind that major parts of that work 
> should be made available to the implementation of "git revert [--] 
> <paths>", as it appears that it will be exactly the same as "git 
> checkout" with the same set of options.
I was planning to put cmd_checkout() into builtin-reset.c for that reason.

But first things first, that "git remote prune" with --mirror'ed repositories misbehaviour annoys me just enough that I started converting this script first. It has been stable enough for quite a long time, and the script now shows its limitations.

Besides, remote.[ch] makes it easy, even if not _really_ easy.

Ciao, Dscho

Pierre Habouzit· Nov 6, 2007, 12:48 UTC · re: Johannes Schindelin · lore
On Tue, Nov 06, 2007 at 12:25:33PM +0000, Johannes Schindelin wrote:
Show 36 quoted lines
> Hi,
>
> On Tue, 6 Nov 2007, Junio C Hamano wrote:
>
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > >
> > >> In the same way, I would expect "git revert <commit> -- file" to undo the
> > >> changes in that commit to _that_ file (something like "git merge-file
> > >> file <commit>:file <commit>^:file"), but this time commit it, since it
> > >> was committed at one stage.
> > >
> > > Allowing people to revert or cherry pick partially by using
> > > paths limiter is a very good idea; ...
> >
> > As Pierre said earlier, a partial revert via "revert <commit> --
> > <paths>" and a partial cherry-pick would make quite a lot of
> > sense, and in addition, it should not be too hard to add.
>
> Yes, but Pierre also said earlier that people want to revert their local
> changes.  And the logical thing to try that really is
>
> 	git revert <path>
>
> Now, if you read that out in English, it does not make too much sense:
> "revert the path" (not "revert the _changes_ to that file").  But it is
> what people try to do.
>
> However, IIUC another thing Pierre mentioned is that
>
> 	$scm revert <commit> <path>
>
> commonly means "revert the file _to the version_ stored in <commit>".
> This is just different enough from "revert the _changes_ to that file
> stored in <commit>" to bite people, no?
  Yeah but that's what checkout is for. The main source of iritation for
new users comes (IMHO) from svn, where `svn revert path/to/file` is part
of the workflow: in case of a conflict when you `svn up`, you have
either to:
  (1) fix the conflict and `svn resolved path/to/file`
  (2) drop your changes and take the trunk version `svn revert path/to/file`

People really expect git revert -- path/to/file to do the same as git checkout HEAD -- path/to/file. Though I believe that like I said, maybe we don't wan't git revert -- path/to/file to become the first class command to do that, but rather to do what the user meant, hinting him in the direction of the proper command. I wasn't really advocating that git-revert should be a complete implementation of what git checkout <comitish> -- <paths> does. YMMV.

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

Wincent Colaiuta· Nov 6, 2007, 17:43 UTC · re: Pierre Habouzit · lore
El 6/11/2007, a las 13:48, Pierre Habouzit escribió:
Show 63 quoted lines
> On Tue, Nov 06, 2007 at 12:25:33PM +0000, Johannes Schindelin wrote:
>> Hi,
>>
>> On Tue, 6 Nov 2007, Junio C Hamano wrote:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>>>
>>>>> In the same way, I would expect "git revert <commit> -- file" to  
>>>>> undo the
>>>>> changes in that commit to _that_ file (something like "git merge- 
>>>>> file
>>>>> file <commit>:file <commit>^:file"), but this time commit it,  
>>>>> since it
>>>>> was committed at one stage.
>>>>
>>>> Allowing people to revert or cherry pick partially by using
>>>> paths limiter is a very good idea; ...
>>>
>>> As Pierre said earlier, a partial revert via "revert <commit> --
>>> <paths>" and a partial cherry-pick would make quite a lot of
>>> sense, and in addition, it should not be too hard to add.
>>
>> Yes, but Pierre also said earlier that people want to revert their  
>> local
>> changes.  And the logical thing to try that really is
>>
>> 	git revert <path>
>>
>> Now, if you read that out in English, it does not make too much  
>> sense:
>> "revert the path" (not "revert the _changes_ to that file").  But  
>> it is
>> what people try to do.
>>
>> However, IIUC another thing Pierre mentioned is that
>>
>> 	$scm revert <commit> <path>
>>
>> commonly means "revert the file _to the version_ stored in <commit>".
>> This is just different enough from "revert the _changes_ to that file
>> stored in <commit>" to bite people, no?
>
>  Yeah but that's what checkout is for. The main source of iritation  
> for
> new users comes (IMHO) from svn, where `svn revert path/to/file` is  
> part
> of the workflow: in case of a conflict when you `svn up`, you have
> either to:
>  (1) fix the conflict and `svn resolved path/to/file`
>  (2) drop your changes and take the trunk version `svn revert path/ 
> to/file`
>
> People really expect git revert -- path/to/file to do the same as git
> checkout HEAD -- path/to/file. Though I believe that like I said,  
> maybe
> we don't wan't git revert -- path/to/file to become the first class
> command to do that, but rather to do what the user meant, hinting  
> him in
> the direction of the proper command. I wasn't really advocating that
> git-revert should be a complete implementation of what git checkout
> <comitish> -- <paths> does. YMMV.

I agree; they're semantically different and it wouldn't be a good thing to start blurring the lines between them too much. It's just unfortunate that the term "revert" is used by most other SCMs to mean something different than what it means in "git-revert". I think the best path here is education, what Pierre says, rather than changing git-revert's semantics.

The other changes discussed so far in this thread (path-limiting git- revert with preserving its semantics) seem like a good thing.

Cheers, Wincent

← back to recent threads