"git revert" message changes...

"git revert" message changes...

[Linus Torvalds]

Can we please revert the "revert" changes to the default message?

Not only did it make the one-line description much less readable by 
putting the shortened SHA1 in there (and removing the quotes), it also 
missed the empty line between the header and the body.

IOW, the old scripted message was much better. The built-in is nice, but 
the new message format is inferior.

Dscho?

		Linus

[PATCH] git-revert: Revert revert message to old behaviour

[Johannes Schindelin]

When converting from the shell script, based on a misreading of the
sed invocation, the builtin included the abbreviated commit name,
and did _not_ include the quotes around the oneline message.

This fixes it.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---

	On Fri, 23 Mar 2007, Linus Torvalds wrote:
	
	> 
	> Can we please revert the "revert" changes to the default message?
	> 
	> Dscho?

	Yeah?

 builtin-revert.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/builtin-revert.c b/builtin-revert.c
index f3f3f5c..f4e1e22 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -296,11 +296,9 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	if (action == REVERT) {
 		base = commit;
 		next = commit->parents->item;
-		add_to_msg("Revert ");
-		add_to_msg(find_unique_abbrev(commit->object.sha1,
-					DEFAULT_ABBREV));
+		add_to_msg("Revert \"");
 		add_to_msg(oneline);
-		add_to_msg("\nThis reverts commit ");
+		add_to_msg("\"\nThis reverts commit ");
 		add_to_msg(sha1_to_hex(commit->object.sha1));
 		add_to_msg(".\n");
 	} else {
-- 
1.5.1.rc1.2356.g2054

Re: [PATCH] git-revert: Revert revert message to old behaviour

[Linus Torvalds]

On Fri, 23 Mar 2007, Johannes Schindelin wrote:
> 
> When converting from the shell script, based on a misreading of the
> sed invocation, the builtin included the abbreviated commit name,
> and did _not_ include the quotes around the oneline message.
> 
> This fixes it.

How about the empty line in between the message and the "This reverts 
commit "..

> +		add_to_msg("Revert \"");
>  		add_to_msg(oneline);
> -		add_to_msg("\nThis reverts commit ");
> +		add_to_msg("\"\nThis reverts commit ");

This should probably be

	add_to_msg("\"\n\nThis reverts commit ");

with *two* \n's, no?

		Linus

Re: [PATCH] git-revert: Revert revert message to old behaviour

[Johannes Schindelin]

Hi,

On Fri, 23 Mar 2007, Linus Torvalds wrote:

> On Fri, 23 Mar 2007, Johannes Schindelin wrote:
> > 
> > When converting from the shell script, based on a misreading of the
> > sed invocation, the builtin included the abbreviated commit name,
> > and did _not_ include the quotes around the oneline message.
> > 
> > This fixes it.
> 
> How about the empty line in between the message and the "This reverts 
> commit "..
> 
> > +		add_to_msg("Revert \"");
> >  		add_to_msg(oneline);
> > -		add_to_msg("\nThis reverts commit ");
> > +		add_to_msg("\"\nThis reverts commit ");
> 
> This should probably be
> 
> 	add_to_msg("\"\n\nThis reverts commit ");
> 
> with *two* \n's, no?

Yes. Sorry.

Ciao,
Dscho

Re: [PATCH] git-revert: Revert revert message to old behaviour

[Junio C Hamano]

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

>> How about the empty line in between the message and the "This reverts 
>> commit "..
>> 
>> > +		add_to_msg("Revert \"");
>> >  		add_to_msg(oneline);
>> > -		add_to_msg("\nThis reverts commit ");
>> > +		add_to_msg("\"\nThis reverts commit ");
>> 
>> This should probably be
>> 
>> 	add_to_msg("\"\n\nThis reverts commit ");
>> 
>> with *two* \n's, no?
>
> Yes. Sorry.

Not only that.  

The older one had two, duplicated abbrev, and your fix reduces
it to once, but we do not want any abbrev there.

-- >8 --
[PATCH] git-revert: Revert revert message to old behaviour

When converting from the shell script, based on a misreading of the
sed invocation, the builtin included the abbreviated commit name,
and did _not_ include the quotes around the oneline message.

This fixes it.

[jc: with a fix for the typo/thinko spotted by Linus, and also
 removing the unwanted abbrev at the beginning.]

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 builtin-revert.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin-revert.c b/builtin-revert.c
index f3f3f5c..4ba0ee6 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -294,13 +294,13 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	oneline = get_oneline(message);
 
 	if (action == REVERT) {
+		char *oneline_body = strchr(oneline, ' ');
+
 		base = commit;
 		next = commit->parents->item;
-		add_to_msg("Revert ");
-		add_to_msg(find_unique_abbrev(commit->object.sha1,
-					DEFAULT_ABBREV));
-		add_to_msg(oneline);
-		add_to_msg("\nThis reverts commit ");
+		add_to_msg("Revert \"");
+		add_to_msg(oneline_body + 1);
+		add_to_msg("\"\n\nThis reverts commit ");
 		add_to_msg(sha1_to_hex(commit->object.sha1));
 		add_to_msg(".\n");
 	} else {
-- 
1.5.1.rc1.666.g2823

Re: [PATCH] git-revert: Revert revert message to old behaviour

[Johannes Schindelin]

Hi,

On Sat, 24 Mar 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> How about the empty line in between the message and the "This reverts 
> >> commit "..
> >> 
> >> > +		add_to_msg("Revert \"");
> >> >  		add_to_msg(oneline);
> >> > -		add_to_msg("\nThis reverts commit ");
> >> > +		add_to_msg("\"\nThis reverts commit ");
> >> 
> >> This should probably be
> >> 
> >> 	add_to_msg("\"\n\nThis reverts commit ");
> >> 
> >> with *two* \n's, no?
> >
> > Yes. Sorry.
> 
> Not only that.  
> 
> The older one had two, duplicated abbrev, and your fix reduces
> it to once, but we do not want any abbrev there.

D'oh.

Thanks,
Dscho