Re: [PATCH] commit: correct advice about aborting a cherry-pick

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 29, 2013, 15:48 UTC
Message-ID: <7vli4p1i6d.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130729152345.GA10393@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 20 quoted lines

> On Mon, Jul 29, 2013 at 08:18:45AM -0700, Junio C Hamano wrote:
>
>> >  	if (file_exists(git_path("MERGE_HEAD")))
>> >  		whence = FROM_MERGE;
>> > -	else if (file_exists(git_path("CHERRY_PICK_HEAD")))
>> > +	else if (file_exists(git_path("CHERRY_PICK_HEAD"))) {
>> >  		whence = FROM_CHERRY_PICK;
>> > +		if (file_exists(git_path("sequencer")))
>> > +			sequencer_in_use = 1;
>> 
>> Should this be
>> 
>> 	sequencer_in_use = file_exists(...)
>> 
>> so readers do not have to wonder what the variable is initialized
>> to?
>
> Yeah, I think that is a readability improvement. I suppose the use-site
> could also just run "file_exists" itself, but I wanted to keep the
> filesystem-reading "magic name" bits together.

I take that one back. The use-site is sufficiently far from this assignment that is protected with a cascading if that the reader needs to be aware that sequencer_in_use is initialized to zero anyway. The code you posted is just as readable, if not more.

> I had also originally considered adding new states to "whence" to cover
> these cases (i.e., FROM_CHERRY_PICK_MULTI), but there are fallouts all
> over the place for call sites that do not care whether we are in the
> single- or multi-commit mode.

;-)