Andy Parkins <andyparkins@xxxxxxxxx> 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.
-
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