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);
/* thi...That's a big improvement. Basically everyone I show git to gets 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 -
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 chec...
Hi, 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 -
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. -
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. -
Hi, 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 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 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 -
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. -- =C2=B7O=C2=B7 Pierre Habouzit =C2=B7=C2=B7O madcoder@debia= n.org OOO http://www.madism.org
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 -
IMO, at least "git cherry-pick -n -- <paths>" makes tons of sense. -- Hannes -
Hi, 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 -
Hi, No. When "git revert <commit>" commits the result, "git revert <commit> -- <file>" should, too. You can always add the "-n" option. Ciao, Dscho -
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. -
Hi, 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 -
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 -
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
-Yea, git-undo(7). -- robin -
Hi, In related news, I know a few users who need an un-rm-rf. Anyone? Ciao, Dscho -
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 -
Hi,
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
-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 -
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..." -
Hi, Now, that is not fair, using my own words against me ;-) Ciao, Dscho -
That's very funny actually :] --=20 =C2=B7O=C2=B7 Pierre Habouzit =C2=B7=C2=B7O madcoder@debia= n.org OOO http://www.madism.org
Yeah, it was doubly funny after I saw you posted a list of "$scm revert" and Dscho still sided with you in that thread. -
Hi, Hey, I had my nice 5 minutes for the day, so give me a break! ;-) Ciao, Dscho -
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/t=
o/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 =E2=80=
=A6),
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>
--=20
=C2=B7O=C2=B7 Pierre Habouzit
=C2=B7=C2=B7O madcoder@debia=
n.org
OOO http://www.madism.orgIt 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 -
Yes, sorry, the `HEAD` in my sentence was spurious. --=20 =C2=B7O=C2=B7 Pierre Habouzit =C2=B7=C2=B7O madcoder@debia= n.org OOO http://www.madism.org
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. --=20 =C2=B7O=C2=B7 Pierre Habouzit =C2=B7=C2=B7O madcoder@debia= n.org OOO http://www.madism.org
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 This is a different case: you're basically performing the same operation, with the second line applying just to a subset of files. - Alex -
Huh? The first one moves HEAD. The second one doesn't. -- David Kastrup, Kriemhildstr. 15, 44793 Bochum -
That's why I wrote "basically". Anyway, the point is that this doesn't seem to be a valid precedent. - Alex -
That's an option, but it wouldn't be maint material then :) --=20 =C2=B7O=C2=B7 Pierre Habouzit =C2=B7=C2=B7O madcoder@debia= n.org OOO http://www.madism.org
F*CK this chunk is obviously spurious. --=20 =C2=B7O=C2=B7 Pierre Habouzit =C2=B7=C2=B7O madcoder@debia= n.org OOO http://www.madism.org
--b. -
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. --=20 =C2=B7O=C2=B7 Pierre Habouzit =C2=B7=C2=B7O madcoder@debia= n.org OOO http://www.madism.org
