Re: Rebase, please help

From: Junio C Hamano <junkio@cox.net>
Date: Apr 12, 2007, 20:37 UTC
Message-ID: <7v7ishjpm9.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <200704111110.18461.andyparkins@gmail.com>

Andy Parkins <andyparkins@gmail.com> writes:

Show 49 quoted lines

> This is interesting and brings to mind a difficult I've had.
> I had problems with rebase when rebasing chains with a file
> that was self-similar.  Indulge me for a while with this
> example (forgive the C++, but that's where I had this
> problem):
>
> class A : public C
> {
>    // ...
>
>    int someVirtualOverride(n) { return ArrayA[n]; }
>
>    // ...
> }
>
> class B : public C
> {
>    // ...
>
>    int someVirtualOverride(n) { return ArrayB[n]; }
>
>    // ...
> }
>
> One patch changed "ArrayX[n]" to "Array.at(n)" and another inserted more 
> similar classes around these two.
>
> When I was rebasing, some strange things happened (without any conflict 
> warnings):
>
> class D : public C
> {
>    int someVirtualOverride(n) { return ArrayA.at(n); }
> }
>
> class A : public C
> {
>    int someVirtualOverride(n) { return ArrayB.at(n); }
> }
>
> class B : public C
> {
>    int someVirtualOverride(n) { return ArrayB[n]; }
> }
>
> Notice that the arrays don't match up with the classes.  By
> some crazy coincidence and the strong similarity between
> localities within the file, the patch successfully applied in
> the wrong place.

A patch that can ambiguously apply to multiple places is indeed a problem, and in such situations merge based rebase is probably safer as it can take advantage of the whole file as the context.

But it brings up another interesting point. The ambiguous patch is a problem even more so outside the context of rebasing, for another reason. When rebasing, you are dealing with your own changes and you know what and how you want each of them to change the tree state, as opposed to applying somebody else's patch outside the context of rebasing.

When we only have the patch text (i.e. applymbox), there is no "merge to use the whole file as the context" fallback. I wonder if this is a common enough problem that we would want to make it safer somehow...

[jc: Since I happen to know somebody who applies more patches in
     one week than anybody else would ever apply in the lifetime
     ;-), I am CC'ing that person]

I can see two possible improvements.

 - On the diff generation side, we could notice that the hunk
   we are going to output can be applied to more than one
   location, and automatically widen the context for it.

   This is only a half-solution, as many patches do not even
   come from git.

 - Inside git-apply, apply_one_fragment(), ask find_offset() if
   the hunk can match more than one location, and exit with an
   error status (still writing out the patch results if it
   otherwise applies cleanly) so that the user can manually
   inspect and confirm.