[PATCH] Make git revert warn the user when reverting a merge commit.

[PATCH] Make git revert warn the user when reverting a merge commit.

[Boyd Stephen Smith Jr.]

Signed-off-by: Boyd Stephen Smith Jr <bss@iguanasuicide.net>
---
On Thursday 2008 December 18 18:21:25 Linus Torvalds wrote:
> I suspect we should warn about reverting merges.

Here is a patch (aginst c0ceb2c, which I believe is master currently) that
does just that.

After applying the patch I get the following test results:
fixed   1
success 4108
failed  0
broken  4
total   4113

 builtin-revert.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/builtin-revert.c b/builtin-revert.c
index 4038b41..7f121a5 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -296,6 +296,21 @@ static int revert_or_cherry_pick(int argc, const char 
**argv)
 		int cnt;
 		struct commit_list *p;
 
+		do {
+			switch (action) {
+			case REVERT:
+				warning("revert on a merge commit may not do what you expect.");
+				continue;
+			case CHERRY_PICK:
+				/* Cherry picking a merge doesn't merge the history, but
+				 * I don't think many people expect that.
+				 */
+				continue;
+			}
+			/* Unhandled enum member. */
+			die("Unknown action on a merge commit.");
+		} while (0);
+
 		if (!mainline)
 			die("Commit %s is a merge but no -m option was given.",
 			    sha1_to_hex(commit->object.sha1));
-- 
1.5.6
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss@iguanasuicide.net                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Johannes Schindelin]

Hi,

On Thu, 18 Dec 2008, Boyd Stephen Smith Jr. wrote:

> +		do {
> +			switch (action) {
> +			case REVERT:
> +				warning("revert on a merge commit may not do what you expect.");
> +				continue;
> +			case CHERRY_PICK:
> +				/* Cherry picking a merge doesn't merge the history, but
> +				 * I don't think many people expect that.
> +				 */
> +				continue;
> +			}
> +			/* Unhandled enum member. */
> +			die("Unknown action on a merge commit.");
> +		} while (0);
> +

Wow.  That must be one of the, uhm, less beautiful ways to write

		if (action == REVERT)
			warning("revert on a merge commit may not do what you "
				"expect.");
		else if (action != CHERRY_PICK)
			die("Unknown action on a merge commit.");

Besides, I am actually pretty much against this change.  You already have 
to ask very explicitely to revert a merge, by specifying a parent number.  
If I ask for something explicitely, I do not want the tool to tell me that 
it's dangerous.  I know that already, thankyouverymuch.

Ciao,
Dscho

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Junio C Hamano]

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

> Wow.  That must be one of the, uhm, less beautiful ways to write
>
> 		if (action == REVERT)
> 			warning("revert on a merge commit may not do what you "
> 				"expect.");
> 		else if (action != CHERRY_PICK)
> 			die("Unknown action on a merge commit.");
>
> Besides, I am actually pretty much against this change.  You already have 
> to ask very explicitely to revert a merge, by specifying a parent number.  
> If I ask for something explicitely, I do not want the tool to tell me that 
> it's dangerous.  I know that already, thankyouverymuch.

Or you may not have known that it is dangerous, but the new warning does
not give you enough clue where to go next, so this warning does not give
real value.  It is pretty much meaningless noise to users.

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Boyd Stephen Smith Jr.]

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

On Thursday 2008 December 18 21:03:46 Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 			warning("revert on a merge commit may not do what you "
> > 				"expect.");
>
> [T]he new warning does
> not give you enough clue where to go next, so this warning does not give
> real value.  It is pretty much meaningless noise to users.

At least, it might make someone read the manpage again.  Still, I'm unhappy 
with the message, but I didn't want to be too wordy.  A URL or manpage 
reference would be nice, but I didn't know of a good guide that explained the 
dangers of reverting a merge commit as well as Linus's emails.
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss@iguanasuicide.net                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Jay Soffian]

On Thu, Dec 18, 2008 at 10:29 PM, Boyd Stephen Smith Jr.
<bss@iguanasuicide.net> wrote:
> At least, it might make someone read the manpage again.  Still, I'm unhappy
> with the message, but I didn't want to be too wordy.  A URL or manpage
> reference would be nice, but I didn't know of a good guide that explained the
> dangers of reverting a merge commit as well as Linus's emails.

