bug#65049: 29.1; vc-do-command fails in windows emacs 29.1

bug#65049: 29.1; vc-do-command fails in windows emacs 29.1

[Maxim Kim]

In a git repository with at least single change:

0. ./runemacs.exe -Q
1. navigate to a dirty git repo
2. C-x v D
3. Add commit message
4. Press C-x v v

As a result I have following error in *Messages*:

    vc-do-command: Failed (status 1): git --no-pager apply --cached
    ../AppData/Local/Temp/git-patchTK9GGr

and in *vc*

    error: patch failed: notes.org:1
    error: notes.org: patch does not apply


If I do C-x v v from within C-x v d RET or directly from changed file --
it works fine, changes are commiteed.




In GNU Emacs 29.1 (build 2, x86_64-w64-mingw32) of 2023-08-03 built on
 AVALON
Windowing system distributor 'Microsoft Corp.', version 10.0.22621
System Description: Microsoft Windows 10 Pro (v10.0.2009.22621.1992)

Configured using:
 'configure --with-modules --without-dbus --with-native-compilation=aot
 --without-compress-install --with-tree-sitter CFLAGS=-O2'

Configured features:
ACL GIF GMP GNUTLS HARFBUZZ JPEG JSON LCMS2 LIBXML2 MODULES NATIVE_COMP
NOTIFY W32NOTIFY PDUMPER PNG RSVG SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XPM ZLIB

(NATIVE_COMP present but libgccjit not available)

Important settings:
  value of $LANG: ENA
  locale-coding-system: cp1252

Major mode: ERC

Minor modes in effect:
  windmove-mode: t
  erc-list-mode: t
  erc-menu-mode: t
  erc-autojoin-mode: t
  erc-ring-mode: t
  erc-pcomplete-mode: t
  erc-track-mode: t
  erc-track-minor-mode: t
  erc-match-mode: t
  erc-netsplit-mode: t
  erc-hl-nicks-mode: t
  erc-button-mode: t
  erc-fill-mode: t
  erc-stamp-mode: t
  erc-irccontrols-mode: t
  erc-noncommands-mode: t
  erc-move-to-prompt-mode: t
  erc-readonly-mode: t
  erc-networks-mode: t
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  shell-dirtrack-mode: t
  server-mode: t
  corfu-popupinfo-mode: t
  corfu-echo-mode: t
  global-corfu-mode: t
  corfu-mode: t
  marginalia-mode: t
  vertico-mode: t
  override-global-mode: t
  pixel-scroll-precision-mode: t
  repeat-mode: t
  savehist-mode: t
  save-place-mode: t
  delete-selection-mode: t
  electric-pair-mode: t
  recentf-mode: t
  winner-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  abbrev-mode: t

Load-path shadows:
c:/Users/maxim.kim/.emacs.d/elpa/transient-20230723.1411/transient hides c:/Program Files/Emacs/emacs-29.1/share/emacs/29.1/lisp/transient
c:/Users/maxim.kim/.emacs.d/elpa/jsonrpc-1.0.17/jsonrpc hides c:/Program Files/Emacs/emacs-29.1/share/emacs/29.1/lisp/jsonrpc
c:/Users/maxim.kim/.emacs.d/elpa/use-package-20230426.2324/use-package hides c:/Program Files/Emacs/emacs-29.1/share/emacs/29.1/lisp/use-package/use-package
c:/Users/maxim.kim/.emacs.d/elpa/use-package-20230426.2324/use-package-lint hides c:/Program Files/Emacs/emacs-29.1/share/emacs/29.1/lisp/use-package/use-package-lint
c:/Users/maxim.kim/.emacs.d/elpa/use-package-20230426.2324/use-package-jump hides c:/Program Files/Emacs/emacs-29.1/share/emacs/29.1/lisp/use-package/use-package-jump
c:/Users/maxim.kim/.emacs.d/elpa/use-package-20230426.2324/use-package-ensure hides c:/Program Files/Emacs/emacs-29.1/share/emacs/29.1/lisp/use-package/use-package-ensure
c:/Users/maxim.kim/.emacs.d/elpa/use-package-20230426.2324/use-package-diminish hides c:/Program Files/Emacs/emacs-29.1/share/emacs/29.1/lisp/use-package/use-package-diminish
c:/Users/maxim.kim/.emacs.d/elpa/use-package-20230426.2324/use-package-delight hides c:/Program Files/Emacs/emacs-29.1/share/emacs/29.1/lisp/use-package/use-package-delight
c:/Users/maxim.kim/.emacs.d/elpa/use-package-20230426.2324/use-package-core hides c:/Program Files/Emacs/emacs-29.1/share/emacs/29.1/lisp/use-package/use-package-core
c:/Users/maxim.kim/.emacs.d/elpa/use-package-20230426.2324/use-package-bind-key hides c:/Program Files/Emacs/emacs-29.1/share/emacs/29.1/lisp/use-package/use-package-bind-key
c:/Users/maxim.kim/.emacs.d/elpa/bind-key-20230203.2004/bind-key hides c:/Program Files/Emacs/emacs-29.1/share/emacs/29.1/lisp/use-package/bind-key
c:/Users/maxim.kim/.emacs.d/elpa/eglot-1.15/eglot hides c:/Program Files/Emacs/emacs-29.1/share/emacs/29.1/lisp/progmodes/eglot
c:/Users/maxim.kim/.emacs.d/elpa/eldoc-1.14.0/eldoc hides c:/Program Files/Emacs/emacs-29.1/share/emacs/29.1/lisp/emacs-lisp/eldoc

