Re: Rebase, please help

Andy Parkins [off-list ref] writes:

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.