Put his email in Documentation/howto/undoing-merge-commits.txt and
reference that?

j.

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Boyd Stephen Smith Jr.]

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

On Thursday 2008 December 18 21:55:13 Jay Soffian wrote:
> On Thu, Dec 18, 2008 at 10:29 PM, Boyd Stephen Smith Jr.
>
> <bss@iguanasuicide.net> wrote:
> > At least, it might make someone read the manpage again.  Still, I'm
> > unhappy with the message, but I didn't want to be too wordy.  A URL or
> > manpage reference would be nice, but I didn't know of a good guide that
> > explained the dangers of reverting a merge commit as well as Linus's
> > emails.
>
> Put his email in Documentation/howto/undoing-merge-commits.txt and
> reference that?

Okay, I've got a documentation patch brewing, but it's too late here to work 
on it more.  I'll post it over the weekend.

In addition, I think a one-time-per-user warning would be nice, but I'm not 
sure the best way to implement that.  My initial thoughts would be reading a 
boolean config option, if unset/true issuing the warning and then if unset 
set it to false.  However, that seems a bit... unclean and I fear there might 
be a policy against writing ~/.gitconfig configuration options from a 
subcommand other than 'git config'.  Any suggestions on the implementation?
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss@iguanasuicide.net                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Junio C Hamano]

"Boyd Stephen Smith Jr." <bss@iguanasuicide.net> writes:

> In addition, I think a one-time-per-user warning would be nice, but I'm not 
> sure the best way to implement that.  My initial thoughts would be reading a 
> boolean config option, if unset/true issuing the warning and then if unset 
> set it to false.  However, that seems a bit... unclean and I fear there might 
> be a policy against writing ~/.gitconfig configuration options from a 
> subcommand other than 'git config'.  Any suggestions on the implementation?

As an end user, I find one-time-per-user warning more frustrating than it
is worth.  I may see the warning issued for the first time of my using
certain feature, and because I am so novice to the program suite that I do
not fully understand what the warning is trying to say when I see it.
Thanks to the "one-time-per-user"-ness, that is the only chance for me to
see the message --- which often means that I won't see the warning before
the gravity of it has any chance to really sink in my mind.

"You can set i-know-what-i-am-doing in your ~/.xyzzyconfig file to squelch
this message" is slightly better, as (1) I can control when I stop seeing
it, and (2) because setting that in my config is done by me, as opposed to
the tool doing behind my back, it is much more likely for me to recall how
to get the warning back when I choose to see it again.

The above discussion is "in general".  In this particular case, I am not
convinced if the warning itself is worth it, though.

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Alan]

On Thu, 2008-12-18 at 21:29 -0600, Boyd Stephen Smith Jr. wrote:
> On Thursday 2008 December 18 21:03:46 Junio C Hamano wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > > 			warning("revert on a merge commit may not do what you "
> > > 				"expect.");
> >
> > [T]he new warning does
> > not give you enough clue where to go next, so this warning does not give
> > real value.  It is pretty much meaningless noise to users.
> 
> At least, it might make someone read the manpage again.  Still, I'm unhappy 
> with the message, but I didn't want to be too wordy.  A URL or manpage 
> reference would be nice, but I didn't know of a good guide that explained the 
> dangers of reverting a merge commit as well as Linus's emails.

That would be OK if the man page actually explained how this is supposed
to work.  it does not.  (Especially where it concerns "parent number"
and reverts of merges, which has no real explanation.)

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Boyd Stephen Smith Jr.]

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

Blah, my --in-reply-to didn't work so this didn't thread right.

