git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] prepare deprecation of git-revert
@ 2008-10-31 15:55 Pierre Habouzit
  2008-10-31 15:57 ` Pierre Habouzit
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Pierre Habouzit @ 2008-10-31 15:55 UTC (permalink / raw)
  To: git; +Cc: Pierre Habouzit

* 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 used to be that the command defaulted to do `-x`
 	described above, and `-r` was to disable it.  Now the
diff --git a/Makefile b/Makefile
index d6f3695..43eb8e4 100644
--- a/Makefile
+++ b/Makefile
@@ -306,7 +306,7 @@ PROGRAMS += git-var$X
 # builtin-$C.o but is linked in as part of some other command.
 BUILT_INS += $(patsubst builtin-%.o,git-%$X,$(BUILTIN_OBJS))
 
-BUILT_INS += git-cherry-pick$X
+BUILT_INS += git-revert$X
 BUILT_INS += git-cherry$X
 BUILT_INS += git-format-patch$X
 BUILT_INS += git-fsck-objects$X
@@ -508,6 +508,7 @@ BUILTIN_OBJS += builtin-check-attr.o
 BUILTIN_OBJS += builtin-check-ref-format.o
 BUILTIN_OBJS += builtin-checkout-index.o
 BUILTIN_OBJS += builtin-checkout.o
+BUILTIN_OBJS += builtin-cherry-pick.o
 BUILTIN_OBJS += builtin-clean.o
 BUILTIN_OBJS += builtin-clone.o
 BUILTIN_OBJS += builtin-commit-tree.o
@@ -556,7 +557,6 @@ BUILTIN_OBJS += builtin-rerere.o
 BUILTIN_OBJS += builtin-reset.o
 BUILTIN_OBJS += builtin-rev-list.o
 BUILTIN_OBJS += builtin-rev-parse.o
-BUILTIN_OBJS += builtin-revert.o
 BUILTIN_OBJS += builtin-rm.o
 BUILTIN_OBJS += builtin-send-pack.o
 BUILTIN_OBJS += builtin-shortlog.o
@@ -1261,7 +1261,7 @@ git-http-push$X: revision.o http.o http-push.o $(GITLIBS)
 
 $(LIB_OBJS) $(BUILTIN_OBJS): $(LIB_H)
 $(patsubst git-%$X,%.o,$(PROGRAMS)): $(LIB_H) $(wildcard */*.h)
-builtin-revert.o wt-status.o: wt-status.h
+builtin-cherry-pick.o wt-status.o: wt-status.h
 
 $(LIB_FILE): $(LIB_OBJS)
 	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(LIB_OBJS)
diff --git a/builtin-revert.c b/builtin-cherry-pick.c
similarity index 98%
rename from builtin-revert.c
rename to builtin-cherry-pick.c
index 4038b41..d1a7188 100644
--- a/builtin-revert.c
+++ b/builtin-cherry-pick.c
@@ -57,6 +57,7 @@ static void parse_args(int argc, const char **argv)
 		OPT_BOOLEAN('r', NULL, &noop, "no-op (backward compatibility)"),
 		OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
 		OPT_INTEGER('m', "mainline", &mainline, "parent number"),
+		OPT_SET_INT('R', "revert", &action, "cherry-pick a reverted patch", REVERT),
 		OPT_END(),
 	};
 
@@ -261,10 +262,6 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	setenv(GIT_REFLOG_ACTION, me, 0);
 	parse_args(argc, argv);
 
-	/* this is copied from the shell script, but it's never triggered... */
-	if (action == REVERT && !no_replay)
-		die("revert is incompatible with replay");
-
 	if (read_cache() < 0)
 		die("git %s: failed to read the index", me);
 	if (no_commit) {
@@ -439,16 +436,17 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
+#if 0
+	warning("git revert is deprecated, please use git cherry-pick --revert/-R instead");
+#endif
 	if (isatty(0))
 		edit = 1;
-	no_replay = 1;
 	action = REVERT;
 	return revert_or_cherry_pick(argc, argv);
 }
 
 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);
 }
-- 
1.6.0.3.790.ga4dd7.dirty

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] prepare deprecation of git-revert
  2008-10-31 15:55 [PATCH] prepare deprecation of git-revert Pierre Habouzit
@ 2008-10-31 15:57 ` Pierre Habouzit
  2008-10-31 16:36 ` Jakub Narebski
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 16+ messages in thread
From: Pierre Habouzit @ 2008-10-31 15:57 UTC (permalink / raw)
  To: git

[-- Attachment #1: Type: text/plain, Size: 2442 bytes --]

On Fri, Oct 31, 2008 at 03:55:27PM +0000, Pierre Habouzit wrote:
> * 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
         ^ this was supposed to spell out 'git cherry-pick -R' of course :/
> +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.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] prepare deprecation of git-revert
  2008-10-31 15:55 [PATCH] prepare deprecation of git-revert Pierre Habouzit
  2008-10-31 15:57 ` Pierre Habouzit
