Re: [BUG] git-am silently applying patches incorrectly
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 4, 2011, 22:34 UTC
- Message-ID
- <7vpqq65xcx.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <4D7165A3.5080308@colin.guthr.ie>
Colin Guthrie <gmane@colin.guthr.ie> writes:
Show 9 quoted lines
> 'Twas brillig, and Junio C Hamano at 04/03/11 21:33 did gyre and gimble: >> In short, Linus and I both know what you are talking about, and we may >> revisit that issue later, but the thing is that it would not be very >> pleasant, and not something that can be done in one sitting during a >> single discussion thread on the list. > > As a simple option to avoid that, how about just printing out (by > default) the line offsets if hunks don't apply 100% cleanly? This would > at least alert you to the fact that some fixups were needed.
Yeah, that is what GNU patch does, and would be a small improvement that might be in a right direction.
While we are at it, here is an alternate patch that does not lose the ability to cope with the case where the target version has functions moved around without introducing new ambiguous patch application sites. Instead of keeping the "last position was here -- we won't look beyond that", we mark the lines that were brought into the target by the patch application so far, and reject preimage matches against these lines.
A full solution for detecting a potential ambiguity and warning it would be based on this version instead. It would involve letting find_pos() not stop at the first hit near the intended target, and marking the hunk that can apply at more than one location.
A yet even more reliable alternative solution _might_ be to first scan the original without applying any hunks just to find the possible sites to be patched, warn ambiguities and decide & commit to these patch application sites, and then apply the hunks. If we did so, we wouldn't need this patch nor the previous one.
But that would be a larger change, and would require a good test vector, perhaps a large quilt series, to make sure it does not introduce regression.
builtin/apply.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c index 14951da..04f56f8 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -204,6 +204,7 @@ struct line { unsigned hash : 24; unsigned flag : 8; #define LINE_COMMON 1 +#define LINE_PATCHED 2 }; /* @@ -2085,7 +2086,8 @@ static int match_fragment(struct image *img, /* Quick hash check */ for (i = 0; i < preimage_limit; i++) - if (preimage->line[i].hash != img->line[try_lno + i].hash) + if ((img->line[try_lno + i].flag & LINE_PATCHED) || + (preimage->line[i].hash != img->line[try_lno + i].hash)) return 0; if (preimage_limit == preimage->nr) { @@ -2428,6 +2430,9 @@ static void update_image(struct image *img, memcpy(img->line + applied_pos, postimage->line, postimage->nr * sizeof(*img->line)); + for (i = 0; i < postimage->nr; i++) + img->line[applied_pos + i].flag |= LINE_PATCHED; + img->nr = nr; }