Re: [PATCH] prepare deprecation of git-revert

Previous thread: Re: jgit as a jira plugin by Shawn O. Pearce on Friday, October 31, 2008 - 7:42 am. (1 message)

Next thread: large publicly accessible HTTP archive? by Bryan Larsen on Friday, October 31, 2008 - 10:07 am. (2 messages)
From: Pierre Habouzit
Date: Friday, October 31, 2008 - 8:55 am

* Rename builtin-revert.c into builtin-cherry-pick.c

* Add option -R/--revert to git-cherry-pick.
  Document it by taking the current content of git-revert manpage for the
  option.

* get rid of the no_replay initialization, just ignore it when we're in
  the revert case, it makes really no sense to error out.

* put the warning of deprecation in cmd_revert, #if 0-ed out for now.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---

 I've not kept the auto-edit feature of git-revert for the git-cherry-pick -R
 case as I don't believe it makes a lot of sense. But if people are unhappy
 with that, I can easily "fix" it.

 Documentation/git-cherry-pick.txt         |   15 +++++++++++++++
 Makefile                                  |    6 +++---
 builtin-revert.c => builtin-cherry-pick.c |   10 ++++------
 3 files changed, 22 insertions(+), 9 deletions(-)
 rename builtin-revert.c => builtin-cherry-pick.c (98%)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 837fb08..2d92f2d 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -40,6 +40,21 @@ OPTIONS
 	development branch), adding this information can be
 	useful.
 
+-R::
+--revert::
+	Given one existing commit, revert the change the patch introduces, and
+	record a new commit that records it.  This requires your working tree
+	to be clean (no modifications from the HEAD commit).
++
+Note: 'git revert' is used to record a new commit to reverse the
+effect of an earlier commit (often a faulty one).  If you want to
+throw away all uncommitted changes in your working directory, you
+should see linkgit:git-reset[1], particularly the '--hard' option.  If
+you want to extract specific files as they were in another commit, you
+should see linkgit:git-checkout[1], specifically the 'git checkout
+<commit> -- <filename>' syntax.  Take care with these alternatives as
+both will discard uncommitted changes in your working directory.
+
 -r::
 	It ...
From: Pierre Habouzit
Date: Friday, October 31, 2008 - 8:57 am

--=20
=C2=B7O=C2=B7  Pierre Habouzit
=C2=B7=C2=B7O                                                madcoder@debia=
n.org
OOO                                                http://www.madism.org
From: Jakub Narebski
Date: Friday, October 31, 2008 - 9:36 am

By the way, Mercurial names this command IIRC 'hg backout'. 

But I think that adding '-R' option to git-cherry-pick is a good idea
even if we don't go deprecating git-revert.
-- 
Jakub Narebski
Poland
ShadeHawk on #git
--

From: Pierre Habouzit
Date: Friday, October 31, 2008 - 9:54 am

Actually part of the "Git UI sucks at time"-talk by pasy, we somehow
decided that git-revert would probably be deprecated in the future to
avoid the clash between what people coming from other's SCM worlds
expect it to be.

I don't remember what the tentative schedule was, that's why I left the
warning commented out for now.
--=20
=C2=B7O=C2=B7  Pierre Habouzit
=C2=B7=C2=B7O                                                madcoder@debia=
n.org
OOO                                                http://www.madism.org
From: Theodore Tso
Date: Friday, October 31, 2008 - 12:01 pm

By the way, BitKeeper names this command "bk undo" (which is another
reason why I would advocate against "git undo" as a syntatic sugar for
"git checkout HEAD -- $*") --- not that I think there are too many BK
refugees that might want to switch to git, but it shows that "undo"
has its own ambiguities; gk uses "undo" the same way we currently use
"git revert".

For people who argue that "git cherry-pick --revert" or "git
cherry-pick -R" is too long, I'd argue that for most people its not a
common command, and for those for which it is common, they can always
make in alias for "git pick".

						- Ted
--

From: Andreas Ericsson
Date: Saturday, November 1, 2008 - 4:53 am

Probably "git unpick" in that case.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231
--

From: Alex Riesen
Date: Friday, October 31, 2008 - 9:50 am

"git revert" is much shorter to type than "git cherry-pick -R".
How about renaming "cherry-pick" into something short, like "pick"?

--

From: Pierre Habouzit
Date: Friday, October 31, 2008 - 9:58 am

Do you really use git revert _that_ often ? I don't. And cherry-pick is
a really usual name for the tool.

FWIW the basic idea is to deprecate revert in a (not so ?) long time,
and leave git revert unimplemented for ever so that people that would
like it to be 'git checkout HEAD --' alias it to that, and the ones that
want to keep the current behaviour alias it to 'git cherry-pick -R'

--=20
=C2=B7O=C2=B7  Pierre Habouzit
=C2=B7=C2=B7O                                                madcoder@debia=
n.org
OOO                                                http://www.madism.org
From: Alex Riesen
Date: Friday, October 31, 2008 - 4:24 pm

Well, I kind of got used to it. And it makes sense (as does -R by
cherry-pick, I have to admit). I have no other argument against the
change.