Features:
(shadow sort mail-extr emacsbug sh-script smie executable oc-basic
bibtex project embark-org org-element org-persist xdg org-id org-refile
avl-tree generator habamax-org org ob ob-tangle ob-ref ob-lob ob-table
ob-exp org-macro org-src ob-comint org-pcomplete org-list org-footnote
org-faces org-entities ob-emacs-lisp ob-core ob-eval org-cycle org-table
ol org-fold org-fold-core org-keys oc org-loaddefs cal-menu calendar
cal-loaddefs org-version org-compat org-macs misearch multi-isearch
marginalia-autoloads noutline outline markdown-mode-autoloads let-alist
finder find-func loaddefs-gen lisp-mnt radix-tree tar-mode arc-mode
archive-mode mm-archive cus-edit cus-start cus-load gnutls url-cache
url-http url-auth url-gw display-line-numbers finder-inf windmove
compile consult-imenu network-stream nsm epa-file erc-list erc-menu
erc-join erc-ring erc-pcomplete erc-track erc-match erc-netsplit
habamax-erc erc-hl-nicks color erc-button erc-fill erc-stamp erc-goodies
erc iso8601 erc-backend erc-networks erc-common erc-compat erc-loaddefs
embark-consult embark ffap thingatpt c-ts-mode c-ts-common treesit
ibuffer ibuffer-loaddefs vc-hg vc-bzr vc-src vc-sccs vc-svn vc-cvs
vc-rcs log-view vc bug-reference face-remap magit-extras magit-bookmark
magit-submodule magit-blame magit-stash magit-reflog magit-bisect
magit-push magit-pull magit-fetch magit-clone magit-remote magit-commit
magit-sequence magit-notes magit-worktree magit-tag magit-merge
magit-branch magit-reset magit-files magit-refs magit-status magit
magit-repos magit-apply magit-wip magit-log which-func imenu magit-diff
smerge-mode diff git-commit log-edit message sendmail yank-media puny
rfc822 mml mml-sec epa derived epg rfc6068 epg-config gnus-util
mm-decode mm-bodies mm-encode mail-parse rfc2231 rfc2047 rfc2045 mm-util
ietf-drums mail-prsvr mailabbrev mail-utils gmm-utils mailheader
pcvs-util add-log magit-core magit-autorevert autorevert filenotify
magit-margin magit-transient magit-process with-editor comp comp-cstr
warnings icons rx shell pcomplete comint ansi-osc server ansi-color
magit-mode transient magit-git magit-base magit-section format-spec
cursor-sensor crm dash hl-line mule-util orderless consult bookmark
text-property-search pp dired-aux dired dired-loaddefs time-date
wildcharm-light-theme vc-git diff-mode vc-dispatcher habamax time
rainbow-delimiters corfu-popupinfo corfu-echo corfu marginalia vertico
compat use-package-ensure habamax-windows edmacro kmacro cl-extra
help-mode use-package-bind-key bind-key easy-mmode use-package-core
pixel-scroll cua-base repeat savehist saveplace delsel elec-pair recentf
tree-widget wid-edit winner ring elfeed-autoloads
embark-consult-autoloads consult-autoloads embark-autoloads
emms-autoloads magit-autoloads pcase git-commit-autoloads
magit-section-autoloads orderless-autoloads package-lint-autoloads
tempel-autoloads info compat-autoloads package browse-url url url-proxy
url-privacy url-expand url-methods url-history url-cookie
generate-lisp-file url-domsuf url-util mailcap url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs password-cache json subr-x
map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs cl-lib
wildcharm-theme rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel dos-w32
ls-lisp disp-table term/w32-win w32-win w32-vars term/common-win
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer nadvice seq
simple cl-generic indonesian philippine cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite emoji-zwj charscript charprop case-table
epa-hook jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button
loaddefs theme-loaddefs faces cus-face macroexp files window
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget keymap hashtable-print-readable backquote threads
w32notify w32 lcms2 multi-tty make-network-process native-compile emacs)

Memory information:
((conses 16 1970441 140565)
 (symbols 48 38478 4)
 (strings 32 327848 23134)
 (string-bytes 1 10987056)
 (vectors 16 104264)
 (vector-slots 8 2294926 86450)
 (floats 8 466 392)
 (intervals 56 208319 1817)
 (buffers 984 49))

bug#65049: Minor update to the repro steps

[Maxim Kim]

Steps in initial message were a bit incorrect, updated:

0. ./runemacs.exe -Q
1. navigate to a dirty git repo
2. C-x v D
3. Press C-x v v
4. Add commit message
5. Press C-c C-c

Sorry for initial misleading steps.

PS, this worked in 28.2 on the same Windows machine.

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> From: Maxim Kim <habamax@gmail.com>
> Date: Fri, 04 Aug 2023 18:02:56 +1000
> 
> 
> 
> Steps in initial message were a bit incorrect, updated:
> 
> 0. ./runemacs.exe -Q
> 1. navigate to a dirty git repo
> 2. C-x v D
> 3. Press C-x v v
> 4. Add commit message
> 5. Press C-c C-c
> 
> Sorry for initial misleading steps.

I cannot reproduce the problem.  I get the expected behavior: the
changes are committed.  Are you sure your recipe is complete?  Do you
have any Git customizations, either in that repository or in your
system-wide ~/.gitconfig etc.?

If you have no customizations, please tell more about the recipe, in
particular please describe exactly what you see at every step of the
recipe.  For example, after "C-x v D" I see the diffs of the single
file that is modified in the repository -- is that what you see as
well?

FTR: I have the HOME environment variable defined on my Windows system
that points to my home directory -- maybe the problem is somehow
related to what Emacs considers HOME on Windows when there's no such
variable defined? does the directory ../AppData/Local/Temp/ actually
exist on your system (and why does Emacs use a relative file name for
it, anyway)?

If nothing else gives a clue, please step in Edebug through the
relevant code and tell which commands fail, and why (show the values
of the relevant variables).

bug#65049: Minor update to the repro steps

[Maxim Kim]

[-- Attachment #1: Type: text/plain, Size: 2559 bytes --]

On Fri, Aug 4, 2023 at 9:05 PM Eli Zaretskii <eliz@gnu.org> wrote:

>  I cannot reproduce the problem.  I get the expected behavior: the
>  changes are committed.  Are you sure your recipe is complete?  Do you
>  have any Git customizations, either in that repository or in your
>  system-wide ~/.gitconfig etc.?

I don't have any Git customization in that repo (I tried in several
different repos).
System-wide ~/.gitconfig is:

[user]
name = Maxim Kim
email = habamax@gmail.com
[core]
autocrlf = input
quotepath = off
[credential]
helper = libsecret
[commit]
verbose = true
[pull]
rebase = true
[github]
user = habamax

>  If you have no customizations, please tell more about the recipe, in
>  particular please describe exactly what you see at every step of the
>  recipe.  For example, after "C-x v D" I see the diffs of the single
>  file that is modified in the repository -- is that what you see as
>  well?

Yes I can see diffs of a single or multiple files (in the recipe I had a
single
file with a single diff).

Basically
> 0. ./runemacs.exe -Q
default emacs is opened

> 1. navigate to a dirty git repo
> 2. C-x v D
I can see a single file with a single diff

> 3. Press C-x v v

*vc-log* and *vc-log-files* windows are opened

> 4. Add commit message
add Summary only

> 5. Press C-c C-c

Get the error described in initial message.

Once I am back to my windows laptop, will record a anigif and attach to
mail.

>  FTR: I have the HOME environment variable defined on my Windows system
>  that points to my home directory -- maybe the problem is somehow
>  related to what Emacs considers HOME on Windows when there's no such
>  variable defined? does the directory ../AppData/Local/Temp/ actually
>  exist on your system (and why does Emacs use a relative file name for
>  it, anyway)?

I have also defined HOME as C:/Users/maxim.kim (and it worked for 28.2). I
should probably install 28.2 again and check if it still works.

I am not sure if ../AppData/Local/Temp/ exists as I didn't check what is
default-directory at that moment. But C:/Users/maxim.kim/AppData/Local/Temp/
exists and I've seen other temp files there that looked like emacs+git
related,
smth with MSG in name.

I am not sure why Emacs use relative file name here to be honest.

>  If nothing else gives a clue, please step in Edebug through the
>  relevant code and tell which commands fail, and why (show the values
>  of the relevant variables).

