Re: [BUG] git-am silently applying patches incorrectly

From

Junio C Hamano <gitster@pobox.com>

Date

Mar 4, 2011, 17:49 UTC

Message-ID

<7vr5am7p30.fsf@alter.siamese.dyndns.org>

In-Reply-To

<4D70EBC3.3010400@colin.guthr.ie>

Colin Guthrie <gmane@colin.guthr.ie> writes:

> It seems that it mis-applied a patch and did so silently without
> generating any warnings. It is reproducible and has been confirmed on
> different distros.

@@ -1190,9 +1222,6 @@ static int element_probe(pa_alsa_element *e, snd_mixer_t *m) {
 
     }
 
-    if (check_required(e, me) < 0)
-        return -1;
-
     if (e->switch_use == PA_ALSA_SWITCH_SELECT) {
         pa_alsa_option *o;
 
@@ -1224,6 +1253,9 @@ static int element_probe(pa_alsa_element *e, snd_mixer_t *m) {
         }
     }
 
+    if (check_required(e, me) < 0)
+        return -1;
+
     return 0;
 }

Thanks for a report.
 
We find the match for the first hunk (there is only a single callsite) and
correctly remove it, but there are many places that match the preimage of
the second hunk (two blocks closed, blank line and then return 0 from the
function).  We chose to add it to at line 1156, instead of patch's choice
of line 1359, presumably because we thought that is closer to the place
the patch tells us to (i.e. ll.1224-1230).

I haven't looked at the offset logic in git-apply for a long time since
Linus wrote its original version (I don't think the logic has changed very
much since then), but I thought we are taking accumulated offsets into
account when we decide where the patch target should roughly correspond
to.  When we attempt to apply the second hunk, we have already found that
the line the patch says should be at l.1190 is actually at l.1296 (iow,
there are about 100 lines of new material above that the patch didn't
expect), so instead of trying to find the lines that matches the preimage
of the second hunk at around l.1224, we _should_ be trying to find that at
around l.1224+100---perhaps we are not doing that.