--

From: Johannes Schindelin
Date: Friday, October 31, 2008 - 4:13 pm

Hi,


I thought we agreed that we should _never_ remove support for "git 
revert"?  I mean, we can deprecate it, but I find it pretty strong, and 
unnecessary, to break existing users' expectations.

Ciao,
Dscho

--

From: Junio C Hamano
Date: Friday, October 31, 2008 - 4:20 pm

Likewise.

The current state of affairs is that there is no remedy if teachers find
"git checkout -- path" or "git revert HEAD~24" is confusing to new people.
By introducing "git unstage path" or "git cherry-pick -r HEAD~24",
teachers can choose to teach what they feel less confusing, and they do
not have to teach "git checkout -- path" or "git revert HEAD~24".  We
should stop there.



--

From: Matthieu Moy
Date: Saturday, November 1, 2008 - 4:01 pm

I think you should go half a step further: officially deprecate
"revert", but keep it supported forever. The reason is just to help
documentation to be homogeneous (I foresee questions of users having
read here about "cherry-pick -R" and there about "revert" and asking
"should I use one or the other"). At least, the man page for "revert"
should state explicitly "this is a convenience alias for ...".

But I agree that removing support for "revert" is probably useless and
somehow harmfull.

(my 2 cents ...)

-- 
Matthieu
--

From: Nguyen Thai Ngoc Duy
Date: Sunday, November 2, 2008 - 2:32 am

Maybe a patch like this can help? With it you can type "git cp" for
cherry-pick. If someday "git cp" is added, you can type "git c-p",
much shorter.

--<--
commit dce5cad329390905bb91115a9de0153772be57d8
Author: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Date:   Sat Nov 1 17:12:04 2008 +0700

    Add git command expansion
    
    This allows git commands to be typed shorter (in shells that do not
    support autocompletion). There are three types of expansion:
    
     - "foo" matches "foo*" commands (bi -> bisect)
     - "foo" also matches "f*-oo*" (fim -> fast-import)
     - "foo-bar" (with dash) matches "foo*-bar*" (fo-p -> format-patch)
    
    This feature is only enabled if core.commandexpansion is true. It
    may work better if we can limit the command set (to porcelain
    only for example) but I have yet to find a way to pull
    commands-list.txt to help.c.

diff --git a/builtin.h b/builtin.h
index 1495cf6..9fb0fef 100644
--- a/builtin.h
+++ b/builtin.h
@@ -12,6 +12,7 @@ extern const char git_more_info_string[];
 
 extern void list_common_cmds_help(void);
 extern const char *help_unknown_cmd(const char *cmd);
+extern const char *expand_command(const char *cmd);
 extern void prune_packed_objects(int);
 extern int read_line_with_nul(char *buf, int size, FILE *file);
 extern int fmt_merge_msg(int merge_summary, struct strbuf *in,
diff --git a/git.c b/git.c
index 89feb0b..1bbe340 100644
--- a/git.c
+++ b/git.c
@@ -14,6 +14,7 @@ struct pager_config {
 	const char *cmd;
 	int val;
 };
+static int command_expansion;
 
 static int pager_command_config(const char *var, const char *value, void *data)
 {
@@ -415,6 +416,13 @@ static void execv_dashed_external(const char **argv)
 	strbuf_release(&cmd);
 }
 
+static int git_command_expansion_config(const char *var, const char *value, void *cb)
+{
+	if (!strcmp(var, "core.commandexpansion"))
+		command_expansion = git_config_bool(var, value);
+
+	return git_default_config(var, value, cb);
+}
 
 int ...
From: Johannes Schindelin
Date: Sunday, November 2, 2008 - 9:12 am

Hi,


I'd rather have the soft-alias code back to perform this expansion, but 
only for a limited and explicit set of abbreviations.

Ciao,
Dscho

--

From: Jeff King
Date: Saturday, November 1, 2008 - 9:41 pm

I disagree. I write a new commit message for every revert I do.

When you cherry-pick, you are pulling a good commit from somewhere else.
So its commit message should suffice to explain why you are making the
change (and infrequently, you might want to give more context or say
"and here is where this comes from").

But when you revert, you are saying "this other commit was bad, so let's
reverse it." So you can look at the other commit to see what it did, but
you still don't know _why_ it was bad. A revert should always give
information about what you know _now_ that you didn't know when you
made the commit originally.

-Peff
--

From: Pierre Habouzit
Date: Sunday, November 2, 2008 - 2:30 am

Indeed that makes sense, I'll update the patch then, and be lighter on
the deprecation side since it seems I misunderstood what people agreed
on.

--=20
=C2=B7O=C2=B7  Pierre Habouzit
=C2=B7=C2=B7O                                                madcoder@debia=
n.org
OOO                                                http://www.madism.org
Previous thread: Re: jgit as a jira plugin by Shawn O. Pearce on Friday, October 31, 2008 - 7:42 am. (1 message)

Next thread: large publicly accessible HTTP archive? by Bryan Larsen on Friday, October 31, 2008 - 10:07 am. (2 messages)