* bug#65049: 29.1; vc-do-command fails in windows emacs 29.1 @ 2023-08-04 7:50 Maxim Kim 2023-08-04 8:02 ` bug#65049: Minor update to the repro steps Maxim Kim 0 siblings, 1 reply; 49+ messages in thread From: Maxim Kim @ 2023-08-04 7:50 UTC (permalink / raw) To: 65049 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)) ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-04 7:50 bug#65049: 29.1; vc-do-command fails in windows emacs 29.1 Maxim Kim @ 2023-08-04 8:02 ` Maxim Kim 2023-08-04 11:05 ` Eli Zaretskii 0 siblings, 1 reply; 49+ messages in thread From: Maxim Kim @ 2023-08-04 8:02 UTC (permalink / raw) To: 65049 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-04 8:02 ` bug#65049: Minor update to the repro steps Maxim Kim @ 2023-08-04 11:05 ` Eli Zaretskii 2023-08-04 11:24 ` Maxim Kim ` (2 more replies) 0 siblings, 3 replies; 49+ messages in thread From: Eli Zaretskii @ 2023-08-04 11:05 UTC (permalink / raw) To: Maxim Kim; +Cc: 65049 > 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). ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-04 11:05 ` Eli Zaretskii @ 2023-08-04 11:24 ` Maxim Kim 2023-08-04 17:56 ` Juri Linkov 2023-08-07 1:09 ` Maxim Kim 2 siblings, 0 replies; 49+ messages in thread From: Maxim Kim @ 2023-08-04 11:24 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 65049 [-- 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 --] ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-04 11:05 ` Eli Zaretskii 2023-08-04 11:24 ` Maxim Kim @ 2023-08-04 17:56 ` Juri Linkov 2023-08-06 23:04 ` Maxim Kim 2023-08-07 1:09 ` Maxim Kim 2 siblings, 1 reply; 49+ messages in thread From: Juri Linkov @ 2023-08-04 17:56 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Maxim Kim, 65049 >> 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-04 17:56 ` Juri Linkov @ 2023-08-06 23:04 ` Maxim Kim 0 siblings, 0 replies; 49+ messages in thread From: Maxim Kim @ 2023-08-06 23:04 UTC (permalink / raw) To: Juri Linkov; +Cc: Eli Zaretskii, 65049 [-- 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 --] ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-04 11:05 ` Eli Zaretskii 2023-08-04 11:24 ` Maxim Kim 2023-08-04 17:56 ` Juri Linkov @ 2023-08-07 1:09 ` Maxim Kim 2023-08-07 16:24 ` Eli Zaretskii 2 siblings, 1 reply; 49+ messages in thread From: Maxim Kim @ 2023-08-07 1:09 UTC (permalink / raw) To: 65049 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-07 1:09 ` Maxim Kim @ 2023-08-07 16:24 ` Eli Zaretskii 2023-08-07 23:17 ` Maxim Kim 2023-08-20 16:49 ` Juri Linkov 0 siblings, 2 replies; 49+ messages in thread From: Eli Zaretskii @ 2023-08-07 16:24 UTC (permalink / raw) To: Maxim Kim; +Cc: 65049 > 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-07 16:24 ` Eli Zaretskii @ 2023-08-07 23:17 ` Maxim Kim 2023-08-20 16:49 ` Juri Linkov 1 sibling, 0 replies; 49+ messages in thread From: Maxim Kim @ 2023-08-07 23:17 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 65049 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-07 16:24 ` Eli Zaretskii 2023-08-07 23:17 ` Maxim Kim @ 2023-08-20 16:49 ` Juri Linkov 2023-08-20 18:25 ` Eli Zaretskii 1 sibling, 1 reply; 49+ messages in thread From: Juri Linkov @ 2023-08-20 16:49 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Maxim Kim, 65049 > 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") ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-20 16:49 ` Juri Linkov @ 2023-08-20 18:25 ` Eli Zaretskii 2023-08-21 6:53 ` Juri Linkov 2023-08-21 23:10 ` Maxim Kim 0 siblings, 2 replies; 49+ messages in thread From: Eli Zaretskii @ 2023-08-20 18:25 UTC (permalink / raw) To: Juri Linkov; +Cc: habamax, 65049 > 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") ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-20 18:25 ` Eli Zaretskii @ 2023-08-21 6:53 ` Juri Linkov 2023-08-21 11:00 ` Eli Zaretskii 2023-08-21 23:10 ` Maxim Kim 1 sibling, 1 reply; 49+ messages in thread From: Juri Linkov @ 2023-08-21 6:53 UTC (permalink / raw) To: Eli Zaretskii; +Cc: habamax, 65049 >> > 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-21 6:53 ` Juri Linkov @ 2023-08-21 11:00 ` Eli Zaretskii 2023-08-21 11:39 ` Maxim Kim 0 siblings, 1 reply; 49+ messages in thread From: Eli Zaretskii @ 2023-08-21 11:00 UTC (permalink / raw) To: Juri Linkov; +Cc: habamax, 65049 > 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-21 11:00 ` Eli Zaretskii @ 2023-08-21 11:39 ` Maxim Kim 2023-08-21 12:18 ` Eli Zaretskii 0 siblings, 1 reply; 49+ messages in thread From: Maxim Kim @ 2023-08-21 11:39 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 65049, Juri Linkov [-- 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 --] ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-21 11:39 ` Maxim Kim @ 2023-08-21 12:18 ` Eli Zaretskii 0 siblings, 0 replies; 49+ messages in thread From: Eli Zaretskii @ 2023-08-21 12:18 UTC (permalink / raw) To: Maxim Kim; +Cc: 65049, juri > 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-20 18:25 ` Eli Zaretskii 2023-08-21 6:53 ` Juri Linkov @ 2023-08-21 23:10 ` Maxim Kim 2023-08-22 12:52 ` Eli Zaretskii 1 sibling, 1 reply; 49+ messages in thread From: Maxim Kim @ 2023-08-21 23:10 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 65049, Juri Linkov 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-21 23:10 ` Maxim Kim @ 2023-08-22 12:52 ` Eli Zaretskii 2023-08-22 13:12 ` Maxim Kim 0 siblings, 1 reply; 49+ messages in thread From: Eli Zaretskii @ 2023-08-22 12:52 UTC (permalink / raw) To: Maxim Kim; +Cc: 65049, juri > 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? ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-22 12:52 ` Eli Zaretskii @ 2023-08-22 13:12 ` Maxim Kim 2023-08-22 13:17 ` Eli Zaretskii 0 siblings, 1 reply; 49+ messages in thread From: Maxim Kim @ 2023-08-22 13:12 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 65049, juri 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-22 13:12 ` Maxim Kim @ 2023-08-22 13:17 ` Eli Zaretskii 2023-08-22 23:43 ` Maxim Kim 2023-08-23 4:28 ` Maxim Kim 0 siblings, 2 replies; 49+ messages in thread From: Eli Zaretskii @ 2023-08-22 13:17 UTC (permalink / raw) To: Maxim Kim; +Cc: 65049, juri > 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.) ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-22 13:17 ` Eli Zaretskii @ 2023-08-22 23:43 ` Maxim Kim 2023-08-23 4:28 ` Maxim Kim 1 sibling, 0 replies; 49+ messages in thread From: Maxim Kim @ 2023-08-22 23:43 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 65049, juri 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. ^ permalink raw reply related [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-22 13:17 ` Eli Zaretskii 2023-08-22 23:43 ` Maxim Kim @ 2023-08-23 4:28 ` Maxim Kim 2023-08-23 16:21 ` Eli Zaretskii 1 sibling, 1 reply; 49+ messages in thread From: Maxim Kim @ 2023-08-23 4:28 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 65049, juri 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-23 4:28 ` Maxim Kim @ 2023-08-23 16:21 ` Eli Zaretskii 2023-08-23 17:42 ` Juri Linkov ` (2 more replies) 0 siblings, 3 replies; 49+ messages in thread From: Eli Zaretskii @ 2023-08-23 16:21 UTC (permalink / raw) To: Maxim Kim, Dmitry Gutov; +Cc: 65049, juri > 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))) ^ permalink raw reply related [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-23 16:21 ` Eli Zaretskii @ 2023-08-23 17:42 ` Juri Linkov 2023-08-23 18:43 ` Eli Zaretskii 2023-08-23 20:13 ` Dmitry Gutov 2023-08-23 23:46 ` Maxim Kim 2 siblings, 1 reply; 49+ messages in thread From: Juri Linkov @ 2023-08-23 17:42 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Dmitry Gutov, Maxim Kim, 65049 > + (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). ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-23 17:42 ` Juri Linkov @ 2023-08-23 18:43 ` Eli Zaretskii 0 siblings, 0 replies; 49+ messages in thread From: Eli Zaretskii @ 2023-08-23 18:43 UTC (permalink / raw) To: Juri Linkov; +Cc: dmitry, habamax, 65049 > 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-23 16:21 ` Eli Zaretskii 2023-08-23 17:42 ` Juri Linkov @ 2023-08-23 20:13 ` Dmitry Gutov 2023-08-24 4:54 ` Eli Zaretskii 2023-08-23 23:46 ` Maxim Kim 2 siblings, 1 reply; 49+ messages in thread From: Dmitry Gutov @ 2023-08-23 20:13 UTC (permalink / raw) To: Eli Zaretskii, Maxim Kim; +Cc: 65049, juri 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.) ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-23 20:13 ` Dmitry Gutov @ 2023-08-24 4:54 ` Eli Zaretskii 2023-08-24 21:06 ` Dmitry Gutov 0 siblings, 1 reply; 49+ messages in thread From: Eli Zaretskii @ 2023-08-24 4:54 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 65049, habamax, juri > 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-24 4:54 ` Eli Zaretskii @ 2023-08-24 21:06 ` Dmitry Gutov 2023-08-24 21:35 ` Dmitry Gutov 0 siblings, 1 reply; 49+ messages in thread From: Dmitry Gutov @ 2023-08-24 21:06 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 65049, habamax, juri 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-24 21:06 ` Dmitry Gutov @ 2023-08-24 21:35 ` Dmitry Gutov 2023-08-24 21:44 ` Dmitry Gutov 0 siblings, 1 reply; 49+ messages in thread From: Dmitry Gutov @ 2023-08-24 21:35 UTC (permalink / raw) To: Eli Zaretskii; +Cc: juri, habamax, 65049 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-24 21:35 ` Dmitry Gutov @ 2023-08-24 21:44 ` Dmitry Gutov 2023-08-25 6:18 ` Eli Zaretskii 0 siblings, 1 reply; 49+ messages in thread From: Dmitry Gutov @ 2023-08-24 21:44 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 65049, habamax, juri 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'. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-24 21:44 ` Dmitry Gutov @ 2023-08-25 6:18 ` Eli Zaretskii 2023-08-26 0:45 ` Dmitry Gutov 0 siblings, 1 reply; 49+ messages in thread From: Eli Zaretskii @ 2023-08-25 6:18 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 65049, habamax, juri > 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.) ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-25 6:18 ` Eli Zaretskii @ 2023-08-26 0:45 ` Dmitry Gutov 2023-08-26 8:50 ` Eli Zaretskii 0 siblings, 1 reply; 49+ messages in thread From: Dmitry Gutov @ 2023-08-26 0:45 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 65049, habamax, juri 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? ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-26 0:45 ` Dmitry Gutov @ 2023-08-26 8:50 ` Eli Zaretskii 2023-08-27 1:14 ` Dmitry Gutov 0 siblings, 1 reply; 49+ messages in thread From: Eli Zaretskii @ 2023-08-26 8:50 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 65049, habamax, juri > 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))) ^ permalink raw reply related [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-26 8:50 ` Eli Zaretskii @ 2023-08-27 1:14 ` Dmitry Gutov 2023-08-27 5:36 ` Eli Zaretskii 0 siblings, 1 reply; 49+ messages in thread From: Dmitry Gutov @ 2023-08-27 1:14 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 65049, habamax, juri 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-27 1:14 ` Dmitry Gutov @ 2023-08-27 5:36 ` Eli Zaretskii 2023-08-27 22:32 ` Dmitry Gutov 0 siblings, 1 reply; 49+ messages in thread From: Eli Zaretskii @ 2023-08-27 5:36 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 65049, habamax, juri > 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-27 5:36 ` Eli Zaretskii @ 2023-08-27 22:32 ` Dmitry Gutov 2023-08-28 12:12 ` Eli Zaretskii 0 siblings, 1 reply; 49+ messages in thread From: Dmitry Gutov @ 2023-08-27 22:32 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 65049, habamax, juri 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-27 22:32 ` Dmitry Gutov @ 2023-08-28 12:12 ` Eli Zaretskii 2023-08-28 13:45 ` Dmitry Gutov 0 siblings, 1 reply; 49+ messages in thread From: Eli Zaretskii @ 2023-08-28 12:12 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 65049, habamax, juri > 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-28 12:12 ` Eli Zaretskii @ 2023-08-28 13:45 ` Dmitry Gutov 2023-08-28 16:12 ` Eli Zaretskii 0 siblings, 1 reply; 49+ messages in thread From: Dmitry Gutov @ 2023-08-28 13:45 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 65049, habamax, juri 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-28 13:45 ` Dmitry Gutov @ 2023-08-28 16:12 ` Eli Zaretskii 2023-08-28 16:51 ` Dmitry Gutov 0 siblings, 1 reply; 49+ messages in thread From: Eli Zaretskii @ 2023-08-28 16:12 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 65049, habamax, juri > 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-28 16:12 ` Eli Zaretskii @ 2023-08-28 16:51 ` Dmitry Gutov 2023-08-28 16:57 ` Eli Zaretskii 0 siblings, 1 reply; 49+ messages in thread From: Dmitry Gutov @ 2023-08-28 16:51 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 65049, habamax, juri 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-28 16:51 ` Dmitry Gutov @ 2023-08-28 16:57 ` Eli Zaretskii 2023-08-28 17:39 ` Dmitry Gutov 0 siblings, 1 reply; 49+ messages in thread From: Eli Zaretskii @ 2023-08-28 16:57 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 65049, habamax, juri > 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-28 16:57 ` Eli Zaretskii @ 2023-08-28 17:39 ` Dmitry Gutov 2023-08-28 18:26 ` Eli Zaretskii 0 siblings, 1 reply; 49+ messages in thread From: Dmitry Gutov @ 2023-08-28 17:39 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 65049, habamax, juri 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-28 17:39 ` Dmitry Gutov @ 2023-08-28 18:26 ` Eli Zaretskii 2023-08-31 2:07 ` Richard Stallman 0 siblings, 1 reply; 49+ messages in thread From: Eli Zaretskii @ 2023-08-28 18:26 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 65049, habamax, juri > 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-28 18:26 ` Eli Zaretskii @ 2023-08-31 2:07 ` Richard Stallman 2023-08-31 2:14 ` Dmitry Gutov 0 siblings, 1 reply; 49+ messages in thread From: Richard Stallman @ 2023-08-31 2:07 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 65049 [[[ 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) ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-31 2:07 ` Richard Stallman @ 2023-08-31 2:14 ` Dmitry Gutov 2023-08-31 6:00 ` Eli Zaretskii 0 siblings, 1 reply; 49+ messages in thread From: Dmitry Gutov @ 2023-08-31 2:14 UTC (permalink / raw) To: rms, Eli Zaretskii; +Cc: 65049 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-31 2:14 ` Dmitry Gutov @ 2023-08-31 6:00 ` Eli Zaretskii 0 siblings, 0 replies; 49+ messages in thread From: Eli Zaretskii @ 2023-08-31 6:00 UTC (permalink / raw) To: Dmitry Gutov; +Cc: rms, 65049 > 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: Minor update to the repro steps 2023-08-23 16:21 ` Eli Zaretskii 2023-08-23 17:42 ` Juri Linkov 2023-08-23 20:13 ` Dmitry Gutov @ 2023-08-23 23:46 ` Maxim Kim 2024-10-02 17:45 ` bug#65049: 29.1; vc-do-command fails in windows emacs 29.1 Sebastián Monía 2 siblings, 1 reply; 49+ messages in thread From: Maxim Kim @ 2023-08-23 23:46 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Dmitry Gutov, 65049, juri 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! ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: 29.1; vc-do-command fails in windows emacs 29.1 2023-08-23 23:46 ` Maxim Kim @ 2024-10-02 17:45 ` Sebastián Monía 2024-10-02 18:51 ` Eli Zaretskii 0 siblings, 1 reply; 49+ messages in thread From: Sebastián Monía @ 2024-10-02 17:45 UTC (permalink / raw) To: Maxim Kim; +Cc: Dmitry Gutov, Eli Zaretskii, juri, 65049 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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: 29.1; vc-do-command fails in windows emacs 29.1 2024-10-02 17:45 ` bug#65049: 29.1; vc-do-command fails in windows emacs 29.1 Sebastián Monía @ 2024-10-02 18:51 ` Eli Zaretskii 2024-10-02 21:32 ` Dmitry Gutov 0 siblings, 1 reply; 49+ messages in thread From: Eli Zaretskii @ 2024-10-02 18:51 UTC (permalink / raw) To: Sebastián Monía, dmitry; +Cc: juri, habamax, 65049 > 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? ^ permalink raw reply [flat|nested] 49+ messages in thread
* bug#65049: 29.1; vc-do-command fails in windows emacs 29.1 2024-10-02 18:51 ` Eli Zaretskii @ 2024-10-02 21:32 ` Dmitry Gutov 0 siblings, 0 replies; 49+ messages in thread From: Dmitry Gutov @ 2024-10-02 21:32 UTC (permalink / raw) To: Eli Zaretskii, Sebastián Monía; +Cc: 65049-done, habamax, juri 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
end of thread, other threads:[~2024-10-02 21:32 UTC | newest] Thread overview: 49+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-08-04 7:50 bug#65049: 29.1; vc-do-command fails in windows emacs 29.1 Maxim Kim 2023-08-04 8:02 ` bug#65049: Minor update to the repro steps Maxim Kim 2023-08-04 11:05 ` Eli Zaretskii 2023-08-04 11:24 ` Maxim Kim 2023-08-04 17:56 ` Juri Linkov 2023-08-06 23:04 ` Maxim Kim 2023-08-07 1:09 ` Maxim Kim 2023-08-07 16:24 ` Eli Zaretskii 2023-08-07 23:17 ` Maxim Kim 2023-08-20 16:49 ` Juri Linkov 2023-08-20 18:25 ` Eli Zaretskii 2023-08-21 6:53 ` Juri Linkov 2023-08-21 11:00 ` Eli Zaretskii 2023-08-21 11:39 ` Maxim Kim 2023-08-21 12:18 ` Eli Zaretskii 2023-08-21 23:10 ` Maxim Kim 2023-08-22 12:52 ` Eli Zaretskii 2023-08-22 13:12 ` Maxim Kim 2023-08-22 13:17 ` Eli Zaretskii 2023-08-22 23:43 ` Maxim Kim 2023-08-23 4:28 ` Maxim Kim 2023-08-23 16:21 ` Eli Zaretskii 2023-08-23 17:42 ` Juri Linkov 2023-08-23 18:43 ` Eli Zaretskii 2023-08-23 20:13 ` Dmitry Gutov 2023-08-24 4:54 ` Eli Zaretskii 2023-08-24 21:06 ` Dmitry Gutov 2023-08-24 21:35 ` Dmitry Gutov 2023-08-24 21:44 ` Dmitry Gutov 2023-08-25 6:18 ` Eli Zaretskii 2023-08-26 0:45 ` Dmitry Gutov 2023-08-26 8:50 ` Eli Zaretskii 2023-08-27 1:14 ` Dmitry Gutov 2023-08-27 5:36 ` Eli Zaretskii 2023-08-27 22:32 ` Dmitry Gutov 2023-08-28 12:12 ` Eli Zaretskii 2023-08-28 13:45 ` Dmitry Gutov 2023-08-28 16:12 ` Eli Zaretskii 2023-08-28 16:51 ` Dmitry Gutov 2023-08-28 16:57 ` Eli Zaretskii 2023-08-28 17:39 ` Dmitry Gutov 2023-08-28 18:26 ` Eli Zaretskii 2023-08-31 2:07 ` Richard Stallman 2023-08-31 2:14 ` Dmitry Gutov 2023-08-31 6:00 ` Eli Zaretskii 2023-08-23 23:46 ` Maxim Kim 2024-10-02 17:45 ` bug#65049: 29.1; vc-do-command fails in windows emacs 29.1 Sebastián Monía 2024-10-02 18:51 ` Eli Zaretskii 2024-10-02 21:32 ` Dmitry Gutov
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).