Christian Couder <chriscool@xxxxxxxxxxxxx> writes:
>> +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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html