[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#65049: Minor update to the repro steps
From: |
Dmitry Gutov |
Subject: |
bug#65049: Minor update to the repro steps |
Date: |
Sun, 27 Aug 2023 04:14:11 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 |
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.
- bug#65049: Minor update to the repro steps, (continued)
- bug#65049: Minor update to the repro steps, Dmitry Gutov, 2023/08/23
- bug#65049: Minor update to the repro steps, Eli Zaretskii, 2023/08/24
- bug#65049: Minor update to the repro steps, Dmitry Gutov, 2023/08/24
- bug#65049: Minor update to the repro steps, Dmitry Gutov, 2023/08/24
- bug#65049: Minor update to the repro steps, Dmitry Gutov, 2023/08/24
- bug#65049: Minor update to the repro steps, Eli Zaretskii, 2023/08/25
- bug#65049: Minor update to the repro steps, Dmitry Gutov, 2023/08/25
- bug#65049: Minor update to the repro steps, Eli Zaretskii, 2023/08/26
- bug#65049: Minor update to the repro steps,
Dmitry Gutov <=
- bug#65049: Minor update to the repro steps, Eli Zaretskii, 2023/08/27
- bug#65049: Minor update to the repro steps, Dmitry Gutov, 2023/08/27
- bug#65049: Minor update to the repro steps, Eli Zaretskii, 2023/08/28
- bug#65049: Minor update to the repro steps, Dmitry Gutov, 2023/08/28
- bug#65049: Minor update to the repro steps, Eli Zaretskii, 2023/08/28
- bug#65049: Minor update to the repro steps, Dmitry Gutov, 2023/08/28
- bug#65049: Minor update to the repro steps, Eli Zaretskii, 2023/08/28
- bug#65049: Minor update to the repro steps, Dmitry Gutov, 2023/08/28
- bug#65049: Minor update to the repro steps, Eli Zaretskii, 2023/08/28
- bug#65049: Minor update to the repro steps, Richard Stallman, 2023/08/30