bug#65049: Minor update to the repro steps

[Dmitry Gutov]

On 25/08/2023 09:18, Eli Zaretskii wrote:

> > > But here's a modification of the scenario that fails (again: both with
> > > and without the patch): replace step 9 with
> > >
> > >     9. C-x v =
> > >
> > > The non-root diff looks a little different to begin with: it doesn't
> > > show those ^M chars at the end of lines (whereas the result of
> > > vc-root-diff shows them). That is likely the reason: buffer set up in a
> > > different way.
> >
> > Looks like it's this line:
> >
> >          (coding-system-for-read
> >           (if files (vc-coding-system-for-diff (car files)) 'undecided))
> >
> > near the beginning of vc-diff-internal that creates the difference.
> > Commenting it out makes the scenario work with both 'C-x v =' and 'C-x v D'.
>
> That code fragment is very old, so just removing it is scary, even if
> only in master.

Yeah, I noticed: it's from 2007 :-)

> What if you change that fragment to say
>
>          (coding-system-for-read
>           (if files (vc-coding-system-for-diff (car files)) 'undecided-unix))
>
> instead?

No change at all. The reasons are twofold:

- You changed the value that was seemingly used for the "root" case, 
because in the individual diff's case files must not be nil: it would 
contain the files to be diff'd. That's why that change doesn't affect 
'C-x v ='.

- But it also doesn't affect 'C-x v D'. Because even in that case FILES 
is non-nil ;-(. In that scenario FILES is a list with one item: the 
repository's root directory.

So we can conclude that this code is at least a little buggy. But... (*)

> If that doesn't work, please tell to what value does
> vc-diff-internal set coding-system-for-read in your case there, and I
> will try to figure out what would needs to be done there.

(vc-coding-system-for-diff (car files)) either returns 'undecided when 
FILES contains the directory (vc-root-diff), or 'undecided-dos when 
FILES contains hello.txt as the sole element (because our scenario made 
sure the file has that encoding), that's the vc-diff case.

These are the values coding-system-for-read is set to.

> (In general, I believe that using Git on Posix hosts with files that
> have DOS EOLs could have such problems in other use cases, where diffs
> are generated and then applied as patches.  We just don't know about
> those cases because they are extremely rare in Real Life.)

I'm definitely curious which scenarios made Eric add that line.

(*) ... upon some reflection, though, it seems like our success here is 
kind of relying on vc-root-diff's bug. Remember I mentioned the ^M chars 
appearing at the ends of lines? That is because the encoding of the diff 
buffer (utf-8-unix) doesn't match the encoding of the file (utf-8-dos).

That only happens with the root diff, but not with vc-diff, which 
follows the old design and uses the return value of 
vc-coding-system-for-diff (undecided-dos). As luck would have it, 
though, our patch generation and application works well with the former 
behavior but not the latter.

Still, Eric's old design did not make allowance for root diffs. Not sure 
what to do with that; though I suppose we could post-process the diff 
outputs instead: read the name of the first file in there, then detect 
its encoding on disk, and then re-decode the diff contents if the 
current value of buffer-file-coding-system doesn't match. And *then* we 
would need to fix vc-git-checkin-patch in that scenario (and maybe other 
backends as well).

Or we decide that seeing ^M in diff buffers is a good thing under those 
conditions, and delete the line in question.

WDYT?