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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 4, 2011, 18:37 UTC
Message-ID: <7vei6m7muw.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7vr5am7p30.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 20 quoted lines

> @@ -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;
>  }
>
> 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.

I don't necessarily think mucking with the first location we try to apply using the offset we found by applying the previous hunk is actually a good thing. With so many offset lines and multiple places that a hunk can apply to make the patch application unreliable, that change would be robbing Peter to pay Paul. Depending on the nature of the change between the version the patch is based on and the version the patch is being applied with offsets, such a heuristics will sometimes err on the wrong side.

Looking at it closer, however, I noticed that the false hit (i.e. "two blocks closed, a blank line, return 0 and the end of function") in this particular case only appears because we applied the previous hunk. In the version of the file in 0ce3017b, there is only one such place and there should be no ambiguity in the patch application.

The problem we are seeing is caused only because we look at the result of application of the previous hunks in the patch and incrementally try to apply the remaining hunks. So clearly "git apply" can and should be fixed for this case by teaching find_pos() not to report a match on a line that was touched by application of the previous hunk.