threads / patch / 16793

patchMake git revert warn the user when reverting a merge commit.

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

## tl;dr

24 messages between Dec 19, 2008 and Dec 21, 2008. Diffs are folded; open one to read it.

replies: 23people: 6as markdown or json

Boyd Stephen Smith Jr.· Dec 19, 2008, 02:39 UTC · lore
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(-)
Show changes to builtin-revert.c +15 −0
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/                      \_/
Johannes Schindelin· Dec 19, 2008, 02:57 UTC · re: Boyd Stephen Smith Jr. · lore

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

Hi,
On Thu, 18 Dec 2008, Boyd Stephen Smith Jr. wrote:
Show 15 quoted lines
> +		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

Junio C Hamano· Dec 19, 2008, 03:03 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 12 quoted lines
> 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.

Boyd Stephen Smith Jr.· Dec 19, 2008, 03:29 UTC · re: Junio C Hamano · lore

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

On Thursday 2008 December 18 21:03:46 Junio C Hamano wrote:
Show 7 quoted lines
> 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/                      \_/
Jay Soffian· Dec 19, 2008, 03:55 UTC · re: Boyd Stephen Smith Jr. · lore

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

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.
Boyd Stephen Smith Jr.· Dec 19, 2008, 05:54 UTC · re: Jay Soffian · lore

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

On Thursday 2008 December 18 21:55:13 Jay Soffian wrote:
Show 11 quoted lines
> 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/                      \_/
Junio C Hamano· Dec 19, 2008, 06:35 UTC · re: Boyd Stephen Smith Jr. · lore

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

"Boyd Stephen Smith Jr." <bss@iguanasuicide.net> writes:
Show 6 quoted lines
> 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.

Alan· Dec 19, 2008, 18:07 UTC · re: Boyd Stephen Smith Jr. · lore

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

On Thu, 2008-12-18 at 21:29 -0600, Boyd Stephen Smith Jr. wrote:
Show 13 quoted lines
> 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.)

Boyd Stephen Smith Jr.· Dec 19, 2008, 03:24 UTC · re: Johannes Schindelin · lore

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

Blah, my --in-reply-to didn't work so this didn't thread right.
On Thursday 2008 December 18 20:57:57 you wrote:
Show 24 quoted lines
> 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/                      \_/
Robin Rosenberg· Dec 20, 2008, 07:08 UTC · re: Boyd Stephen Smith Jr. · lore

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

fredag 19 december 2008 03:39:15 skrev Boyd Stephen Smith Jr.:
Show 5 quoted lines
> 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(-)
Show changes to builtin-revert.c +4 −0
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
Boyd Stephen Smith Jr.· Dec 20, 2008, 22:54 UTC · re: Robin Rosenberg · lore

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

On Saturday 2008 December 20 01:08:01 Robin Rosenberg wrote:
Show 26 quoted lines
> 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/                      \_/
Robin Rosenberg· Dec 20, 2008, 23:31 UTC · re: Boyd Stephen Smith Jr. · lore

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

lördag 20 december 2008 23:54:19 skrev Boyd Stephen Smith Jr.:
Show 32 quoted lines
> 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
Junio C Hamano· Dec 21, 2008, 02:37 UTC · re: Robin Rosenberg · lore

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

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?
Boyd Stephen Smith Jr.· Dec 21, 2008, 03:11 UTC · re: Junio C Hamano · lore

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

On Saturday 2008 December 20 20:37:16 Junio C Hamano wrote:
Show 5 quoted lines
> 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/                      \_/
Robin Rosenberg· Dec 21, 2008, 10:09 UTC · re: Boyd Stephen Smith Jr. · lore

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

söndag 21 december 2008 04:11:13 skrev Boyd Stephen Smith Jr.:
Show 10 quoted lines
> 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
Junio C Hamano· Dec 21, 2008, 10:59 UTC · re: Robin Rosenberg · lore

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

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.

Boyd Stephen Smith Jr.· Dec 21, 2008, 19:59 UTC · re: Robin Rosenberg · lore

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

On Sunday 2008 December 21 04:09:36 Robin Rosenberg wrote:
Show 15 quoted lines
> 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/                      \_/
Junio C Hamano· Dec 21, 2008, 20:23 UTC · re: Boyd Stephen Smith Jr. · lore

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

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

Boyd Stephen Smith Jr.· Dec 21, 2008, 21:13 UTC · re: Junio C Hamano · lore

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

On Sunday 2008 December 21 14:23:17 Junio C Hamano wrote:
Show 8 quoted lines
> "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/                      \_/
Junio C Hamano· Dec 21, 2008, 22:17 UTC · re: Boyd Stephen Smith Jr. · lore

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

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(-)
Show changes to diff +5 −0
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;
Junio C Hamano· Dec 21, 2008, 22:38 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
>   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:
Show changes to builtin-revert.c +5 −0
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
Robin Rosenberg· Dec 21, 2008, 22:40 UTC · re: Junio C Hamano · lore

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

söndag 21 december 2008 23:17:12 skrev Junio C Hamano:
Show 9 quoted lines
> 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
Junio C Hamano· Dec 21, 2008, 22:46 UTC · re: Robin Rosenberg · lore

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

Robin Rosenberg <robin.rosenberg.lists@dewire.com> writes:
Show 11 quoted lines
> 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?
Robin Rosenberg· Dec 21, 2008, 22:56 UTC · re: Junio C Hamano · lore

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

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

← back to recent threads