@ 2008-10-31 16:36 ` Jakub Narebski
  2008-10-31 16:54   ` Pierre Habouzit
  2008-10-31 19:01   ` Theodore Tso
  2008-10-31 16:50 ` Alex Riesen
  2008-11-02  4:41 ` Jeff King
  3 siblings, 2 replies; 16+ messages in thread
From: Jakub Narebski @ 2008-10-31 16:36 UTC (permalink / raw)
  To: Pierre Habouzit; +Cc: git

Pierre Habouzit <madcoder@debian.org> writes:

> * 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.

> +#if 0
> +	warning("git revert is deprecated, please use git cherry-pick --revert/-R instead");
> +#endif

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] prepare deprecation of git-revert
  2008-10-31 15:55 [PATCH] prepare deprecation of git-revert Pierre Habouzit
  2008-10-31 15:57 ` Pierre Habouzit
  2008-10-31 16:36 ` Jakub Narebski
@ 2008-10-31 16:50 ` Alex Riesen
  2008-10-31 16:58   ` Pierre Habouzit
                     ` (2 more replies)
  2008-11-02  4:41 ` Jeff King
  3 siblings, 3 replies; 16+ messages in thread
From: Alex Riesen @ 2008-10-31 16:50 UTC (permalink / raw)
  To: Pierre Habouzit; +Cc: git

Pierre Habouzit, Fri, Oct 31, 2008 16:55:27 +0100:
> @@ -439,16 +436,17 @@ static int revert_or_cherry_pick(int argc, const char **argv)
>  
>  int cmd_revert(int argc, const char **argv, const char *prefix)
>  {
> +#if 0
> +	warning("git revert is deprecated, please use git cherry-pick --revert/-R instead");
> +#endif

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] prepare deprecation of git-revert
  2008-10-31 16:36 ` Jakub Narebski
@ 2008-10-31 16:54   ` Pierre Habouzit
  2008-10-31 19:01   ` Theodore Tso
  1 sibling, 0 replies; 16+ messages in thread
From: Pierre Habouzit @ 2008-10-31 16:54 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git

