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

From: Boyd Stephen Smith Jr. <bss@iguanasuicide.net>
Date: Dec 19, 2008, 03:24 UTC
Message-ID: <200812182124.15568.bss@iguanasuicide.net>
In-Reply-To: <alpine.DEB.1.00.0812190353520.14632@racer>

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/                      \_/