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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Junio C Hamano <gitster@xxxxxxxxx> writes:

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


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]