bug#65049: Minor update to the repro steps

[Dmitry Gutov]

On 26/08/2023 11:50, Eli Zaretskii wrote:

> > > That code fragment is very old, so just removing it is scary, even if
> > > only in master.
> >
> > Yeah, I noticed: it's from 2007 :-)
>
> No, it's older.  The addition of 'undecided' is from 2007, but the
> vc-coding-system-for-diff part is from the original 1992 code.

Even better.

> I guess we need to force the EOL conversion part to be 'unix?  Like
> this:
>
> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> index 410fe5c..529553e 100644
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -1910,7 +1910,11 @@ vc-diff-internal
>          ;; but the only way to set it for each file included would
>          ;; be to call the back end separately for each file.
>          (coding-system-for-read
> -         (if files (vc-coding-system-for-diff (car files)) 'undecided))
> +          ;; Force EOL conversion to -unix, in case the file itself
> +          ;; has DOS EOLs.
> +          (coding-system-change-eol-conversion
> +          (if files (vc-coding-system-for-diff (car files)) 'undecided)
> +           'unix))
>            (orig-diff-buffer-clone
>             (if revert-buffer-in-progress-p
>                 (clone-buffer

Yes, that fixes that scenario, thanks. Both standalone and as part of 
the full patch at the end of your message.

> > 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.
>
> OK, clear.  So the above should DTRT in both cases.

At least in regards to line endings, yes.

I'm guessing that if we try hard enough with files encoded in an "alien" 
coding system, we will see a similar difference between vc-diff and 
vc-root-diff.

> > > (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.
>
> I don't completely understand what you are saying, probably because I
> don't have a clear picture of all the callers of vc-diff-internal.  So
> I can only explain the fundamental issues here of which I'm aware:
>
>    . When the compared files have DOS EOLs, applying the patch on Posix
>      hosts (and with Git on all hosts) must preserve the ^M characters
>      at ends of lines in the diffs buffer.  This might be a bit ugly
>      when viewing the diffs, but if the same commands are used for
>      patching, this cannot be helped.

There are two questions here: how the diff buffer should look to the 
user, and what patch to feed to Git programmatically. If we decide that 
the formats should be different (e.g. with/without ^M), we could 
probably perform additional newline conversion inside the patch text too.

>    . In all my experience with VCSes managing repositories with mixed
>      EOL formats (such as what we have in Emacs) on Windows, the only
>      sane way of doing that is to force the VCS to leave the original
>      EOLs intact.  With CVS and RCS, this is done by checking out all
>      the text files as "binary"; in Git, there's a config setting to do
>      that.  I have no real experience with SVN and Hg, so I don't know
>      what happens there.  So it's possible we should remove the special
>      handling of Windows in vc-diff-internal, because its only reason
>      is to show "nicer" diffs.

What does it look like on Windows without the "special handling"? Not 
displayed as a bunch of ^M, right?

>    . The line you suggest to remove should IMO stay, because your
>      suggestion is based on what you see with plain-ASCII files.  If
>      the files have some non-trivial text encoding, failing to use the
>      right encoding for the diffs will produce mojibake.  The EOL
>      conversion produced by vc-coding-system-for-diff is indeed
>      problematic, see above; but the text-conversion part is not, and
>      should stay.
>
> Therefore, I propose the patch below, which incorporates the above
> change, for the emacs-29 branch.  I think it is safe to use the 'unix
> EOL conversion on all systems, in the vc-git.el part of the changeset,
> but if you feel uneasy about that on the release branch, we could make
> it Windows-specific on emacs-29 and remove the condition on master.

LGTM for emacs-29, thank you. In case anybody reports a problem, we can 
add that OS limitation later.

Regarding your paragraph above about mojibake, though. That makes a lot 
of sense, but I feel I have to stress: this mechanism doesn't work for 
vc-root-diff (C-x v D).

Does that mean the coding system mismatch sufferers just silently use 
vc-diff and never report their problems with vc-root-diff? The latter 
command was added in 2009. No contest with 1992, but still.