bug#65049: Minor update to the repro steps

[Dmitry Gutov]

On 23/08/2023 19:21, Eli Zaretskii wrote:
> > From: Maxim Kim <habamax@gmail.com>
> > Cc: juri@linkov.net,  65049@debbugs.gnu.org
> > Date: Wed, 23 Aug 2023 14:28:10 +1000
> >
> > Eli Zaretskii <eliz@gnu.org> writes:
> >
> > So, it works for the repo with files having windows CRLF.
> > And doesn't work if the file has EOL type: LF.
>
> Thanks, this finally allowed me to nail the sucker.  The fix is a bit
> more complex than I thought, because this bug has two parts: one of
> them in "C-x v D", the other in vc-git-checkin.  The patch is below;
> please give it a try with all your real-life use cases.
>
> Dmitry, do you see any problems with installing this on the release
> branch?  Or do you prefer to keep it on master for a while, and then
> backport if no one complains?

It doesn't look "obviously correct" to me, and I'm no pro in the coding 
system department.

If the patch shouldn't affect non-Windows systems (which seems to be 
clear for the second hunk, but not for the first one), I'd say we test 
(and ask to test) several orthogonal scenarios: when the system is in 
the standard language environment, and when it's in the "unixy" one. And 
the same regarding the repository.

If everything's okay, maybe it's good for emacs-29. It will have a few 
months to stabilize, right? Just as long as the patch goes into master 
too (that's where the bulk of the stabilization will happen, since we 
have generally switched to that branch now for daily use).

> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index 7ae763d..218696c 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -1051,7 +1051,15 @@ vc-git-checkin
>                   (user-error "Index not empty"))
>                 (setq pos (point))))))
>         (unless (string-empty-p vc-git-patch-string)
> -        (let ((patch-file (make-nearby-temp-file "git-patch")))
> +        (let ((patch-file (make-nearby-temp-file "git-patch"))
> +              ;; Temporarily countermand the let-binding at the
> +              ;; beginning of this function.
> +              (coding-system-for-write
> +               (coding-system-change-eol-conversion
> +                ;; On DOS/Windows, it is important for the patch file
> +                ;; to have the Unix EOL format, because Git expects
> +                ;; that, even on Windows.
> +                (or pcsw vc-git-commits-coding-system) 'unix)))

Any chance this change could negatively affect non-Windows systems? Is 
it a given that the 'unix' line endings are always used there?

>             (with-temp-file patch-file
>               (insert vc-git-patch-string))
>             (unwind-protect
> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> index 410fe5c..4e008de 100644
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -1917,8 +1917,12 @@ vc-diff-internal
>                  (generate-new-buffer-name " *vc-diff-clone*") nil))))
>       ;; On MS-Windows and MS-DOS, Diff is likely to produce DOS-style
>       ;; EOLs, which will look ugly if (car files) happens to have Unix
> -    ;; EOLs.
> -    (if (memq system-type '(windows-nt ms-dos))
> +    ;; EOLs.  But for Git, we must force Unix EOLs in the diffs, since
> +    ;; Git always produces Unix EOLs in the parts that didn't come
> +    ;; from the file, and wants to see any CR characters when applying
> +    ;; patches.
> +    (if (and (memq system-type '(windows-nt ms-dos))
> +             (not (eq (vc-deduce-backend) 'Git)))
>         (setq coding-system-for-read
>               (coding-system-change-eol-conversion coding-system-for-read
>                                                    'dos)))

:thumbsup:

(All the coding system juggling in these files is already too scary for 
my taste, but I don't have any better suggestions.)