On Thursday 2008 December 18 20:57:57 you wrote:
> On Thu, 18 Dec 2008, Boyd Stephen Smith Jr. wrote:
> > +		do {
> > +			switch (action) {
> > +			case REVERT:
> > +				warning("revert on a merge commit may not do what you expect.");
> > +				continue;
> > +			case CHERRY_PICK:
> > +				/* Cherry picking a merge doesn't merge the history, but
> > +				 * I don't think many people expect that.
> > +				 */
> > +				continue;
> > +			}
> > +			/* Unhandled enum member. */
> > +			die("Unknown action on a merge commit.");
> > +		} while (0);
> > +
>
> Wow.  That must be one of the, uhm, less beautiful ways to write
>
> 		if (action == REVERT)
> 			warning("revert on a merge commit may not do what you "
> 				"expect.");
> 		else if (action != CHERRY_PICK)
> 			die("Unknown action on a merge commit.");

My way, a smart compiler will warn at compile time that there's a new enum 
member that needs to be handled.  Your way, no such compile-time warning will 
be emitted.  At runtime, they have the same behavior.  Athestically, I agree 
with you, but my way may have technical advantages.

I did check the CodingGuidelines and didn't see this construct mentioned.

> Besides, I am actually pretty much against this change.

I've never had a need to revert a merge commit, so it's not a big win either 
way for me.  I wrote the patch because alan@clueserver.org had the revert 
behavior bite him and Linus suggested a warning might be apropos.
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss@iguanasuicide.net                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Robin Rosenberg]

fredag 19 december 2008 03:39:15 skrev Boyd Stephen Smith Jr.:
> Signed-off-by: Boyd Stephen Smith Jr <bss@iguanasuicide.net>
> ---
> On Thursday 2008 December 18 18:21:25 Linus Torvalds wrote:
> > I suspect we should warn about reverting merges.
> 

Or mention the reverted parent in the commit message since it is not obvious.

-- robin

>From e982c8cefcdeefd6e8aabc8c354bed69161f40ee Mon Sep 17 00:00:00 2001
From: Robin Rosenberg <robin.rosenberg@dewire.com>
Date: Sat, 20 Dec 2008 07:22:39 +0100
Subject: [PATCH] Mention reverted parent in commit message for reverted merge.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 builtin-revert.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/builtin-revert.c b/builtin-revert.c
index 4038b41..fc59229 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -352,6 +352,10 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		add_to_msg(oneline_body + 1);
 		add_to_msg("\"\n\nThis reverts commit ");
 		add_to_msg(sha1_to_hex(commit->object.sha1));
+		if (commit->parents->next) {
+			add_to_msg(" removing\ncontributions from ");
+			add_to_msg(sha1_to_hex(parent->object.sha1));
+		}
 		add_to_msg(".\n");
 	} else {
 		base = parent;
-- 
1.6.1.rc3.36.g43d5.dirty

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Boyd Stephen Smith Jr.]

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

On Saturday 2008 December 20 01:08:01 Robin Rosenberg wrote:
> fredag 19 december 2008 03:39:15 skrev Boyd Stephen Smith Jr.:
> > On Thursday 2008 December 18 18:21:25 Linus Torvalds wrote:
> > > I suspect we should warn about reverting merges.
>
> Or mention the reverted parent in the commit message since it is not
> obvious.
>
> ---
>  builtin-revert.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/builtin-revert.c b/builtin-revert.c
> index 4038b41..fc59229 100644
> --- a/builtin-revert.c
> +++ b/builtin-revert.c
> @@ -352,6 +352,10 @@ static int revert_or_cherry_pick(int argc, const char
> **argv) add_to_msg(oneline_body + 1);
>  		add_to_msg("\"\n\nThis reverts commit ");
>  		add_to_msg(sha1_to_hex(commit->object.sha1));
> +		if (commit->parents->next) {
> +			add_to_msg(" removing\ncontributions from ");
> +			add_to_msg(sha1_to_hex(parent->object.sha1));
> +		}
>  		add_to_msg(".\n");
>  	} else {
>  		base = parent;

I'm still new to the code, but parent is the "mainline" specified on the 
command-line, which (I think) is actually the parent to be reverted to, so we 
are actually removing contributions from all the *other* parents.  So, the 
message may be backward.  Because of that, I'd say the patch doesn't handle 
octopus merges well, either.
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss@iguanasuicide.net                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Robin Rosenberg]

