Package: emacs;
Reported by: Maxim Kim <habamax <at> gmail.com>
Date: Fri, 4 Aug 2023 07:51:01 UTC
Severity: normal
Found in version 29.1
Done: Dmitry Gutov <dmitry <at> gutov.dev>
Bug is archived. No further changes may be made.
Message #107 received at 65049 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dmitry <at> gutov.dev> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 65049 <at> debbugs.gnu.org, habamax <at> gmail.com, juri <at> linkov.net Subject: Re: bug#65049: Minor update to the repro steps Date: Sun, 27 Aug 2023 04:14:11 +0300
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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.