Will check how to use Edebug, thank you!

[-- Attachment #2: Type: text/html, Size: 2935 bytes --]

bug#65049: Minor update to the repro steps

[Juri Linkov]

>> 0. ./runemacs.exe -Q
>> 1. navigate to a dirty git repo
>> 2. C-x v D
>> 3. Press C-x v v
>> 4. Add commit message
>> 5. Press C-c C-c
>
> I cannot reproduce the problem.

I don't have Windows, but I noticed these lines in vc-git-checkin:

  ;; On MS-Windows, pass the commit log message through a
  ;; file, to work around the limitation that command-line
  ;; arguments must be in the system codepage, and therefore
  ;; might not support the non-ASCII characters in the log
  ;; message.  Handle also remote files.
  (if (eq system-type 'windows-nt)
      (let ((default-directory (file-name-directory file1)))
        (make-nearby-temp-file "git-msg")))

So probably the same let-default-directory wrapper should be added
around (make-nearby-temp-file "git-patch") in the same function.
And maybe also to (make-nearby-temp-file "git-cached") if needed.

bug#65049: Minor update to the repro steps

[Maxim Kim]

[-- Attachment #1: Type: text/plain, Size: 102 bytes --]

I have installed 28.2 and with the same steps everything works -- the last
C-c C-c checks changes in.

[-- Attachment #2: Type: text/html, Size: 187 bytes --]

bug#65049: Minor update to the repro steps

[Maxim Kim]

It looks like line ending issue, I have tried generated patch in a terminal:

    PS C:\Users\maxim.kim\.emacs.d> git --no-pager apply --cached ..\AppData\Local\Temp\git-patchyYAcN5
    error: patch failed: init.el:1
    error: init.el: patch does not apply

Then added --ignore-space-change (found in
https://www.delftstack.com/howto/git/git-patch-does-not-apply/#troubleshoot-git-patch-error-patch-does-not-apply
and in magit issues https://github.com/magit/magit/issues/1139 plus
linked https://github.com/magit/magit/issues/487#issuecomment-31727377)

    PS C:\Users\maxim.kim\.emacs.d> git --no-pager apply --ignore-space-change --cached ..\AppData\Local\Temp\git-patchyYAcN5
    PS C:\Users\maxim.kim\.emacs.d>

and git apply worked out.

Then I changed line 1058 in vc-git-checkin to:

              (vc-git-command nil 0 patch-file "apply" "--ignore-space-change" "--cached")

And now it looks like it "works", however I can see a change in line
endings for the patched lines (here I have changed first line and
after comitting I can see ^M there):

 ;;; init.el --- emacs init file -*- lexical-binding: t; -*-^M
;;; Commentary:
;; Maxim Kim <habamax@gmail.com>
;;; Code:


Not sure what a proper fix should look like though.

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> From: Maxim Kim <habamax@gmail.com>
> Date: Mon, 07 Aug 2023 11:09:09 +1000
> 
> 
> It looks like line ending issue, I have tried generated patch in a terminal:
> 
>     PS C:\Users\maxim.kim\.emacs.d> git --no-pager apply --cached ..\AppData\Local\Temp\git-patchyYAcN5
>     error: patch failed: init.el:1
>     error: init.el: patch does not apply
> 
> Then added --ignore-space-change (found in
> https://www.delftstack.com/howto/git/git-patch-does-not-apply/#troubleshoot-git-patch-error-patch-does-not-apply
> and in magit issues https://github.com/magit/magit/issues/1139 plus
> linked https://github.com/magit/magit/issues/487#issuecomment-31727377)
> 
>     PS C:\Users\maxim.kim\.emacs.d> git --no-pager apply --ignore-space-change --cached ..\AppData\Local\Temp\git-patchyYAcN5
>     PS C:\Users\maxim.kim\.emacs.d>
> 
> and git apply worked out.

Yes, but where did the file git-patchyYAcN5 come from in the first
place?  It's that file that is the problem, not how we apply the
diffs in that file.

If you invoke "git diff SOME-FILE > diffs", from the shell prompt,
where SOME-FILE is a file that is modified wrt the repository's state,
does the file 'diffs' created by this command have DOS CRLF EOL
format, or does it have Unix Newline-only EOL format?  Try this with a
file that has Unix EOLs in the repository.

If Git produces DOS CRLF EOLs when it generates diffs, then that is
your problem, and it can only be fixed in Git itself: you need to
configure Git to never perform any EOL conversions:

  $ git config --global core.autocrlf false

This is the only sane setting for multiplatform Git repositories,
especially if you are using Emacs as your editor, because Emacs always
preserves the EOL format of a file you edit, even if the EOL format is
not the "native" one on the platform where you edit the file.

(In general, when you install Git on Windows, select the "as-is"
option of EOL conversions, then you will not need to perform the above
"git config" command manually, as this will be set up for you from the
get-go.)

I'm guessing that I don't see the problem you report because I have
disabled EOL conversion in Git years ago.

bug#65049: Minor update to the repro steps

[Maxim Kim]

Eli Zaretskii <eliz@gnu.org> writes:


> Yes, but where did the file git-patchyYAcN5 come from in the first
> place?  It's that file that is the problem, not how we apply the
> diffs in that file.

This file is from the failed attempt of C-x v v

> If you invoke "git diff SOME-FILE > diffs", from the shell prompt,
> where SOME-FILE is a file that is modified wrt the repository's state,
> does the file 'diffs' created by this command have DOS CRLF EOL
> format, or does it have Unix Newline-only EOL format?  Try this with a
> file that has Unix EOLs in the repository.

It has:

    U -- utf-16le-with-signature-dos (alias: utf-16-le-dos)

    UTF-16 (little endian, with signature (BOM)).
    Type: utf-16
    EOL type: CRLF
    This coding system encodes the following charsets:
      unicode


> If Git produces DOS CRLF EOLs when it generates diffs, then that is
> your problem, and it can only be fixed in Git itself: you need to
> configure Git to never perform any EOL conversions:
>
>   $ git config --global core.autocrlf false

I have just did it and it didn't change the outcome -- still same error,
and I have recreated "git diff SOME-FILE > diffs" and it gives the same
output.

This is strange that after the issued command and "git config --list" I
still can see "core.autocrlf=true" even though in the .gitconfig I have
it set to false now (and it was "input" before the change).

"git config --list --show-origin" shows 2 places:

    file:C:/Program Files/Git/etc/gitconfig core.autocrlf=true
    ...
    file:C:/Users/maxim.kim/.gitconfig      core.autocrlf=false

and this is weird git isn't picking up user defined setting
(git version 2.41.0.windows.3).

> This is the only sane setting for multiplatform Git repositories,
> especially if you are using Emacs as your editor, because Emacs always
> preserves the EOL format of a file you edit, even if the EOL format is
> not the "native" one on the platform where you edit the file.
>
> (In general, when you install Git on Windows, select the "as-is"
> option of EOL conversions, then you will not need to perform the above
> "git config" command manually, as this will be set up for you from the
> get-go.)

Let me try to reinstall git and select "as-is" option of EOL conversion

> I'm guessing that I don't see the problem you report because I have
> disabled EOL conversion in Git years ago.

That is probably the case, thanks for your time and help.

Max

bug#65049: Minor update to the repro steps

[Juri Linkov]

> Yes, but where did the file git-patchyYAcN5 come from in the first
> place?  It's that file that is the problem, not how we apply the
> diffs in that file.

It's created in 'vc-git-checkin':

        (let ((patch-file (make-nearby-temp-file "git-patch")))
          (with-temp-file patch-file
            (insert vc-git-patch-string))
          (unwind-protect
              (vc-git-command nil 0 patch-file "apply" "--cached")

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Maxim Kim <habamax@gmail.com>,  65049@debbugs.gnu.org
> Date: Sun, 20 Aug 2023 19:49:53 +0300
> 
> > Yes, but where did the file git-patchyYAcN5 come from in the first
> > place?  It's that file that is the problem, not how we apply the
> > diffs in that file.
> 
> It's created in 'vc-git-checkin':
> 
>         (let ((patch-file (make-nearby-temp-file "git-patch")))
>           (with-temp-file patch-file
>             (insert vc-git-patch-string))
>           (unwind-protect
>               (vc-git-command nil 0 patch-file "apply" "--cached")

Then this should set up EOL conversion correctly for the temporary
file.  Something like (untested):

        (let ((patch-file (make-nearby-temp-file "git-patch")))
          (with-temp-file patch-file
            (insert vc-git-patch-string)
            (set-buffer-file-coding-system 'unix)))
          (unwind-protect
              (vc-git-command nil 0 patch-file "apply" "--cached")

bug#65049: Minor update to the repro steps

[Juri Linkov]

>> > Yes, but where did the file git-patchyYAcN5 come from in the first
>> > place?  It's that file that is the problem, not how we apply the
>> > diffs in that file.
>>
>> It's created in 'vc-git-checkin':
>>
>>         (let ((patch-file (make-nearby-temp-file "git-patch")))
>>           (with-temp-file patch-file
>>             (insert vc-git-patch-string))
>>           (unwind-protect
>>               (vc-git-command nil 0 patch-file "apply" "--cached")
>
> Then this should set up EOL conversion correctly for the temporary
> file.  Something like (untested):
>
>         (let ((patch-file (make-nearby-temp-file "git-patch")))
>           (with-temp-file patch-file
>             (insert vc-git-patch-string)
>             (set-buffer-file-coding-system 'unix)))
>           (unwind-protect
>               (vc-git-command nil 0 patch-file "apply" "--cached")

Sorry, I can't test this on Windows.

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: habamax@gmail.com,  65049@debbugs.gnu.org
> Date: Mon, 21 Aug 2023 09:53:28 +0300
> 
> > Then this should set up EOL conversion correctly for the temporary
> > file.  Something like (untested):
> >
> >         (let ((patch-file (make-nearby-temp-file "git-patch")))
> >           (with-temp-file patch-file
> >             (insert vc-git-patch-string)
> >             (set-buffer-file-coding-system 'unix)))
> >           (unwind-protect
> >               (vc-git-command nil 0 patch-file "apply" "--cached")
> 
> Sorry, I can't test this on Windows.

I didn't ask you to.  I expect that Maxim will be interested, and
hope he'll be able to test.

Note that the other part of this -- to have Git configured not to
modify the EOL format of files, neither when committing nor when
checking out -- still stands, and must be part of the solution.

bug#65049: Minor update to the repro steps

[Maxim Kim]

[-- Attachment #1: Type: text/plain, Size: 485 bytes --]

> I didn't ask you to.  I expect that Maxim will be interested, and
> hope he'll be able to test.

Sure will test it.

> Note that the other part of this -- to have Git configured not to
>modify the EOL format of files, neither when committing nor when
> checking out -- still stands, and must be part of the solution.

Git is configured to not change line endings but without this fix (which I
didn't check yet) it still yields the same error.
Will update you tomorrow if this helps.

[-- Attachment #2: Type: text/html, Size: 683 bytes --]

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> From: Maxim Kim <habamax@gmail.com>
> Date: Mon, 21 Aug 2023 21:39:24 +1000
> Cc: Juri Linkov <juri@linkov.net>, 65049@debbugs.gnu.org
> 
> > I didn't ask you to.  I expect that Maxim will be interested, and
> > hope he'll be able to test.
> 
> Sure will test it.
> 
> > Note that the other part of this -- to have Git configured not to
> >modify the EOL format of files, neither when committing nor when
> > checking out -- still stands, and must be part of the solution.
> 
> Git is configured to not change line endings but without this fix (which I didn't check yet) it still yields
> the same error.
> Will update you tomorrow if this helps.

Yes, please.  Appreciated.

bug#65049: Minor update to the repro steps

[Maxim Kim]

Eli Zaretskii <eliz@gnu.org> writes:

> Then this should set up EOL conversion correctly for the temporary
> file.  Something like (untested):
>
>         (let ((patch-file (make-nearby-temp-file "git-patch")))
>           (with-temp-file patch-file
>             (insert vc-git-patch-string)
>             (set-buffer-file-coding-system 'unix)))
>           (unwind-protect
>               (vc-git-command nil 0 patch-file "apply" "--cached")

This didn't help, I get exact same error.

File I am changing has:

    U -- utf-8-unix (alias: mule-utf-8-unix cp65001-unix)

    UTF-8 (no signature (BOM))
    Type: utf-8 (UTF-8: Emacs internal multibyte form)
    EOL type: LF
    This coding system encodes the following charsets:
      unicode
      

Current ~/.gitconfig:

    [user]
            name = Maxim Kim
            email = habamax@gmail.com
    [core]
            autocrlf = false
            quotepath = off
    [credential]
            helper = manager-core
    [commit]
            verbose = true
    [pull]
            rebase = true
    [github]
            user = habamax


In fact, I get the same error with the fresh repo:

1. mkdir ~/prj/test
2. cd ~/prj/test
3. git init
4. open emacs and create a new text file with "hello" line.
5. C-x v v to register it in vc
6. C-x v v for vc-log, add summary, commit with C-c C-c
7. Add a new line to text file, save
8. C-x v D to get vc-root-diff
9. C-x v v for vc-log and add Summary
10. C-c C-c to commit

And it fails even though autocrlf=false and the file in question has LF.

With or without the patch.

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> From: Maxim Kim <habamax@gmail.com>
> Cc: Juri Linkov <juri@linkov.net>,  65049@debbugs.gnu.org
> Date: Tue, 22 Aug 2023 09:10:44 +1000
> 
> In fact, I get the same error with the fresh repo:
> 
> 1. mkdir ~/prj/test
> 2. cd ~/prj/test
> 3. git init
> 4. open emacs and create a new text file with "hello" line.
> 5. C-x v v to register it in vc
> 6. C-x v v for vc-log, add summary, commit with C-c C-c
> 7. Add a new line to text file, save
> 8. C-x v D to get vc-root-diff
> 9. C-x v v for vc-log and add Summary
> 10. C-c C-c to commit
> 
> And it fails even though autocrlf=false and the file in question has LF.

I'm sorry, but it doesn't fail here, even without installing the patch
with -unix encoding.

I don't know what to say.  Maybe it is something with Git for Windows:
my version is quite old (2.10.0); perhaps something has changed since
then?  Also are you sure you tried with Emacs 29?  I used the latest
emacs-29 branch from Git and stock Emacs 29.1, and they both work as
expected.

If none of the above helps, perhaps describe the steps above in more
detail.  For example, the following details I had to guess:

 . was the new file with Unix EOL or Windows CRLF EOL format?
 . what was buffer-file-coding-system of the new file?
 . what was the exact text of summary in log message you typed each
   time in the recipe?
 . what did the "new line" in the file say, exactly?

Maybe one of these details is significant?

bug#65049: Minor update to the repro steps

[Maxim Kim]

Eli Zaretskii <eliz@gnu.org> writes:

> I'm sorry, but it doesn't fail here, even without installing the patch
> with -unix encoding.
>
> I don't know what to say.  Maybe it is something with Git for Windows:
> my version is quite old (2.10.0); perhaps something has changed since
> then?  Also are you sure you tried with Emacs 29?  I used the latest
> emacs-29 branch from Git and stock Emacs 29.1, and they both work as
> expected.

I use the latest build available from
https://ftp.gnu.org/gnu/emacs/windows/emacs-29/emacs-29.1_2-installer.exe

I don't remember exact git version (will check when I am at my windows
laptop) but it is more recent than 2.10.0. I will downgrade it to see if
this will help.

> If none of the above helps, perhaps describe the steps above in more
> detail.  For example, the following details I had to guess:
>
>  . was the new file with Unix EOL or Windows CRLF EOL format?
   new file had UNIX EOL
>  . what was buffer-file-coding-system of the new file?
   'utf-8-unix
>  . what was the exact text of summary in log message you typed each
>    time in the recipe?
   test
>  . what did the "new line" in the file say, exactly?
   world
   
> Maybe one of these details is significant?

Actually all my files I create in emacs should be with LF endings:

(set-language-environment 'utf-8)
(setq-default buffer-file-coding-system 'utf-8-unix)

I will double-check if the generated patch file has LF endings tomorrow.

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> From: Maxim Kim <habamax@gmail.com>
> Cc: juri@linkov.net,  65049@debbugs.gnu.org
> Date: Tue, 22 Aug 2023 23:12:40 +1000
> 
> Actually all my files I create in emacs should be with LF endings:
> 
> (set-language-environment 'utf-8)
> (setq-default buffer-file-coding-system 'utf-8-unix)

If these are your settings, then please try the recipe in "emacs -Q"
first and see if it works there.  I tried (and failed) to reproduce in
"emacs -Q".  (Using UTF-8 by default on Windows is not a good idea in
general.  But we can revisit this later; it is more important to
understand what causes your problem.)

bug#65049: Minor update to the repro steps

[Maxim Kim]

Eli Zaretskii <eliz@gnu.org> writes:

> If these are your settings, then please try the recipe in "emacs -Q"
> first and see if it works there.

For the repo created earlier when language-environment was utf-8 and
(setq-default buffer-file-coding-system 'utf-8-unix) I get the same
error with "runemacs.exe -Q":


file: hello.txt

--8<---------------cut here---------------start------------->8---
hello
world
--8<---------------cut here---------------end--------------->8---

buffer info: hello.txt

--8<---------------cut here---------------start------------->8---
- -- undecided-unix (alias: unix)

No conversion on encoding, automatic conversion on decoding.
Type: undecided (do automatic conversion)
EOL type: LF
This coding system encodes the following charsets:
  ascii
--8<---------------cut here---------------end--------------->8---

*vc-diff*:

--8<---------------cut here---------------start------------->8---
diff --git a/hello.txt b/hello.txt
index ce01362..94954ab 100644
--- a/hello.txt
+++ b/hello.txt
@@ -1 +1,2 @@
 hello
+world
--8<---------------cut here---------------end--------------->8---

buffer info: *vc-diff*

--8<---------------cut here---------------start------------->8---
1 -- iso-latin-1-dos (alias: iso-8859-1-dos latin-1-dos)

ISO 2022 based 8-bit encoding for Latin-1 (MIME:ISO-8859-1).
Type: charset (charset)
EOL type: CRLF
This coding system encodes the following charsets:
  iso-8859-1

--8<---------------cut here---------------end--------------->8---

git version 2.41.0.windows.3

However, if I do it for a fresh repo again and "runemacs -Q" it works.

All the buffers have EOL type: CRLF (hello.txt, *vc-diff*).

Now if I do "C-x RET f utf-8-unix" to have unix LF and add another
"line" to hello.txt "C-x v D" fails again with same error as before.

bug#65049: Minor update to the repro steps

[Maxim Kim]

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.

The steps:

1. create new git repo (mkdir test, cd test, git init)
2. runemacs -Q
3. navigate to the created test dir and find a new file "hello.txt"
4. C-x RET f utf-8-unix RET
5. add "hello" text line and save
6. C-x v v to register file
7. C-x v v and add summary "test"
8. C-c C-c
8. add "world" text line to "hello.txt" and save
9. C-x v D
10. C-x v v and add summary "test"
11. C-c C-c

error: patch failed: hello.txt:1
error: hello.txt: patch does not apply

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> 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?

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)))
           (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)))

bug#65049: Minor update to the repro steps

[Juri Linkov]

> +    (if (and (memq system-type '(windows-nt ms-dos))
> +             (not (eq (vc-deduce-backend) 'Git)))

Possible optimization: the backend is already in (car vc-fileset).

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Maxim Kim <habamax@gmail.com>,  Dmitry Gutov <dmitry@gutov.dev>,
>   65049@debbugs.gnu.org
> Date: Wed, 23 Aug 2023 20:42:38 +0300
> 
> > +    (if (and (memq system-type '(windows-nt ms-dos))
> > +             (not (eq (vc-deduce-backend) 'Git)))
> 
> Possible optimization: the backend is already in (car vc-fileset).

Thanks, will use that.

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.)

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> Date: Wed, 23 Aug 2023 23:13:18 +0300
> Cc: juri@linkov.net, 65049@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> > 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.

See below.

> If everything's okay, maybe it's good for emacs-29. It will have a few 
> months to stabilize, right?

For some, hopefully small, value of "few", yes.

> 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).

OK.

> > 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?

Yes, that should be the case.  If you will be more happy with making
this a Windows-only change, I can do that.  But before I do, could you
please try the recipe here:

   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65049#68

but with the following change in step 4:

  4. C-x RET f utf-8-dos RET

That is, try the recipe on a Posix host with a file whose EOL format
is CRLF.  If that works without any changes in the current VC code, I
will be happy to make the first hunk Windows-specific.

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

Neither do I.

bug#65049: Minor update to the repro steps

[Dmitry Gutov]

On 24/08/2023 07:54, Eli Zaretskii wrote:

>> If everything's okay, maybe it's good for emacs-29. It will have a few
>> months to stabilize, right?
> 
> For some, hopefully small, value of "few", yes.

Hopefully.

>> 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).
> 
> OK.
> 
>>> 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?
> 
> Yes, that should be the case.  If you will be more happy with making
> this a Windows-only change, I can do that.

I'd be more happy with making the "right" change rather than with 
limiting its scope, given that no release is imminent. Then we can 
decide which branch to put it on.

And given the subject matter, I can't tell whether it's better to limit 
to Windows users or not.

> But before I do, could you
> please try the recipe here:
> 
>     https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65049#68
> 
> but with the following change in step 4:
> 
>    4. C-x RET f utf-8-dos RET
> 
> That is, try the recipe on a Posix host with a file whose EOL format
> is CRLF.  If that works without any changes in the current VC code, I
> will be happy to make the first hunk Windows-specific.

That works with the current emacs-29. Also tried with the patch applied 
-- still works.

bug#65049: Minor update to the repro steps

[Dmitry Gutov]

On 25/08/2023 00:06, Dmitry Gutov wrote:
> 
>> But before I do, could you
>> please try the recipe here:
>>
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65049#68
>>
>> but with the following change in step 4:
>>
>>    4. C-x RET f utf-8-dos RET
>>
>> That is, try the recipe on a Posix host with a file whose EOL format
>> is CRLF.  If that works without any changes in the current VC code, I
>> will be happy to make the first hunk Windows-specific.
> 
> That works with the current emacs-29. Also tried with the patch applied 
> -- still works.

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.

bug#65049: Minor update to the repro steps

[Dmitry Gutov]

On 25/08/2023 00:35, Dmitry Gutov wrote:
> On 25/08/2023 00:06, Dmitry Gutov wrote:
>>
>>> But before I do, could you
>>> please try the recipe here:
>>>
>>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65049#68
>>>
>>> but with the following change in step 4:
>>>
>>>    4. C-x RET f utf-8-dos RET
>>>
>>> That is, try the recipe on a Posix host with a file whose EOL format
>>> is CRLF.  If that works without any changes in the current VC code, I
>>> will be happy to make the first hunk Windows-specific.
>>
>> That works with the current emacs-29. Also tried with the patch 
>> applied -- still works.
> 
> 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'.

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> Date: Fri, 25 Aug 2023 00:44:58 +0300
> From: Dmitry Gutov <dmitry@gutov.dev>
> Cc: juri@linkov.net, habamax@gmail.com, 65049@debbugs.gnu.org
> 
> On 25/08/2023 00:35, Dmitry Gutov wrote:
> > On 25/08/2023 00:06, Dmitry Gutov wrote:
> >>
> >>> But before I do, could you
> >>> please try the recipe here:
> >>>
> >>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65049#68
> >>>
> >>> but with the following change in step 4:
> >>>
> >>>    4. C-x RET f utf-8-dos RET
> >>>
> >>> That is, try the recipe on a Posix host with a file whose EOL format
> >>> is CRLF.  If that works without any changes in the current VC code, I
> >>> will be happy to make the first hunk Windows-specific.
> >>
> >> That works with the current emacs-29. Also tried with the patch 
> >> applied -- still works.
> > 
> > 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.

What if you change that fragment to say

	 (coding-system-for-read
	  (if files (vc-coding-system-for-diff (car files)) 'undecided-unix))

instead?  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.

(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.)

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?

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> Date: Sat, 26 Aug 2023 03:45:41 +0300
> Cc: juri@linkov.net, habamax@gmail.com, 65049@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 25/08/2023 09:18, Eli Zaretskii wrote:
> 
> >> 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 :-)

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.

> > 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.

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

> 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.

> > (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.
  . 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.
  . 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.

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)))
           (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..c314988 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1910,15 +1910,26 @@ 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 the EOL conversion to be -unix, in case the files
+          ;; to be compared have DOS EOLs.  In that case, EOL
+          ;; conversion will produce a patch file that will either
+          ;; fail to apply, or will change the EOL format of some of
+          ;; the lines in the patched file.
+          (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
                (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)))

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.

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> Date: Sun, 27 Aug 2023 04:14:11 +0300
> Cc: juri@linkov.net, habamax@gmail.com, 65049@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 26/08/2023 11:50, Eli Zaretskii wrote:
> 
> 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.

We could.  The 'undecided-unix' value is a good default, but if the
fileset includes files with different incompatible encodings, there's
no single coding-system that could satisfy that, and what Emacs
guesses will probably be okay for the first file, but not necessarily
for the rest.

> >    . 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.

Yes, we could implement some display-time nicety.

> >    . 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?

Yes, the ^M are removed by EOL conversion.

> >    . 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.

Thanks, installed on the emacs-29 branch.

> 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).

Not sure I understand.  Can you show a recipe for "doesn't work"?

> 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.

I think it means most source files are plain-ASCII or UTF-8 at most.

bug#65049: Minor update to the repro steps

[Dmitry Gutov]

On 27/08/2023 08:36, Eli Zaretskii wrote:
>> Date: Sun, 27 Aug 2023 04:14:11 +0300
>> Cc: juri@linkov.net, habamax@gmail.com, 65049@debbugs.gnu.org
>> From: Dmitry Gutov <dmitry@gutov.dev>
>>
>> On 26/08/2023 11:50, Eli Zaretskii wrote:
>>
>> 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.
> 
> We could.  The 'undecided-unix' value is a good default, but if the
> fileset includes files with different incompatible encodings, there's
> no single coding-system that could satisfy that, and what Emacs
> guesses will probably be okay for the first file, but not necessarily
> for the rest.

And it only guesses when passed a file or list of them (e.g. from a 
selection in vc-dir).

>>>     . 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.
> 
> Thanks, installed on the emacs-29 branch.

Thanks for the fix.

>> 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).
> 
> Not sure I understand.  Can you show a recipe for "doesn't work"?

It's the same recipe as what you proposed I test (a file with dos line 
ending on unix). But you don't even have to test that.

Try edebug-instrumenting vc-diff-internal and then calling vc-root-diff 
anywhere (C-x v D). When the execution reaches the line that we have 
been discussing, you'll see that (vc-coding-system-for-diff (car files)) 
evaluates to 'undecided because (car files) is a directory.

So this mechanism is always unused in vc-root-diff.

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> Date: Mon, 28 Aug 2023 01:32:57 +0300
> Cc: juri@linkov.net, habamax@gmail.com, 65049@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> >> 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).
> > 
> > Not sure I understand.  Can you show a recipe for "doesn't work"?
> 
> It's the same recipe as what you proposed I test (a file with dos line 
> ending on unix). But you don't even have to test that.
> 
> Try edebug-instrumenting vc-diff-internal and then calling vc-root-diff 
> anywhere (C-x v D). When the execution reaches the line that we have 
> been discussing, you'll see that (vc-coding-system-for-diff (car files)) 
> evaluates to 'undecided because (car files) is a directory.
> 
> So this mechanism is always unused in vc-root-diff.

OK, but in that case 'undecided' is the best guess we can come up
with.  It basically lets Emacs guess when it actually sees the stuff
in the diffs, while reading it into a buffer.

bug#65049: Minor update to the repro steps

[Dmitry Gutov]

On 28/08/2023 15:12, Eli Zaretskii wrote:
>> Try edebug-instrumenting vc-diff-internal and then calling vc-root-diff
>> anywhere (C-x v D). When the execution reaches the line that we have
>> been discussing, you'll see that (vc-coding-system-for-diff (car files))
>> evaluates to 'undecided because (car files) is a directory.
>>
>> So this mechanism is always unused in vc-root-diff.
> OK, but in that case 'undecided' is the best guess we can come up
> with.  It basically lets Emacs guess when it actually sees the stuff
> in the diffs, while reading it into a buffer.

Yes, and if it's good enough for the (possibly?) most-frequently used 
out of the vc-*-diff commands, then perhaps we don't need the additional 
detection logic?

Since its introduction 30 years ago indeed the situation has changed a 
lot, with UTF-8 and its ubiquity. Removing the extra complication would 
make code a little easier to read, and reduce variability when 
reproducing problems. But there's no hurry, of course.

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> Date: Mon, 28 Aug 2023 16:45:23 +0300
> Cc: juri@linkov.net, habamax@gmail.com, 65049@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 28/08/2023 15:12, Eli Zaretskii wrote:
> >> Try edebug-instrumenting vc-diff-internal and then calling vc-root-diff
> >> anywhere (C-x v D). When the execution reaches the line that we have
> >> been discussing, you'll see that (vc-coding-system-for-diff (car files))
> >> evaluates to 'undecided because (car files) is a directory.
> >>
> >> So this mechanism is always unused in vc-root-diff.
> > OK, but in that case 'undecided' is the best guess we can come up
> > with.  It basically lets Emacs guess when it actually sees the stuff
> > in the diffs, while reading it into a buffer.
> 
> Yes, and if it's good enough for the (possibly?) most-frequently used 
> out of the vc-*-diff commands, then perhaps we don't need the additional 
> detection logic?
> 
> Since its introduction 30 years ago indeed the situation has changed a 
> lot, with UTF-8 and its ubiquity. Removing the extra complication would 
> make code a little easier to read, and reduce variability when 
> reproducing problems. But there's no hurry, of course.

I'm not sure I understand which part do you want to remove.

bug#65049: Minor update to the repro steps

[Dmitry Gutov]

On 28/08/2023 19:12, Eli Zaretskii wrote:
>> Date: Mon, 28 Aug 2023 16:45:23 +0300
>> Cc:juri@linkov.net,habamax@gmail.com,65049@debbugs.gnu.org
>> From: Dmitry Gutov<dmitry@gutov.dev>
>>
>> On 28/08/2023 15:12, Eli Zaretskii wrote:
>>>> Try edebug-instrumenting vc-diff-internal and then calling vc-root-diff
>>>> anywhere (C-x v D). When the execution reaches the line that we have
>>>> been discussing, you'll see that (vc-coding-system-for-diff (car files))
>>>> evaluates to 'undecided because (car files) is a directory.
>>>>
>>>> So this mechanism is always unused in vc-root-diff.
>>> OK, but in that case 'undecided' is the best guess we can come up
>>> with.  It basically lets Emacs guess when it actually sees the stuff
>>> in the diffs, while reading it into a buffer.
>> Yes, and if it's good enough for the (possibly?) most-frequently used
>> out of the vc-*-diff commands, then perhaps we don't need the additional
>> detection logic?
>>
>> Since its introduction 30 years ago indeed the situation has changed a
>> lot, with UTF-8 and its ubiquity. Removing the extra complication would
>> make code a little easier to read, and reduce variability when
>> reproducing problems. But there's no hurry, of course.
> I'm not sure I understand which part do you want to remove.

The part that currently looks like this:

	 (coding-system-for-read
           ;; Force the EOL conversion to be -unix, in case the files
           ;; to be compared have DOS EOLs.  In that case, EOL
           ;; conversion will produce a patch file that will either
           ;; fail to apply, or will change the EOL format of some of
           ;; the lines in the patched file.
           (coding-system-change-eol-conversion
	   (if files (vc-coding-system-for-diff (car files)) 'undecided)
            'unix))

As we've established, the only part that's used in vc-root-diff (and 
only now) is binding coding-system-change-eol-conversion to 
'undecided-unix'. We could leave it there, though it doesn't seem to 
change the behavior in my tests.

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> Date: Mon, 28 Aug 2023 19:51:25 +0300
> Cc: juri@linkov.net, habamax@gmail.com, 65049@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 28/08/2023 19:12, Eli Zaretskii wrote:
> >> Date: Mon, 28 Aug 2023 16:45:23 +0300
> >> Cc:juri@linkov.net,habamax@gmail.com,65049@debbugs.gnu.org
> >> From: Dmitry Gutov<dmitry@gutov.dev>
> >>
> >> On 28/08/2023 15:12, Eli Zaretskii wrote:
> >>>> Try edebug-instrumenting vc-diff-internal and then calling vc-root-diff
> >>>> anywhere (C-x v D). When the execution reaches the line that we have
> >>>> been discussing, you'll see that (vc-coding-system-for-diff (car files))
> >>>> evaluates to 'undecided because (car files) is a directory.
> >>>>
> >>>> So this mechanism is always unused in vc-root-diff.
> >>> OK, but in that case 'undecided' is the best guess we can come up
> >>> with.  It basically lets Emacs guess when it actually sees the stuff
> >>> in the diffs, while reading it into a buffer.
> >> Yes, and if it's good enough for the (possibly?) most-frequently used
> >> out of the vc-*-diff commands, then perhaps we don't need the additional
> >> detection logic?
> >>
> >> Since its introduction 30 years ago indeed the situation has changed a
> >> lot, with UTF-8 and its ubiquity. Removing the extra complication would
> >> make code a little easier to read, and reduce variability when
> >> reproducing problems. But there's no hurry, of course.
> > I'm not sure I understand which part do you want to remove.
> 
> The part that currently looks like this:
> 
> 	 (coding-system-for-read
>            ;; Force the EOL conversion to be -unix, in case the files
>            ;; to be compared have DOS EOLs.  In that case, EOL
>            ;; conversion will produce a patch file that will either
>            ;; fail to apply, or will change the EOL format of some of
>            ;; the lines in the patched file.
>            (coding-system-change-eol-conversion
> 	   (if files (vc-coding-system-for-diff (car files)) 'undecided)
>             'unix))
> 
> As we've established, the only part that's used in vc-root-diff (and 
> only now) is binding coding-system-change-eol-conversion to 
> 'undecided-unix'. We could leave it there, though it doesn't seem to 
> change the behavior in my tests.

Remove that?  What will happen to non-vc-root-diff clients of that?

Or do you mean remove the vc-coding-system-for-diff call, and use
undecided-unix instead?

If the latter, then it is sub-optimal when vc-coding-system-for-diff
does produce non-undecided value for some reason.

bug#65049: Minor update to the repro steps

[Dmitry Gutov]

On 28/08/2023 19:57, Eli Zaretskii wrote:
> Remove that?  What will happen to non-vc-root-diff clients of that?

It is my understanding that all (or almost all?) users of vc-diff and 
other callers of vc-diff-internal use vc-root-diff just as often (and 
don't complain about it).

There could be some exceptions, of course. Like people who use CVS 
exclusively.

> Or do you mean remove the vc-coding-system-for-diff call, and use
> undecided-unix instead?

That's also an option, but I'd have to see a scenario where this binding 
changes the observed behavior.

 > If the latter, then it is sub-optimal when vc-coding-system-for-diff
does produce non-undecided value for some reason.

The question is how often that actually happens, and how useful the 
result is compared to the default behavior.

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> Date: Mon, 28 Aug 2023 20:39:24 +0300
> Cc: juri@linkov.net, habamax@gmail.com, 65049@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 28/08/2023 19:57, Eli Zaretskii wrote:
> > Remove that?  What will happen to non-vc-root-diff clients of that?
> 
> It is my understanding that all (or almost all?) users of vc-diff and 
> other callers of vc-diff-internal use vc-root-diff just as often (and 
> don't complain about it).
> 
> There could be some exceptions, of course. Like people who use CVS 
> exclusively.
> 
> > Or do you mean remove the vc-coding-system-for-diff call, and use
> > undecided-unix instead?
> 
> That's also an option, but I'd have to see a scenario where this binding 
> changes the observed behavior.
> 
>  > If the latter, then it is sub-optimal when vc-coding-system-for-diff
> does produce non-undecided value for some reason.
> 
> The question is how often that actually happens, and how useful the 
> result is compared to the default behavior.

Even if it happens in 1% of cases, it is useful in those cases.  I
think it's a net win.

bug#65049: Minor update to the repro steps

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

What effect would the proposed change have that I as a user of VC mode
notice?  I use it with CVS and with git.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#65049: Minor update to the repro steps

[Dmitry Gutov]

On 31/08/2023 05:07, Richard Stallman wrote:
> What effect would the proposed change have that I as a user of VC mode
> notice?  I use it with CVS and with git.

If you have files in that repository which use a coding system that 
doesn't match your configured language environment, it's possible that 
the output of vc-diff might look inappropriate (decoded incorrectly).

I'm curious to hear of any such issues in practice.

And I mentioned CVS here only because vc-root-diff doesn't work with it, 
so the CVS users in particular wouldn't be able to try it, see the 
aforementioned problem, and report it.

bug#65049: Minor update to the repro steps

[Eli Zaretskii]

> Date: Thu, 31 Aug 2023 05:14:47 +0300
> Cc: 65049@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 31/08/2023 05:07, Richard Stallman wrote:
> > What effect would the proposed change have that I as a user of VC mode
> > notice?  I use it with CVS and with git.
> 
> If you have files in that repository which use a coding system that 
> doesn't match your configured language environment, it's possible that 
> the output of vc-diff might look inappropriate (decoded incorrectly).

Yes, but I think the existing code has the same issue.  IOW, the
situation where a VC fileset includes files with different encodings,
and the diffs of the entire fileset are shown in Emacs, is not well
supported by the Emacs decoding infrastructure, because that assumes
there's a single coding-system suitable to decode the entire text
shown in a buffer, and thus makes decisions about the encoding based
on sampling a small initial portion of the text.

Btw, the CVS case is slightly less problematic here, since in CVS one
usually looks at diffs of separate files, and the whole idea of VC
fileset is quite artificial, since CVS itself doesn't really support
multiple-file changesets, it treats each file separately.

bug#65049: Minor update to the repro steps

[Maxim Kim]

Eli Zaretskii <eliz@gnu.org> writes:

> 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.

I have tried it for several repos I have -- all good!

Thank you, Eli!

bug#65049: 29.1; vc-do-command fails in windows emacs 29.1

[Sebastián Monía]

Hi all,

Was looking at existing bugs that need reproduction work in Windows.

Maxim Kim <habamax@gmail.com> writes:
> Eli Zaretskii <eliz@gnu.org> writes:
>
>> 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.
>
> I have tried it for several repos I have -- all good!
>
> Thank you, Eli!

Based on the comment above, shouldn't this patch be merged?
I read _some_ messages in the other sub-thread, not all of them, but I
understand we should be good to go.


-- 
Sebastián Monía
https://site.sebasmonia.com/

bug#65049: 29.1; vc-do-command fails in windows emacs 29.1

[Eli Zaretskii]

> From: Sebastián Monía <sebastian@sebasmonia.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Dmitry Gutov <dmitry@gutov.dev>,
>   65049@debbugs.gnu.org,  juri@linkov.net
> Date: Wed, 02 Oct 2024 13:45:43 -0400
> 
> 
> Hi all,
> 
> Was looking at existing bugs that need reproduction work in Windows.
> 
> Maxim Kim <habamax@gmail.com> writes:
> > Eli Zaretskii <eliz@gnu.org> writes:
> >
> >> 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.
> >
> > I have tried it for several repos I have -- all good!
> >
> > Thank you, Eli!
> 
> Based on the comment above, shouldn't this patch be merged?

It was already installed, see
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65049#110.

We should probably close the bug.  Dmitry, any objections?

bug#65049: 29.1; vc-do-command fails in windows emacs 29.1

[Dmitry Gutov]

On 02/10/2024 21:51, Eli Zaretskii wrote:
>> From: Sebastián Monía<sebastian@sebasmonia.com>
>> Cc: Eli Zaretskii<eliz@gnu.org>,  Dmitry Gutov<dmitry@gutov.dev>,
>>    65049@debbugs.gnu.org,juri@linkov.net
>> Date: Wed, 02 Oct 2024 13:45:43 -0400
>>
>>
>> Hi all,
>>
>> Was looking at existing bugs that need reproduction work in Windows.
>>
>> Maxim Kim<habamax@gmail.com> writes:
>>> Eli Zaretskii<eliz@gnu.org> writes:
>>>
>>>> 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.
>>> I have tried it for several repos I have -- all good!
>>>
>>> Thank you, Eli!
>> Based on the comment above, shouldn't this patch be merged?
> It was already installed, see
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65049#110.
> 
> We should probably close the bug.  Dmitry, any objections?

Sure. Let me just close it with this message.