lördag 20 december 2008 23:54:19 skrev Boyd Stephen Smith Jr.:
> On Saturday 2008 December 20 01:08:01 Robin Rosenberg wrote:
> > fredag 19 december 2008 03:39:15 skrev Boyd Stephen Smith Jr.:
> > > On Thursday 2008 December 18 18:21:25 Linus Torvalds wrote:
> > > > I suspect we should warn about reverting merges.
> >
> > Or mention the reverted parent in the commit message since it is not
> > obvious.
> >
> > ---
> >  builtin-revert.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/builtin-revert.c b/builtin-revert.c
> > index 4038b41..fc59229 100644
> > --- a/builtin-revert.c
> > +++ b/builtin-revert.c
> > @@ -352,6 +352,10 @@ static int revert_or_cherry_pick(int argc, const char
> > **argv) add_to_msg(oneline_body + 1);
> >  		add_to_msg("\"\n\nThis reverts commit ");
> >  		add_to_msg(sha1_to_hex(commit->object.sha1));
> > +		if (commit->parents->next) {
> > +			add_to_msg(" removing\ncontributions from ");
> > +			add_to_msg(sha1_to_hex(parent->object.sha1));
> > +		}
> >  		add_to_msg(".\n");
> >  	} else {
> >  		base = parent;
> 
> I'm still new to the code, but parent is the "mainline" specified on the 
> command-line, which (I think) is actually the parent to be reverted to, so we 
> are actually removing contributions from all the *other* parents.  So, the 
> message may be backward.  Because of that, I'd say the patch doesn't handle 

Indeed the message is backward. How about  "removing all contributions except from"... etc ?

An alternative, would be "removing changes relative to .." (mainline). The changes are
the contributions from all other parents. I have to huge interest in the exact phrase used.

> octopus merges well, either.

Same problem, I think.

-- robin

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> writes:

> An alternative, would be "removing changes relative to .."
> (mainline). The changes are the contributions from all other parents. I
> have to huge interest in the exact phrase used.

But that is exactly what "This reverts commit X" means, isn't it?

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Boyd Stephen Smith Jr.]

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

On Saturday 2008 December 20 20:37:16 Junio C Hamano wrote:
> Robin Rosenberg <robin.rosenberg.lists@dewire.com> writes:
> > An alternative, would be "removing changes relative to .."
> > (mainline).
>
> But that is exactly what "This reverts commit X" means, isn't it?

When X is a merge commit, the phrase "the reverts commit X" is ambiguous.  Did 
you revert the tree to X^, X^2, or X^8?  I'd be fine with "This reverts 
commit X to X^y", but we definitely need some mention of X^y.
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss@iguanasuicide.net                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Robin Rosenberg]

söndag 21 december 2008 04:11:13 skrev Boyd Stephen Smith Jr.:
> On Saturday 2008 December 20 20:37:16 Junio C Hamano wrote:
> > Robin Rosenberg <robin.rosenberg.lists@dewire.com> writes:
> > > An alternative, would be "removing changes relative to .."
> > > (mainline).
> >
> > But that is exactly what "This reverts commit X" means, isn't it?
> 
> When X is a merge commit, the phrase "the reverts commit X" is ambiguous.  Did 
> you revert the tree to X^, X^2, or X^8?  I'd be fine with "This reverts 
> commit X to X^y", but we definitely need some mention of X^y.

One could consider keeping the contributions from ^1 a special case and not
mention the parent, making it look like any revert commit. I guess most merge
reverts are like this in practice.

-- robin

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> writes:

> One could consider keeping the contributions from ^1 a special case and not
> mention the parent, making it look like any revert commit. I guess most merge
> reverts are like this in practice.

I think that makes sense.  There are cases where the mainline maintainer
punts a merge and pass the baton to a subsystem maintainer, saying "Your
tree has many conflicts with my tip, and I'd rather ask you to resolve it"
(and after such a merge, the mainline maintainer will fast forward to the
result), in which case the merge will be in the reverse direction, but
that should be rare.  Reverting such a merge later from the mainline's
point of view would involve "revert -m 2".

So if your patch is tightened a bit to record extra information only in
such a case, I think that would be an acceptable approach to the issue.

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Boyd Stephen Smith Jr.]

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

