Re: [PATCH v2 00/12] add --ff option to cherry-pick

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 4, 2010, 20:48 UTC
Message-ID: <7vocj33jaf.fsf@alter.siamese.dyndns.org>
In-Reply-To: <201003040755.23485.chriscool@tuxfamily.org>

Christian Couder <chriscool@tuxfamily.org> writes:

Show 12 quoted lines

>> +static int fast_forward_to(const unsigned char *to, const unsigned char
>>  *from) +{
>> +	struct ref_lock *ref_lock;
>> +
>> +	read_cache();
>> +	if (checkout_fast_forward(from, to))
>> +		exit(1); /* the callee should have complained already */
>> +	ref_lock = lock_any_ref_for_update("HEAD", from, 0);
>> +	return write_ref_sha1(ref_lock, to, "cherry-pick");
>> +}
>
> Ok, so your patch adds new code to do the job,...

Come on.

The above calls an _existing_ code to go from one rev to another, and another _existing_ code to update the HEAD pointer with a reflog record. I could have inlined this at its sole callsite just so that I won't give you an excuse to quibble like you just did, but I thought that would make the code less readable.

Also I thought that you knew better than that, and I wouldn't need such a "quibble blocker". Please don't disappoint me.

In other words, it was a demonstration that no refactoring was necessary to do what you seem to have wanted to do. We already had API functions that exactly match what we wanted. No need to add "pick.c" file, no need to touch reset.c, no need to move the code around to add a check_parent() that is called from only one place.

It is a separate issue if there are some other functions that are totally irrelevant to the immediate goal of implementing your "cherry-pick --ff" (which is the title of the series) that do something similar to what checkout_fast_forward() does. Maybe they need to be rewritten in terms of checkout_fast_forward(); maybe all of them and checkout_fast_forward() can be split into smaller pieces and get their logically freestanding parts consolidated into a common helper function. But by conflating that into your series, you made a lot more work to review and judge the merit of it; I don't think it was necessarily.

You saw a handful of "*-refactor" patches queued recently. They were all "function X, Y and Z do exactly the same thing; add one common code and call it from these places". That kind of patch does not _fix_ nor improve anything observable from the outside, but at the same time it is easy to review. The only thing that is necessary is to verify the claim "X, Y and Z do the same thing" and validate the refactored code do the same thing for X, Y and Z, and the review can _end there_. I think you've been here long enough to realize the difference.