bug#81395: 32.0.50; diff-apply-hunk should be case-sensitive
Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <[email protected]>
| Newsgroups | gmane.emacs.bugs |
|---|---|
| Message-ID | <[email protected]> |
>> AFAIK there are actually 4 different possible behaviors here, depending
>> on which file/buffer to apply the hunk and on the boolean REVERSE
>> argument:
>>
>> A. Always apply the hunk (or its reverse) to the OLD file.
>> B. Always apply the hunk (or its reverse) to the NEW file.
>> C. Apply to OLD if REVERSE is non-nil and to NEW otherwise.
>> D. Apply to NEW if REVERSE is non-nil and to OLD otherwise.
>>
>> The best behavior depends on the circumstances.
>> IIRC by default we use (D) but in the above case (C) would presumably
>> make more sense.
>
> Fair. Especially considering my reproducer. However, I ran into case
> sensitivity issue when the diff was on a single file, without any
> ambiguity.
>
> I can try to produce a cleaner reproducer, but I think it is clear that
> the code does case-insensitive regexp search.
Indeed the matching is case-insensitive. I remember making this
decision consciously. 🙂
I'm OK if you make it case-sensitive, tho.
>>> However, that function has more issues in it than just case sensitivity.
>>> For example, it will run into problems when there is duplicate text
>>> inside buffer, and we are looking to patch the non-first instance. I am
>>> not sure if I need to file a separate bug report for this - the logic in
>>> general seems to be shaky.
>>
>> Please do: the logic has to be fuzzy because we need it to work even
>> when the file/buffer has changed since the diff was generated (or
>> equivalently even if the diff is applied to some other file/buffer), but
>> IIRC it uses the line-number info in the hunk header to try and find the
>> "closest" applicable place.
>
> I will provide a reproducer here, since the discussion is anyway getting
> less focused than the original reproducer. Let me know if you want it in a
> separate bug report instead.
>
> 1. Create file /tmp/test.txt
> This is test2.
>
> bar
>
> This is test2.
>
> bar
>
> This is test.
>
> 2. Create test.diff
> diff -u /tmp/test.txt /tmp/test2.txt
> --- /tmp/test.txt 2026-07-26 11:42:55.175090850 +0200
> +++ /tmp/test.txt 2026-07-26 11:43:11.705090601 +0200
> @@ -6,4 +6,4 @@
>
> bar
>
> -This is test.
> +This is test2.
>
> Diff finished. Sun Jul 26 11:43:23 2026
>
> 3. emacs -Q /tmp/test.diff
> 4. C-c C-a
> 5. Observe "hunk already applied".
Hmm... so single-stepping through `diff-find-source-location` shows that
it finds both the "old" text and the "new" text, so it sees that the
patch can be applied *and* it can be reverse-applied.
Then it uses the funny:
(and maybe-new maybe-old (null reverse) (setq switched t) maybe-new)
to decide that it should say "hunk already applied" (which it does by
setting SWITCHED to t). I can't remember enough what REVERSE means
here, but it looks like a plain bug: either we should test `reverse`
rather than `(null reverse)` or maybe the caller should pass `(not
reverse)` or something.
I guess we could also check the "offset" between the diff's line-number
and the place where we found the corresponding text to see which of OLD
or NEW is closest to what the hunk describes (if it's exactly at the
right place, it's more likely to be what was meant), but if we apply
hunks one by one starting from the beginning, then the line numbers end
up not matching exactly anyway (because of the changes made from previous
hunks), so distrusting approximate matches is not a panacea.
In any case, there's a plain UI error: for `C-u C-c C-a` we silently
undo the hunk, whereas for `C-c C-a` we ask "already applied, undo
instead?", so in the end we can only undo the hunk and never apply it.
🙁
=== Stefan