On Sunday 2008 December 21 04:09:36 Robin Rosenberg wrote:
> söndag 21 december 2008 04:11:13 skrev Boyd Stephen Smith Jr.:
> > On Saturday 2008 December 20 20:37:16 Junio C Hamano wrote:
> > > Robin Rosenberg <robin.rosenberg.lists@dewire.com> writes:
> > > > An alternative, would be "removing changes relative to .."
> > > > (mainline).
> > >
> > > But that is exactly what "This reverts commit X" means, isn't it?
> >
> > When X is a merge commit, the phrase "the reverts commit X" is ambiguous.
> >  Did you revert the tree to X^, X^2, or X^8?  I'd be fine with "This
> > reverts commit X to X^y", but we definitely need some mention of X^y.
>
> One could consider keeping the contributions from ^1 a special case and not
> mention the parent, making it look like any revert commit. I guess most
> merge reverts are like this in practice.

Then why not have "-m 1" be assumed instead of forcing the user to specify it?  
If we force the user to specify that information, shouldn't we hold the code 
to the same standard and have it output a message with that information?

I think git should mention the parent to which we reverted whenever there are 
multiple parents.
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss@iguanasuicide.net                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Junio C Hamano]

"Boyd Stephen Smith Jr." <bss@iguanasuicide.net> writes:

> Then why not have "-m 1" be assumed instead of forcing the user to specify it?  

The reason we don't is because until very recently we did not even allow
you to revert a merge relative to any parent.  We wanted to avoid
surprising people who are _relying on_ that behaviour to make sure that
they do not revert a merge by accident.

We could certainly do what you suggest to imply "-m 1" when the commit
requested to be reverted happens to be a merge, but we shouldn't be doing
that without thinking things through.  It will break people's longstanding
expectations.

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Boyd Stephen Smith Jr.]

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

On Sunday 2008 December 21 14:23:17 Junio C Hamano wrote:
> "Boyd Stephen Smith Jr." <bss@iguanasuicide.net> writes:
> > Then why not have "-m 1" be assumed instead of forcing the user to
> > specify it?
>
> The reason we don't is because until very recently we did not even allow
> you to revert a merge relative to any parent.  We wanted to avoid
> surprising people who are _relying on_ that behaviour to make sure that
> they do not revert a merge by accident.

That makes sense.

> We could certainly do what you suggest to imply "-m 1" when the commit
> requested to be reverted happens to be a merge, but we shouldn't be doing
> that without thinking things through.  It will break people's longstanding
> expectations.

I wasn't really suggesting that.  I was pointing out what I thought was an 
inconsistency: making the user specify the parent but not making the commit 
message specify the parent.

I still think the parent we are reverting to should be mentioned in the 
automatically generated commit message, even if it is the first parent.  Even 
if it is decided to elide that information for the first parent, I agree 
that, at least for now, the "-m" should still be required when reverting a 
merge commit.
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss@iguanasuicide.net                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Junio C Hamano]

From: Robin Rosenberg <robin.rosenberg.lists@dewire.com>
Subject: git-revert: record the parent against which a revert was made

As described in Documentation/howto/revert-a-faulty-merge.txt, re-merging
from a previously reverted a merge of a side branch may need a revert of
the revert beforehand.  Record against which parent the revert was made in
the commit, so that later the user can figure out what went on.

[jc: original had the logic in the message reversed, so I tweaked it.]

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
  "Boyd Stephen Smith Jr." <bss@iguanasuicide.net> writes:

  > I still think the parent we are reverting to should be mentioned in the 
  > automatically generated commit message, even if it is the first parent.  Even 
  > if it is decided to elide that information for the first parent, I agree 
  > that, at least for now, the "-m" should still be required when reverting a 
  > merge commit.

  Ok, so here is Robin's patch with a bit of rewording.  I want to have
  something usable now, so that I can tag -rc4 and still have time left
  for sipping my Caipirinha in the evening ;-)

  I think we later _could_ use this information inside ancestry traversal
  made while computing the merge base in such a way to eliminate the
  necessity of the "revert of the revert".  When we see a message that
  records a revert of a merge, we keep a mental note of it, and when we
  encounter such a merge during the ancestry traversal, we pretend as if
  the merge never happened (i.e. instead we traverse only the named
  parent).

  But that needs more thought, and we do not have to do that now.

 builtin-revert.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git c/builtin-revert.c w/builtin-revert.c