[-- Attachment #1: Type: text/plain, Size: 1379 bytes --]

On Fri, Oct 31, 2008 at 04:36:33PM +0000, Jakub Narebski wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > * 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.
> 
> > +#if 0
> > +	warning("git revert is deprecated, please use git cherry-pick --revert/-R instead");
> > +#endif
> 
> 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.

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.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] prepare deprecation of git-revert
  2008-10-31 16:50 ` Alex Riesen
@ 2008-10-31 16:58   ` Pierre Habouzit
  2008-10-31 23:24     ` Alex Riesen
  2008-10-31 23:13   ` Johannes Schindelin
  2008-11-02  9:32   ` Nguyen Thai Ngoc Duy
  2 siblings, 1 reply; 16+ messages in thread
From: Pierre Habouzit @ 2008-10-31 16:58 UTC (permalink / raw)
  To: Alex Riesen; +Cc: git

[-- Attachment #1: Type: text/plain, Size: 1125 bytes --]

On Fri, Oct 31, 2008 at 04:50:03PM +0000, Alex Riesen wrote:
> Pierre Habouzit, Fri, Oct 31, 2008 16:55:27 +0100:
> > @@ -439,16 +436,17 @@ static int revert_or_cherry_pick(int argc, const char **argv)
> >  
> >  int cmd_revert(int argc, const char **argv, const char *prefix)
> >  {
> > +#if 0
> > +	warning("git revert is deprecated, please use git cherry-pick --revert/-R instead");
> > +#endif
> 
> "git revert" is much shorter to type than "git cherry-pick -R".
> How about renaming "cherry-pick" into something short, like "pick"?

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'

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

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] prepare deprecation of git-revert
  2008-10-31 16:36 ` Jakub Narebski
  2008-10-31 16:54   ` Pierre Habouzit
@ 2008-10-31 19:01   ` Theodore Tso
  2008-11-01 11:53     ` Andreas Ericsson
  1 sibling, 1 reply; 16+ messages in thread
From: Theodore Tso @ 2008-10-31 19:01 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: Pierre Habouzit, git

On Fri, Oct 31, 2008 at 09:36:33AM -0700, Jakub Narebski wrote:
> > +#if 0
> > +	warning("git revert is deprecated, please use git cherry-pick --revert/-R instead");
> > +#endif
> 
> By the way, Mercurial names this command IIRC 'hg backout'. 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] prepare deprecation of git-revert
  2008-10-31 16:50 ` Alex Riesen
  2008-10-31 16:58   ` Pierre Habouzit
@ 2008-10-31 23:13   ` Johannes Schindelin
  2008-10-31 23:20     ` Junio C Hamano
  2008-11-02  9:32   ` Nguyen Thai Ngoc Duy
  2 siblings, 1 reply; 16+ messages in thread
From: Johannes Schindelin @ 2008-10-31 23:13 UTC (permalink / raw)
  To: Alex Riesen; +Cc: Pierre Habouzit, git

Hi,

On Fri, 31 Oct 2008, Alex Riesen wrote:

> Pierre Habouzit, Fri, Oct 31, 2008 16:55:27 +0100:
> > @@ -439,16 +436,17 @@ static int revert_or_cherry_pick(int argc, const char **argv)
> >  
> >  int cmd_revert(int argc, const char **argv, const char *prefix)
> >  {
> > +#if 0
> > +	warning("git revert is deprecated, please use git cherry-pick --revert/-R instead");
> > +#endif
> 
> "git revert" is much shorter to type than "git cherry-pick -R". How 
> about renaming "cherry-pick" into something short, like "pick"?

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] prepare deprecation of git-revert
  2008-10-31 23:13   ` Johannes Schindelin
@ 2008-10-31 23:20     ` Junio C Hamano
  2008-11-01 23:01       ` Matthieu Moy
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2008-10-31 23:20 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Alex Riesen, Pierre Habouzit, git

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Fri, 31 Oct 2008, Alex Riesen wrote:
>
>> Pierre Habouzit, Fri, Oct 31, 2008 16:55:27 +0100:
>> > @@ -439,16 +436,17 @@ static int revert_or_cherry_pick(int argc, const char **argv)
>> >  
>> >  int cmd_revert(int argc, const char **argv, const char *prefix)
>> >  {
>> > +#if 0
>> > +	warning("git revert is deprecated, please use git cherry-pick --revert/-R instead");
>> > +#endif
>> 
>> "git revert" is much shorter to type than "git cherry-pick -R". How 
>> about renaming "cherry-pick" into something short, like "pick"?
>
> 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.

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] prepare deprecation of git-revert
  2008-10-31 16:58   ` Pierre Habouzit
@ 2008-10-31 23:24     ` Alex Riesen
  0 siblings, 0 replies; 16+ messages in thread
From: Alex Riesen @ 2008-10-31 23:24 UTC (permalink / raw)
  To: Pierre Habouzit; +Cc: git

Pierre Habouzit, Fri, Oct 31, 2008 17:58:14 +0100:
> On Fri, Oct 31, 2008 at 04:50:03PM +0000, Alex Riesen wrote:
> > Pierre Habouzit, Fri, Oct 31, 2008 16:55:27 +0100:
> > > @@ -439,16 +436,17 @@ static int revert_or_cherry_pick(int argc, const char **argv)
> > >  
> > >  int cmd_revert(int argc, const char **argv, const char *prefix)
> > >  {
> > > +#if 0
> > > +	warning("git revert is deprecated, please use git cherry-pick --revert/-R instead");
> > > +#endif
> > 
> > "git revert" is much shorter to type than "git cherry-pick -R".
> > How about renaming "cherry-pick" into something short, like "pick"?
> 
> Do you really use git revert _that_ often ? I don't. And cherry-pick is
> a really usual name for the tool.

Have it 5 times in my bash history of 20k lines. 4 recent, one relatively old.

> 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'

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] prepare deprecation of git-revert
  2008-10-31 19:01   ` Theodore Tso
