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

From: Christian Couder <chriscool@tuxfamily.org>
Date: Mar 4, 2010, 06:55 UTC
Message-ID: <201003040755.23485.chriscool@tuxfamily.org>
In-Reply-To: <7vhbowoj8y.fsf@alter.siamese.dyndns.org>

On Thursday 04 March 2010 04:31:25 Junio C Hamano wrote:

Show 15 quoted lines

> 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).

Show 10 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, 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.