index 4038b41..fae0fe8 100644
--- c/builtin-revert.c
+++ w/builtin-revert.c
@@ -352,6 +352,11 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		add_to_msg(oneline_body + 1);
 		add_to_msg("\"\n\nThis reverts commit ");
 		add_to_msg(sha1_to_hex(commit->object.sha1));
+
+		if (commit->parents->next) {
+			add_to_msg(",\nreverting damages made to %s");
+			add_to_msg(sha1_to_hex(parent->object.sha1));
+		}
 		add_to_msg(".\n");
 	} else {
 		base = parent;

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Junio C Hamano]

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

>   Ok, so here is Robin's patch with a bit of rewording.  I want to have
>   something usable now, so that I can tag -rc4 and still have time left
>   for sipping my Caipirinha in the evening ;-)
> ...
> +			add_to_msg(",\nreverting damages made to %s");
> +			add_to_msg(sha1_to_hex(parent->object.sha1));

Crap.  Scratch that.  Obviously I should have done this:

diff --git a/builtin-revert.c b/builtin-revert.c
index 4038b41..c188150 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -352,6 +352,11 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		add_to_msg(oneline_body + 1);
 		add_to_msg("\"\n\nThis reverts commit ");
 		add_to_msg(sha1_to_hex(commit->object.sha1));
+
+		if (commit->parents->next) {
+			add_to_msg(",\nreverting damages made to ");
+			add_to_msg(sha1_to_hex(parent->object.sha1));
+		}
 		add_to_msg(".\n");
 	} else {
 		base = parent;
-- 
1.6.1.rc3.72.gf4bf6

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Robin Rosenberg]

söndag 21 december 2008 23:17:12 skrev Junio C Hamano:
> From: Robin Rosenberg <robin.rosenberg.lists@dewire.com>
> Subject: git-revert: record the parent against which a revert was made
> 
> As described in Documentation/howto/revert-a-faulty-merge.txt, re-merging
> from a previously reverted a merge of a side branch may need a revert of
> the revert beforehand.  Record against which parent the revert was made in
> the commit, so that later the user can figure out what went on.
> 
> [jc: original had the logic in the message reversed, so I tweaked it.]
No need for this comment.

> +			add_to_msg(",\nreverting damages made to %s");
maybe "changes" is more neutrral language. I also think you break
the line too early.

Are we done now?

-- robin

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> writes:

> söndag 21 december 2008 23:17:12 skrev Junio C Hamano:
>> From: Robin Rosenberg <robin.rosenberg.lists@dewire.com>
>> Subject: git-revert: record the parent against which a revert was made
>> 
>> As described in Documentation/howto/revert-a-faulty-merge.txt, re-merging
>> from a previously reverted a merge of a side branch may need a revert of
>> the revert beforehand.  Record against which parent the revert was made in
>> the commit, so that later the user can figure out what went on.
>> 
>> [jc: original had the logic in the message reversed, so I tweaked it.]
> No need for this comment.

Ok.

>> +			add_to_msg(",\nreverting damages made to %s");
> maybe "changes" is more neutrral language. I also think you break
> the line too early.

The above (without %s which shouldn't have been there) would give: 

    This reverts commit efe05b019ca19328d27c07ef32b4698a7f36166f,
    reverting damages made to ec9f0ea3e6ecf1237223dec8428e7bb73d339320.
 
Do you want:

    This reverts commit efe05b019ca19328d27c07ef32b4698a7f36166f, reversing
    changes made to ec9f0ea3e6ecf1237223dec8428e7bb73d339320.

this instead?

Re: [PATCH] Make git revert warn the user when reverting a merge commit.

[Robin Rosenberg]

söndag 21 december 2008 23:46:45 skrev Junio C Hamano:
> Do you want:
> 
>     This reverts commit efe05b019ca19328d27c07ef32b4698a7f36166f, reversing
>     changes made to ec9f0ea3e6ecf1237223dec8428e7bb73d339320.

Yes, it fills the paragraph nicely. It is a normal text flow after all.

-- robin