@ 2008-11-01 11:53     ` Andreas Ericsson
  0 siblings, 0 replies; 16+ messages in thread
From: Andreas Ericsson @ 2008-11-01 11:53 UTC (permalink / raw)
  To: Theodore Tso; +Cc: Jakub Narebski, Pierre Habouzit, git

Theodore Tso wrote:
> On Fri, Oct 31, 2008 at 09:36:33AM -0700, Jakub Narebski wrote:
>>> +#if 0
>>> +	warning("git revert is deprecated, please use git cherry-pick --revert/-R instead");
>>> +#endif
>> By the way, Mercurial names this command IIRC 'hg backout'. 
> 
> 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".
> 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] prepare deprecation of git-revert
  2008-10-31 23:20     ` Junio C Hamano
@ 2008-11-01 23:01       ` Matthieu Moy
  0 siblings, 0 replies; 16+ messages in thread
From: Matthieu Moy @ 2008-11-01 23:01 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, Alex Riesen, Pierre Habouzit, git

Junio C Hamano <gitster@pobox.com> writes:

> 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.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] prepare deprecation of git-revert
  2008-10-31 15:55 [PATCH] prepare deprecation of git-revert Pierre Habouzit
                   ` (2 preceding siblings ...)
  2008-10-31 16:50 ` Alex Riesen
@ 2008-11-02  4:41 ` Jeff King
  2008-11-02  9:30   ` Pierre Habouzit
  3 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2008-11-02  4:41 UTC (permalink / raw)
  To: Pierre Habouzit; +Cc: git

On Fri, Oct 31, 2008 at 04:55:27PM +0100, Pierre Habouzit wrote:

>  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.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] prepare deprecation of git-revert
  2008-11-02  4:41 ` Jeff King
@ 2008-11-02  9:30   ` Pierre Habouzit
  0 siblings, 0 replies; 16+ messages in thread
From: Pierre Habouzit @ 2008-11-02  9:30 UTC (permalink / raw)
  To: Jeff King; +Cc: git

[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]

On Sun, Nov 02, 2008 at 04:41:59AM +0000, Jeff King wrote:
> On Fri, Oct 31, 2008 at 04:55:27PM +0100, Pierre Habouzit wrote:
> 
> >  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.
> 
> 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.

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.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] prepare deprecation of git-revert
  2008-10-31 16:50 ` Alex Riesen
  2008-10-31 16:58   ` Pierre Habouzit
  2008-10-31 23:13   ` Johannes Schindelin
@ 2008-11-02  9:32   ` Nguyen Thai Ngoc Duy
  2008-11-02 16:12     ` Johannes Schindelin
  2 siblings, 1 reply; 16+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2008-11-02  9:32 UTC (permalink / raw)
  To: Alex Riesen; +Cc: Pierre Habouzit, git

On Fri, Oct 31, 2008 at 05:50:03PM +0100, Alex Riesen wrote:
> Pierre Habouzit, Fri, Oct 31, 2008 16:55:27 +0100:
> > @@ -439,16 +436,17 @@ static int revert_or_cherry_pick(int argc, const char **argv)
> >  
> >  int cmd_revert(int argc, const char **argv, const char *prefix)
> >  {
> > +#if 0
> > +	warning("git revert is deprecated, please use git cherry-pick --revert/-R instead");
> > +#endif
> 
> "git revert" is much shorter to type than "git cherry-pick -R".
> How about renaming "cherry-pick" into something short, like "pick"?

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 main(int argc, const char **argv)
 {
@@ -501,6 +509,15 @@ int main(int argc, const char **argv)
 				cmd, argv[0]);
 			exit(1);
 		}
