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

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

On Monday 01 March 2010 09:48:45 Junio C Hamano wrote:

Show 19 quoted lines

> Christian Couder <chriscool@tuxfamily.org> writes:
> > On Monday 01 March 2010 04:49:12 Junio C Hamano wrote:
> >> Thanks, but this seems to conflict with too many things in flight (it
> >> applies cleanly on top of 'pu' but not on top of 'next').
> >>
> >> Given that "rebase--interactive", which is the sole in-tree user of
> >> cherry-pick, has its own fast-forwarding logic to skip call to it, it
> >> seems to take too much time out of me to deal with the code churn for
> >> dubious benefit---the series does not seem to solve any real problem.
> >>
> >> After other topics have either graduated to 'master' or dropped out of
> >> 'pu', things might look differently, though.
> >
> > Ok I will wait for something like a week, and then rebase on top of next
> > and resend.
> 
> Actually, waiting, rebasing and resending, without any simplification,
> would be the worst thing you could do.  Perhaps the "waiting" time can be
> used to think how this can be simplified not to be such a big churn.

Ok, I removed some refactoring that was not really needed for this.

> For example, why wouldn't the core of a "cherry-pick --ff" be something
> like the attached patch, which obviously does not have "fast_forward_to"
> yet, but whose implementation should be obvious (the code should already
> be in "merge --ff" fast forward codepath, although I didn't look)?

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

So the next version still refactors builtin/reset.c to create and then use a reset() function.

Thanks, Christian.