On Thursday 04 March 2010 04:31:25 Junio C Hamano wrote:
> Christian Couder <chriscool@xxxxxxxxxxxxx> 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.
--
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