+		git_config(git_command_expansion_config, NULL);
+		if (command_expansion) {
+			const char *expand_cmd = expand_command(cmd);
+			if (expand_cmd) {
+				argv[0] = expand_cmd;
+				handle_internal_command(argc, argv);
+				execv_dashed_external(argv);
+			}
+		}
 		argv[0] = help_unknown_cmd(cmd);
 		handle_internal_command(argc, argv);
 		execv_dashed_external(argv);
diff --git a/help.c b/help.c
index fd87bb5..4f0e5a0 100644
--- a/help.c
+++ b/help.c
@@ -359,6 +359,67 @@ const char *help_unknown_cmd(const char *cmd)
 	exit(1);
 }
 
+const char *expand_command(const char *cmd)
+{
+	int i, n, len;
+	struct cmdnames main_cmds, other_cmds;
+	char *src, *dst;
+
+	memset(&main_cmds, 0, sizeof(main_cmds));
+	memset(&other_cmds, 0, sizeof(main_cmds));
+
+	load_command_list("git-", &main_cmds, &other_cmds);
+
+	add_cmd_list(&main_cmds, &aliases);
+	add_cmd_list(&main_cmds, &other_cmds);
+	qsort(main_cmds.names, main_cmds.cnt,
+	      sizeof(main_cmds.names), cmdname_compare);
+	uniq(&main_cmds);
+
+	len = strlen(cmd);
+	n = -1;
+	src = strchr(cmd, '-');
+	for (i = 0;i < main_cmds.cnt; i++) {
+		const char *gitcmd = main_cmds.names[i]->name;
+
+		/* match prefix */
+		if (!strncmp(cmd, gitcmd, len))
+			goto ok_expand;
+
+		if (*cmd != *gitcmd)
+			continue;
+		dst = strchr(gitcmd, '-');
+		if (!dst)
+			continue;
+
+		/* cmd is foo-bar, match foo*-bar* */
+		if (src &&
+		    !strncmp(cmd, gitcmd, src-cmd) &&
+		    !strncmp(src+1, dst+1, cmd+len-src-1))
+			goto ok_expand;
+
+		/* cmd is foobar,match f*-oobar* */
+		if (!src && !strncmp(cmd+1, dst+1, len-1))
+			goto ok_expand;
+
+		continue;
+ok_expand:
+		trace_printf("expand: %s\n", main_cmds.names[i]->name);
+		if (n != -1)
+			return NULL;
+		n = i;
+	}
+
+	if (n != -1) {
+		const char *assumed = main_cmds.names[n]->name;
+		main_cmds.names[n] = NULL;
+		clean_cmdnames(&main_cmds);
+		return assumed;
+	}
+	else
+		return NULL;
+}
+
 int cmd_version(int argc, const char **argv, const char *prefix)
 {
 	printf("git version %s\n", git_version_string);
--<--
-- 
Duy

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] prepare deprecation of git-revert
  2008-11-02  9:32   ` Nguyen Thai Ngoc Duy
@ 2008-11-02 16:12     ` Johannes Schindelin
  0 siblings, 0 replies; 16+ messages in thread
From: Johannes Schindelin @ 2008-11-02 16:12 UTC (permalink / raw)
  To: Nguyen Thai Ngoc Duy; +Cc: Alex Riesen, Pierre Habouzit, git

Hi,

On Sun, 2 Nov 2008, Nguyen Thai Ngoc Duy wrote:

>     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)

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2008-11-02 16:06 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-10-31 15:55 [PATCH] prepare deprecation of git-revert Pierre Habouzit
2008-10-31 15:57 ` Pierre Habouzit
2008-10-31 16:36 ` Jakub Narebski
2008-10-31 16:54   ` Pierre Habouzit
2008-10-31 19:01   ` Theodore Tso
2008-11-01 11:53     ` Andreas Ericsson
2008-10-31 16:50 ` Alex Riesen
2008-10-31 16:58   ` Pierre Habouzit
2008-10-31 23:24     ` Alex Riesen
2008-10-31 23:13   ` Johannes Schindelin
2008-10-31 23:20     ` Junio C Hamano
2008-11-01 23:01       ` Matthieu Moy
2008-11-02  9:32   ` Nguyen Thai Ngoc Duy
2008-11-02 16:12     ` Johannes Schindelin
2008-11-02  4:41 ` Jeff King
2008-11-02  9:30   ` Pierre Habouzit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).