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

[Christian Couder <chriscool@tuxfamily.org>]

On Thursday 04 March 2010 04:31:25 Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> > I tried to use the checkout_fast_forward() function from builtin/merge.c
> > but unfortunately it doesn't work. It gives an error like that in the
> > tests :
> >
> > error: Your local changes to 'file1' would be overwritten by merge. 
> > Aborting. Please, commit your changes or stash them before you can merge.
> >
> > and I don't really understand why. (Though I didn't spend a lot of time
> > on this.)
> 
> Shouldn't it be something like this?  The patch is still unnecessarily
> large because I wanted to share options between revert and cherry-pick
> while giving --ff only to cherry-pick, but I don't understand why it needs
> a nine patch series worth of code churn.

Please have another look at the patch series and realize that only the first 4 
patches are (trivial) refactoring (that you call "code churn"), so I think 
it's not fair at all to say that it's "a nine patch series worth of code 
churn".

And it used to be accepted to do some refactoring, and IMHO some reasonable 
amount of refactoring is good, as long as it is well done of course. So there 
is no problem if you criticize the amount in term of line of codes or if you 
criticize the quality of refactoring. But I don't think the patch count is 
right metric (especially when applied to the whole series).

> +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, while my refactoring patches 
only move existing code into new functions. So I think the net result is that 
your patch in fact increases the complexity of the code base and duplicates 
existing functionality, while my patches reuse and increase readability of 
existing code.

Now it's true that your patch adds a very small amount of new code and maybe I 
am missing many things and your patch is much better for many reasons that I 
can't see. If that is the case I am sorry to be so stupid.

I also agree that it might not be the right time for refactoring, and that 
it's more work for reviewers, especially yourself, and you (and other 
reviewers) probably just need less work and not more, but in the long run I 
think that reusing existing code is better as that means less maintenance 
work.

Best regards,
Christian.