bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Manuel Uberti via Bug reports for GNU Emacs, the Swiss army knife of text editors]

I don't know if I am missing something, but I don't think staging specific hunks 
from vc-git-mode or diff-mode is possible at the moment.

Is adding such a capability to one (or both) of these modes worth
considering? I know Magit does it, and I remember something similar in a 
git-gutter package. It's probably something diff-hl does as well by now (I have 
not checked).

I feel like it'd be a nice improvement to VC, what do you think?


Thank you

In GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.18, cairo 
version 1.16.0)
  of 2021-12-07 built on hathaway
Repository revision: 9a1e87ba4486df19259abf3564a0281d8be25e64
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12009000
System Description: Ubuntu 20.04 LTS

Configured using:
  'configure --with-harfbuzz --with-native-compilation'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF
TOOLKIT_SCROLL_BARS X11 XDBE XIM XPM GTK3 ZLIB

Important settings:
   value of $LC_MONETARY: it_IT.UTF-8
   value of $LC_NUMERIC: it_IT.UTF-8
   value of $LC_TIME: it_IT.UTF-8
   value of $LANG: en_US.UTF-8
   value of $XMODIFIERS: @im=ibus
   locale-coding-system: utf-8-unix

Major mode: VC dir

Minor modes in effect:
   vc-parent-buffer: .emacs.d/*vc-dir*
   shell-dirtrack-mode: t
   electric-pair-mode: t
   vc-dir-git-mode: t
   savehist-mode: t
   global-so-long-mode: t
   global-subword-mode: t
   subword-mode: t
   windmove-mode: t
   winner-mode: t
   global-company-mode: t
   company-mode: t
   envrc-global-mode: t
   envrc-mode: t
   fido-vertical-mode: t
   icomplete-vertical-mode: t
   icomplete-mode: t
   fido-mode: t
   delete-selection-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
   window-divider-mode: t
   auto-composition-mode: t
   auto-encryption-mode: t
   auto-compression-mode: t
   buffer-read-only: t
   column-number-mode: t
   line-number-mode: t
   transient-mark-mode: t

Load-path shadows:
/home/manuel/.emacs.d/elpa/transient-0.3.7.0.20211107.234631/transient hides 
/usr/local/share/emacs/29.0.50/lisp/transient

Features:
(shadow sort mail-extr help-fns radix-tree emacsbug sendmail tramp-cmds
mm-archive message yank-media rfc822 mml mml-sec epa gnus-util rmail
rmail-loaddefs mailabbrev gmm-utils mailheader mm-decode mm-bodies
mm-encode mail-utils mule-util gnutls url-cache epg rfc6068 epg-config
hl-line finder-inf misearch multi-isearch pkg-info url-http url-auth
url-gw find-func epl network-stream puny nsm rmc cider tramp-sh
cider-debug cider-browse-ns cider-repl-history pulse derived sh-script
smie executable find-dired cider-mode cider-find cider-inspector
cider-completion cider-profile cider-eval cider-repl cider-resolve
cider-eldoc cider-test cider-stacktrace cider-doc advice
cider-browse-spec cider-clojuredocs cider-overlays cider-jar arc-mode
archive-mode cider-client cider-common cider-connection cider-util color
cider-popup sesman-browser nrepl-client tramp tramp-loaddefs trampver
tramp-integration files-x tramp-compat shell pcomplete parse-time
iso8601 ls-lisp format-spec queue nrepl-dict cider-compat spinner
parseedn parseclj-parser parseclj-lex parseclj-alist flymake-kondor
sesman clojure-mode align imenu smerge-mode diff vc-dir ewoc time-date
noutline outline checkdoc lisp-mnt mail-parse rfc2231 rfc2047 rfc2045
mm-util ietf-drums mail-prsvr dired-x dired dired-loaddefs elec-pair
flymake-proc flymake compile text-property-search comint flyspell ispell
goto-addr thingatpt pp vc-git diff-mode easy-mmode vc vc-dispatcher
cursor-sensor server modus-operandi-theme modus-themes delight comp
comp-cstr warnings cl-extra help-mode savehist so-long cap-words
superword subword windmove winner company-oddmuse company-keywords
company-etags etags fileloop generator xref project ring company-gtags
company-dabbrev-code company-dabbrev company-files company-clang
company-capf company-cmake company-semantic company-template
company-bbdb company pcase envrc inheritenv ansi-color icomplete rx
delsel info tex-site package browse-url url url-proxy url-privacy
url-expand url-methods url-history url-cookie url-domsuf url-util
mailcap url-handlers url-parse auth-source cl-seq eieio eieio-core
cl-macs eieio-loaddefs password-cache json map url-vars seq gv subr-x
byte-opt bytecomp byte-compile cconv cl-loaddefs cl-lib iso-transl
tooltip eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel term/x-win x-win term/common-win x-dnd
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 cl-generic
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 simple abbrev obarray
cl-preloaded nadvice button 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
dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo move-toolbar gtk x-toolkit x multi-tty
make-network-process native-compile emacs)

Memory information:
((conses 16 539597 325220)
  (symbols 48 27815 1)
  (strings 32 136565 77490)
  (string-bytes 1 5745034)
  (vectors 16 47250)
  (vector-slots 8 1113451 95518)
  (floats 8 236 658)
  (intervals 56 2079 129)
  (buffers 992 30))

-- 
Manuel Uberti
www.manueluberti.eu

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

> I don't know if I am missing something, but I don't think staging specific
> hunks from vc-git-mode or diff-mode is possible at the moment.
>
> Is adding such a capability to one (or both) of these modes worth
> considering? I know Magit does it, and I remember something similar in
> a git-gutter package. It's probably something diff-hl does as well by now
> (I have not checked).
>
> I feel like it'd be a nice improvement to VC, what do you think?

I tried to do what is described in the second part of
https://lists.gnu.org/archive/html/emacs-devel/2021-08/msg00207.html
Is this the same that you request here?

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Manuel Uberti via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 07/12/21 18:03, Juri Linkov wrote:
> I tried to do what is described in the second part of
> https://lists.gnu.org/archive/html/emacs-devel/2021-08/msg00207.html
> Is this the same that you request here?

It does look like the same approach. I was only thinking of staging specific 
hunks, while you suggest killing them as well, which would be just as useful I 
guess.

My use-case is the following: I often do different changes to a file (or in a 
project) and it may happen that I have to stage, commit, and push just some of 
them, while keep working on the others. If this matches your proposal somehow, 
then we are saying the same thing indeed.

-- 
Manuel Uberti
www.manueluberti.eu

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

>> I tried to do what is described in the second part of
>> https://lists.gnu.org/archive/html/emacs-devel/2021-08/msg00207.html
>> Is this the same that you request here?
>
> It does look like the same approach. I was only thinking of staging
> specific hunks, while you suggest killing them as well, which would be just
> as useful I guess.
>
> My use-case is the following: I often do different changes to a file (or in
> a project) and it may happen that I have to stage, commit, and push just
> some of them, while keep working on the others. If this matches your
> proposal somehow, then we are saying the same thing indeed.

This is exactly my use case as well: when some parts of the file are not
yet ready to commit, then these hunks can be deleted from the output
of 'C-x v D'.  Then the remaining patch in the *vc-diff* buffer could be
committed after temporarily moving all file changes into the stash.
After committing the selected changes, the stash should be returned back
by using `stash pop`.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

On 07.12.2021 11:59, Manuel Uberti via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> It's probably something diff-hl does as well by now (I have not checked).

It does, since recently.

But Juri's approach seems like it will fit VC's design better, given 
that we don't have a semantic way to "show all stages hunks" or commit 
them only (or instruct Emacs to do that).

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

>> It's probably something diff-hl does as well by now (I have not checked).
>
> It does, since recently.
>
> But Juri's approach seems like it will fit VC's design better, given that
> we don't have a semantic way to "show all stages hunks" or commit them only
> (or instruct Emacs to do that).

The problem is that 'git apply --cached' doesn't perform the merge
with other changes in the same file, whereas 'git stash pop'
merges committed changes with uncommitted changes.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

On 07.12.2021 22:06, Juri Linkov wrote:
> The problem is that 'git apply --cached' doesn't perform the merge
> with other changes in the same file, whereas 'git stash pop'
> merges committed changes with uncommitted changes.

This seems to address our previous discussion, rather than the 
difference vs. diff-hl.

Anyway, I don't know if it is a problem.

E.g., you might want to edit a diff (if you know how, which is a 
significant "if") to commit a slightly different change than what the 
current file contents show.

But then, I'm not sure you'll want the applied change to be reflected in 
the file on disk too (as opposed to being saved in the commit). I 
probably won't (and it would let us avoid the awkward step of seeing the 
stashing operation temporarily reflected in the file contents, as well 
as any possible conflicts).

Either way, the editing of the diff that's more complex than splitting 
hunks and deleting some of them will probably be very rare. So the 
behavior in this scenario doesn't have to affect our choice of 
implementation.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

>> The problem is that 'git apply --cached' doesn't perform the merge
>> with other changes in the same file, whereas 'git stash pop'
>> merges committed changes with uncommitted changes.
>
> This seems to address our previous discussion, rather than the difference
> vs. diff-hl.
>
> Anyway, I don't know if it is a problem.
>
> E.g., you might want to edit a diff (if you know how, which is
> a significant "if") to commit a slightly different change than what the
> current file contents show.

Actually, not quite edit, but only to delete unneeded hunks with ‘k’.

The intention is to emulate the interactive command `git add --patch`
that has many keys:

  y - stage this hunk
  n - do not stage this hunk
  q - quit; do not stage this hunk or any of the remaining ones
  a - stage this hunk and all later hunks in the file
  d - do not stage this hunk or any of the later hunks in the file
  g - select a hunk to go to
  / - search for a hunk matching the given regex
  j - leave this hunk undecided, see next undecided hunk
  J - leave this hunk undecided, see next hunk
  k - leave this hunk undecided, see previous undecided hunk
  K - leave this hunk undecided, see previous hunk
  s - split the current hunk into smaller hunks
  e - manually edit the current hunk
  ? - print help

Instead of these numerous keys, in Emacs it should be sufficient
just to type ‘k’ (diff-hunk-kill) a few times on the output of ‘C-x v D’
in the *vc-diff* buffer (and maybe some splitting).

> But then, I'm not sure you'll want the applied change to be reflected in
> the file on disk too (as opposed to being saved in the commit). I probably
> won't (and it would let us avoid the awkward step of seeing the stashing
> operation temporarily reflected in the file contents, as well as any
> possible conflicts).
>
> Either way, the editing of the diff that's more complex than splitting
> hunks and deleting some of them will probably be very rare. So the behavior
> in this scenario doesn't have to affect our choice of implementation.

I had in mind a different scenario: when you have uncommitted changes
in one part of the file, and receive an external patch from outside of the repo
with changes in another part of the file, and need to commit it.
But I admit such scenario is very rare.

So if using ‘git apply --cached’ is the preferred solution,
then I could finish the patch with it.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

On 08.12.2021 21:57, Juri Linkov wrote:
>>> The problem is that 'git apply --cached' doesn't perform the merge
>>> with other changes in the same file, whereas 'git stash pop'
>>> merges committed changes with uncommitted changes.
>>
>> This seems to address our previous discussion, rather than the difference
>> vs. diff-hl.
>>
>> Anyway, I don't know if it is a problem.
>>
>> E.g., you might want to edit a diff (if you know how, which is
>> a significant "if") to commit a slightly different change than what the
>> current file contents show.
> 
> Actually, not quite edit, but only to delete unneeded hunks with ‘k’.

Then it will probably be fine, most of the time?

> The intention is to emulate the interactive command `git add --patch`
> that has many keys:
> 
>    y - stage this hunk
>    n - do not stage this hunk
>    q - quit; do not stage this hunk or any of the remaining ones
>    a - stage this hunk and all later hunks in the file
>    d - do not stage this hunk or any of the later hunks in the file
>    g - select a hunk to go to
>    / - search for a hunk matching the given regex
>    j - leave this hunk undecided, see next undecided hunk
>    J - leave this hunk undecided, see next hunk
>    k - leave this hunk undecided, see previous undecided hunk
>    K - leave this hunk undecided, see previous hunk
>    s - split the current hunk into smaller hunks
>    e - manually edit the current hunk
>    ? - print help
> 
> Instead of these numerous keys, in Emacs it should be sufficient
> just to type ‘k’ (diff-hunk-kill) a few times on the output of ‘C-x v D’
> in the *vc-diff* buffer (and maybe some splitting).

Yes, the commit-patch (external package) workflow. It totally sounds 
fine to me, though it might require some additional explanation when a 
random user tries to take advantage of it (Git's interactive command UI 
is more self-explanatory).

>> But then, I'm not sure you'll want the applied change to be reflected in
>> the file on disk too (as opposed to being saved in the commit). I probably
>> won't (and it would let us avoid the awkward step of seeing the stashing
>> operation temporarily reflected in the file contents, as well as any
>> possible conflicts).
>>
>> Either way, the editing of the diff that's more complex than splitting
>> hunks and deleting some of them will probably be very rare. So the behavior
>> in this scenario doesn't have to affect our choice of implementation.
> 
> I had in mind a different scenario: when you have uncommitted changes
> in one part of the file, and receive an external patch from outside of the repo
> with changes in another part of the file, and need to commit it.

Couldn't you 'git apply external/patch/file/name.ext' first?

> But I admit such scenario is very rare.
>
> So if using ‘git apply --cached’ is the preferred solution,
> then I could finish the patch with it.

I don't have a particularly strong opinion, but the approach with 'git 
apply --cached' followed by 'git commit' seems easier to implement and 
avoids changing the file contents on disk, or risking any of the 
stashes. So I'd try implementing it first and then see if the remaining 
problems are worth the trouble of doing it in a more difficult way.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

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

Hi Dmitry,

>> So if using ‘git apply --cached’ is the preferred solution,
>> then I could finish the patch with it.
>
> I don't have a particularly strong opinion, but the approach with 'git
> apply --cached' followed by 'git commit' seems easier to implement and
> avoids changing the file contents on disk, or risking any of the
> stashes. So I'd try implementing it first and then see if the remaining
> problems are worth the trouble of doing it in a more difficult way.

This is implemented now, and I'm already using it without problems.
Please review this patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-git-checkin-from-diff.patch --]
[-- Type: text/x-diff, Size: 5743 bytes --]

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 0bf7899246..84128d40b8 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2860,6 +2860,17 @@ diff-syntax-fontify-props
     (nreverse props)))
 
 
+;;;###autoload
+(defun diff-vc-deduce-fileset ()
+  (let ((backend (vc-responsible-backend default-directory))
+        files)
+    (save-excursion
+      (goto-char (point-min))
+      (while (progn (diff-file-next) (not (eobp)))
+        (push (diff-find-file-name nil t) files)))
+    (list backend (nreverse files) nil nil 'patch)))
+
+
 (defun diff--filter-substring (str)
   (when diff-font-lock-prettify
     ;; Strip the `display' properties added by diff-font-lock-prettify,
diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index 5c664d58f1..5400352efe 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -753,7 +753,8 @@ vc-finish-logentry
 (defun vc-dispatcher-browsing ()
   "Are we in a directory browser buffer?"
   (or (derived-mode-p 'vc-dir-mode)
-      (derived-mode-p 'dired-mode)))
+      (derived-mode-p 'dired-mode)
+      (derived-mode-p 'diff-mode)))
 
 ;; These are unused.
 ;; (defun vc-dispatcher-in-fileset-p (fileset)
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 208cbee0e6..ddadca2084 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -942,13 +942,23 @@ vc-git-checkin
           ;; message.  Handle also remote files.
           (if (eq system-type 'windows-nt)
               (let ((default-directory (file-name-directory file1)))
-                (make-nearby-temp-file "git-msg")))))
+                (make-nearby-temp-file "git-msg"))))
+         (patch-buffer (derived-mode-p 'diff-mode)))
+    (when patch-buffer
+      (let ((patch-string (buffer-string))
+            (patch-file (make-temp-file "git-patch")))
+        (with-temp-file patch-file
+          (insert patch-string))
+        (unwind-protect
+            (apply #'vc-git-command nil 0 patch-file
+                   (list "apply" "--cached"))
+          (delete-file patch-file))))
     (cl-flet ((boolean-arg-fn
                (argument)
                (lambda (value) (when (equal value "yes") (list argument)))))
       ;; When operating on the whole tree, better pass "-a" than ".", since "."
       ;; fails when we're committing a merge.
-      (apply #'vc-git-command nil 0 (if only files)
+      (apply #'vc-git-command nil 0 (if (and only (not patch-buffer)) files)
              (nconc (if msg-file (list "commit" "-F"
                                        (file-local-name msg-file))
                       (list "commit" "-m"))
@@ -966,7 +976,8 @@ vc-git-checkin
                           (write-region (car args) nil msg-file))
                         (setq args (cdr args)))
                       args)
-		    (if only (list "--only" "--") '("-a")))))
+		    (unless patch-buffer
+                      (if only (list "--only" "--") '("-a"))))))
     (if (and msg-file (file-exists-p msg-file)) (delete-file msg-file))))
 
 (defun vc-git-find-revision (file rev buffer)
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index a6124acadd..d55cf64b3d 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1098,6 +1098,8 @@ vc-deduce-fileset-1
                             state-model-only-files)
   (let (backend)
     (cond
+     ((derived-mode-p 'diff-mode)
+      (diff-vc-deduce-fileset))
      ((derived-mode-p 'vc-dir-mode)
       (vc-dir-deduce-fileset state-model-only-files))
      ((derived-mode-p 'dired-mode)
@@ -1114,7 +1116,8 @@ vc-deduce-fileset-1
            (or (buffer-file-name vc-parent-buffer)
 				(with-current-buffer vc-parent-buffer
 				  (or (derived-mode-p 'vc-dir-mode)
-				      (derived-mode-p 'dired-mode)))))
+				      (derived-mode-p 'dired-mode)
+				      (derived-mode-p 'diff-mode)))))
       (progn                  ;FIXME: Why not `with-current-buffer'? --Stef.
 	(set-buffer vc-parent-buffer)
 	(vc-deduce-fileset-1 not-state-changing allow-unregistered state-model-only-files)))
@@ -1230,6 +1233,7 @@ vc-next-action
       (error "Fileset files are missing, so cannot be operated on"))
      ((eq state 'ignored)
       (error "Fileset files are ignored by the version-control system"))
+     ((eq model 'patch) (vc-checkin files backend))
      ((or (null state) (eq state 'unregistered))
       (cond (verbose
              (let ((backend (vc-read-backend "Backend to register to: ")))
@@ -1800,7 +1804,11 @@ vc-diff-internal
          (orig-diff-buffer-clone
           (if revert-buffer-in-progress-p
               (clone-buffer
-               (generate-new-buffer-name " *vc-diff-clone*") nil))))
+               (generate-new-buffer-name " *vc-diff-clone*") nil)))
+         (patch-buffer (and (buffer-live-p vc-parent-buffer)
+                            (with-current-buffer vc-parent-buffer
+			      (derived-mode-p 'diff-mode))
+                            vc-parent-buffer)))
     ;; 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.
@@ -1837,8 +1845,11 @@ vc-diff-internal
                      (if async 'async 1) "diff" file
                      (append (vc-switches nil 'diff) `(,(null-device)))))))
         (setq files (nreverse filtered))))
-    (vc-call-backend (car vc-fileset) 'diff files rev1 rev2 buffer async)
+    (unless patch-buffer
+      (vc-call-backend (car vc-fileset) 'diff files rev1 rev2 buffer async))
     (set-buffer buffer)
+    (when patch-buffer
+      (insert (with-current-buffer patch-buffer (buffer-string))))
     ;; Make the *vc-diff* buffer read only, the diff-mode key
     ;; bindings are nicer for read only buffers. pcl-cvs does the
     ;; same thing.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

Hi Juri!

On 08.02.2022 21:57, Juri Linkov wrote:
> This is implemented now, and I'm already using it without problems.
> Please review this patch:

Looking good.

But could you explain the case when the changes to 'vc-diff-internal' 
are going to be used? If those are only for log-edit-show-diff, I think 
it'd be better if the new logic was implemented in the new value of 
log-edit-diff-function, rather than having it spliced into the common 
code path. Would that result in a lot of code duplication?

It might also be worth it to thread the 'patch-buffer' value through the 
backend method arguments (the actual value will be the patch string), so 
that vc-git-checkin gets it in the 4th argument, rather than having it 
call (derived-mode-p 'diff-mode) (this feels a little brittle: I suppose 
which buffer is current during this call might change in the future). It 
would also automatically weed out backends which don't support this 
feature, rather than having an attempt to commit from a diff buffer 
using Hg fail silently.

But the current method is decent too, up to you.

Finally, we'd should probably have at least one test in vc-tests.el 
which exercises the new functionality. Though I guess it might be tricky 
to set up.

> +;;;###autoload
> +(defun diff-vc-deduce-fileset ()
> +  (let ((backend (vc-responsible-backend default-directory))
> +        files)
> +    (save-excursion
> +      (goto-char (point-min))
> +      (while (progn (diff-file-next) (not (eobp)))
> +        (push (diff-find-file-name nil t) files)))
> +    (list backend (nreverse files) nil nil 'patch)))
> +
> +

Very nitpicky nitpick: please drop the extra empty lines before and 
after this function.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

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

> But could you explain the case when the changes to 'vc-diff-internal' are
> going to be used? If those are only for log-edit-show-diff, I think it'd be
> better if the new logic was implemented in the new value of
> log-edit-diff-function, rather than having it spliced into the common code
> path. Would that result in a lot of code duplication?

Not much code duplication, thanks for the suggestion, the patch below
sets log-edit-diff-function for log-edit-show-diff.

> It might also be worth it to thread the 'patch-buffer' value through the
> backend method arguments (the actual value will be the patch string), so
> that vc-git-checkin gets it in the 4th argument, rather than having it call
> (derived-mode-p 'diff-mode) (this feels a little brittle: I suppose which
> buffer is current during this call might change in the future). It would
> also automatically weed out backends which don't support this feature,
> rather than having an attempt to commit from a diff buffer using Hg fail
> silently.

I agree that in the first version ‘(derived-mode-p 'diff-mode)’ was brittle.
But changing the established API with a new argument doesn't look right.
So the next version below uses the buffer-local variable ‘vc-patch-string’.
In other backends such as Hg it shouldn't fail silently, but it will be
just less granular, and will commit whole files instead of edited diffs.

There is a new problem however.  After starting vc-log-edit from *vc-diff*,
and using log-edit-show-diff, it reuses the original buffer *vc-diff*.
This is not a problem, because the buffer-local variable ‘vc-patch-string’
is saved in the *vc-log* buffer.  But after deleting *vc-diff*, log-edit-done
fails on the deleted vc-parent-buffer with

  Debugger entered--Lisp error: (error "Selecting deleted buffer")
    vc-finish-logentry()
    funcall-interactively(vc-finish-logentry)
    log-edit-done()

But this is an old problem.  The same error is signaled
after typing ‘v’ in *vc-dir* buffer, then deleting the
original *vc-dir* buffer, and trying to type ‘C-c C-c’
(log-edit-done) in the *vc-log* buffer.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-patch-string.patch --]
[-- Type: text/x-diff, Size: 6034 bytes --]

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 0bf7899246..50f68bef70 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2859,6 +2859,15 @@ diff-syntax-fontify-props
         (forward-line 1)))
     (nreverse props)))
 
+;;;###autoload
+(defun diff-vc-deduce-fileset ()
+  (let ((backend (vc-responsible-backend default-directory))
+        files)
+    (save-excursion
+      (goto-char (point-min))
+      (while (progn (diff-file-next) (not (eobp)))
+        (push (diff-find-file-name nil t) files)))
+    (list backend (nreverse files) nil nil 'patch)))
 
 (defun diff--filter-substring (str)
   (when diff-font-lock-prettify
diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index 5c664d58f1..dc141fd650 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -653,9 +653,14 @@ vc-log-edit
                      (mapcar
                       (lambda (file) (file-relative-name file root))
                       ',fileset))))
-	      (log-edit-diff-function . vc-diff)
+	      (log-edit-diff-function
+               . ,(if (buffer-local-value 'vc-patch-buffer vc-parent-buffer)
+                      'vc-diff-patch 'vc-diff))
 	      (log-edit-vc-backend . ,backend)
-	      (vc-log-fileset . ,fileset))
+	      (vc-log-fileset . ,fileset)
+              ,@(if (buffer-local-value 'vc-patch-buffer vc-parent-buffer)
+                    `((vc-patch-string . ,(with-current-buffer vc-parent-buffer
+                                            (buffer-string))))))
 	    nil
 	    mode)
   (set-buffer-modified-p nil)
@@ -753,7 +758,8 @@ vc-finish-logentry
 (defun vc-dispatcher-browsing ()
   "Are we in a directory browser buffer?"
   (or (derived-mode-p 'vc-dir-mode)
-      (derived-mode-p 'dired-mode)))
+      (derived-mode-p 'dired-mode)
+      (derived-mode-p 'diff-mode)))
 
 ;; These are unused.
 ;; (defun vc-dispatcher-in-fileset-p (fileset)
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 208cbee0e6..4eb114633d 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -943,12 +943,20 @@ vc-git-checkin
           (if (eq system-type 'windows-nt)
               (let ((default-directory (file-name-directory file1)))
                 (make-nearby-temp-file "git-msg")))))
+    (when vc-patch-string
+      (let ((patch-file (make-temp-file "git-patch")))
+        (with-temp-file patch-file
+          (insert vc-patch-string))
+        (unwind-protect
+            (apply #'vc-git-command nil 0 patch-file
+                   (list "apply" "--cached"))
+          (delete-file patch-file))))
     (cl-flet ((boolean-arg-fn
                (argument)
                (lambda (value) (when (equal value "yes") (list argument)))))
       ;; When operating on the whole tree, better pass "-a" than ".", since "."
       ;; fails when we're committing a merge.
-      (apply #'vc-git-command nil 0 (if only files)
+      (apply #'vc-git-command nil 0 (if (and only (not vc-patch-string)) files)
              (nconc (if msg-file (list "commit" "-F"
                                        (file-local-name msg-file))
                       (list "commit" "-m"))
@@ -966,7 +974,8 @@ vc-git-checkin
                           (write-region (car args) nil msg-file))
                         (setq args (cdr args)))
                       args)
-		    (if only (list "--only" "--") '("-a")))))
+                    (unless vc-patch-string
+                      (if only (list "--only" "--") '("-a"))))))
     (if (and msg-file (file-exists-p msg-file)) (delete-file msg-file))))
 
 (defun vc-git-find-revision (file rev buffer)
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index a6124acadd..1b30e9b671 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1102,6 +1102,8 @@ vc-deduce-fileset-1
       (vc-dir-deduce-fileset state-model-only-files))
      ((derived-mode-p 'dired-mode)
       (dired-vc-deduce-fileset state-model-only-files not-state-changing))
+     ((derived-mode-p 'diff-mode)
+      (diff-vc-deduce-fileset))
      ((setq backend (vc-backend buffer-file-name))
       (if state-model-only-files
 	(list backend (list buffer-file-name)
@@ -1114,7 +1116,8 @@ vc-deduce-fileset-1
            (or (buffer-file-name vc-parent-buffer)
 				(with-current-buffer vc-parent-buffer
 				  (or (derived-mode-p 'vc-dir-mode)
-				      (derived-mode-p 'dired-mode)))))
+				      (derived-mode-p 'dired-mode)
+				      (derived-mode-p 'diff-mode)))))
       (progn                  ;FIXME: Why not `with-current-buffer'? --Stef.
 	(set-buffer vc-parent-buffer)
 	(vc-deduce-fileset-1 not-state-changing allow-unregistered state-model-only-files)))
@@ -1230,6 +1233,9 @@ vc-next-action
       (error "Fileset files are missing, so cannot be operated on"))
      ((eq state 'ignored)
       (error "Fileset files are ignored by the version-control system"))
+     ((eq model 'patch)
+      (setq-local vc-patch-buffer t)
+      (vc-checkin files backend))
      ((or (null state) (eq state 'unregistered))
       (cond (verbose
              (let ((backend (vc-read-backend "Backend to register to: ")))
@@ -1779,6 +1785,24 @@ vc-diff-finish
 (defvar vc-diff-added-files nil
   "If non-nil, diff added files by comparing them to /dev/null.")
 
+(defvar vc-patch-buffer nil)
+(defvar vc-patch-string nil)
+
+(defun vc-diff-patch ()
+  (let ((buffer "*vc-diff*")
+        (patch-string vc-patch-string))
+    (vc-setup-buffer buffer)
+    (set-buffer buffer)
+    (let ((buffer-undo-list t)
+          (inhibit-read-only t))
+      (insert patch-string))
+    (setq buffer-read-only t)
+    (diff-mode)
+    (setq-local diff-vc-backend (vc-responsible-backend default-directory))
+    (setq-local revert-buffer-function (lambda (_ _) (vc-diff-patch)))
+    (pop-to-buffer (current-buffer))
+    (vc-run-delayed (vc-diff-finish (current-buffer) nil))))
+
 (defun vc-diff-internal (async vc-fileset rev1 rev2 &optional verbose buffer)
   "Report diffs between two revisions of a fileset.
 Output goes to the buffer BUFFER, which defaults to *vc-diff*.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

On 12.02.2022 21:42, Juri Linkov wrote:
>> But could you explain the case when the changes to 'vc-diff-internal' are
>> going to be used? If those are only for log-edit-show-diff, I think it'd be
>> better if the new logic was implemented in the new value of
>> log-edit-diff-function, rather than having it spliced into the common code
>> path. Would that result in a lot of code duplication?
> 
> Not much code duplication, thanks for the suggestion, the patch below
> sets log-edit-diff-function for log-edit-show-diff.

Nice.

>> It might also be worth it to thread the 'patch-buffer' value through the
>> backend method arguments (the actual value will be the patch string), so
>> that vc-git-checkin gets it in the 4th argument, rather than having it call
>> (derived-mode-p 'diff-mode) (this feels a little brittle: I suppose which
>> buffer is current during this call might change in the future). It would
>> also automatically weed out backends which don't support this feature,
>> rather than having an attempt to commit from a diff buffer using Hg fail
>> silently.
> 
> I agree that in the first version ‘(derived-mode-p 'diff-mode)’ was brittle.
> But changing the established API with a new argument doesn't look right.

We could make sure to call the function with the current number of 
arguments when a patch-buffer is not used, and with the additional one 
when it is used. Which would automatically force an abort when a backend 
does not support this feature.

> So the next version below uses the buffer-local variable ‘vc-patch-string’.
> In other backends such as Hg it shouldn't fail silently, but it will be
> just less granular, and will commit whole files instead of edited diffs.

That's not the worst approach, but it's bound to trip up some users who 
get used to the new feature's behavior with Git repos, and then try it 
with a different one that does not support this feature properly (such 
as Hg). Without any warning, they will see a different behavior.

What do people think? Is that not a problem?

> There is a new problem however.  After starting vc-log-edit from *vc-diff*,
> and using log-edit-show-diff, it reuses the original buffer *vc-diff*.
> This is not a problem, because the buffer-local variable ‘vc-patch-string’
> is saved in the *vc-log* buffer.  But after deleting *vc-diff*, log-edit-done
> fails on the deleted vc-parent-buffer with
> 
>    Debugger entered--Lisp error: (error "Selecting deleted buffer")
>      vc-finish-logentry()
>      funcall-interactively(vc-finish-logentry)
>      log-edit-done()
> 
> But this is an old problem.  The same error is signaled
> after typing ‘v’ in *vc-dir* buffer, then deleting the
> original *vc-dir* buffer, and trying to type ‘C-c C-c’
> (log-edit-done) in the *vc-log* buffer.

Seems like a slightly different issue, though. Speaking of myself only, 
I can see myself casually killing the vc-diff buffer (especially if I 
just displayed it with C-c C-d), but it seems less likely that I would 
kill the vc-dir buffer when completing the commit.

I suppose 'vc-diff-patch' could use a different buffer name than 
"*vc-diff*" and thus avoid reusing that buffer?

If it brings other problems somehow, oh well. An old bug is something we 
can live with.

But also note that if the patch string was passed as an argument to the 
backend action, this problem might be avoided as well.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

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

> We could make sure to call the function with the current number of
> arguments when a patch-buffer is not used, and with the additional one when
> it is used. Which would automatically force an abort when a backend does
> not support this feature.

This is fine, so this patch does this:

         (if patch-string
             (vc-call-backend backend 'checkin files comment rev patch-string)
           (vc-call-backend backend 'checkin files comment rev))

Also I added the new arg ‘patch-string’ to vc-checkin and vc-start-logentry.
These are not API calls, so they could use a buffer-local variables instead
of args, but I'm not sure about this less important technical detail.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-checkin-patch-string.patch --]
[-- Type: text/x-diff, Size: 9142 bytes --]

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 0bf7899246..50f68bef70 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2859,6 +2859,15 @@ diff-syntax-fontify-props
         (forward-line 1)))
     (nreverse props)))
 
+;;;###autoload
+(defun diff-vc-deduce-fileset ()
+  (let ((backend (vc-responsible-backend default-directory))
+        files)
+    (save-excursion
+      (goto-char (point-min))
+      (while (progn (diff-file-next) (not (eobp)))
+        (push (diff-find-file-name nil t) files)))
+    (list backend (nreverse files) nil nil 'patch)))
 
 (defun diff--filter-substring (str)
   (when diff-font-lock-prettify
diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index 5c664d58f1..d000e8eedc 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -653,15 +655,17 @@ vc-log-edit
                      (mapcar
                       (lambda (file) (file-relative-name file root))
                       ',fileset))))
-	      (log-edit-diff-function . vc-diff)
+	      (log-edit-diff-function
+               . ,(if vc-patch-string 'vc-diff-patch 'vc-diff))
 	      (log-edit-vc-backend . ,backend)
-	      (vc-log-fileset . ,fileset))
+	      (vc-log-fileset . ,fileset)
+	      (vc-patch-string . ,vc-patch-string))
 	    nil
 	    mode)
   (set-buffer-modified-p nil)
   (setq buffer-file-name nil))
 
-(defun vc-start-logentry (files comment initial-contents msg logbuf mode action &optional after-hook backend)
+(defun vc-start-logentry (files comment initial-contents msg logbuf mode action &optional after-hook backend patch-string)
   "Accept a comment for an operation on FILES.
 If COMMENT is nil, pop up a LOGBUF buffer, emit MSG, and set the
 action on close to ACTION.  If COMMENT is a string and
@@ -688,6 +692,7 @@ vc-start-logentry
     (setq-local vc-parent-buffer parent)
     (setq-local vc-parent-buffer-name
                 (concat " from " (buffer-name vc-parent-buffer)))
+    (setq-local vc-patch-string patch-string)
     (vc-log-edit files mode backend)
     (make-local-variable 'vc-log-after-operation-hook)
     (when after-hook
@@ -753,7 +758,8 @@ vc-finish-logentry
 (defun vc-dispatcher-browsing ()
   "Are we in a directory browser buffer?"
   (or (derived-mode-p 'vc-dir-mode)
-      (derived-mode-p 'dired-mode)))
+      (derived-mode-p 'dired-mode)
+      (derived-mode-p 'diff-mode)))
 
 ;; These are unused.
 ;; (defun vc-dispatcher-in-fileset-p (fileset)
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 208cbee0e6..aa8bf5ca44 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -53,7 +53,7 @@
 ;; - responsible-p (file)                          OK
 ;; - receive-file (file rev)                       NOT NEEDED
 ;; - unregister (file)                             OK
-;; * checkin (files rev comment)                   OK
+;; * checkin (files comment rev patch-string)      OK
 ;; * find-revision (file rev buffer)               OK
 ;; * checkout (file &optional rev)                 OK
 ;; * revert (file &optional contents-done)         OK
@@ -921,7 +921,7 @@ vc-git-log-edit-mode
   "Major mode for editing Git log messages.
 It is based on `log-edit-mode', and has Git-specific extensions.")
 
-(defun vc-git-checkin (files comment &optional _rev)
+(defun vc-git-checkin (files comment &optional _rev patch-string)
   (let* ((file1 (or (car files) default-directory))
          (root (vc-git-root file1))
          (default-directory (expand-file-name root))
@@ -943,12 +943,20 @@ vc-git-checkin
           (if (eq system-type 'windows-nt)
               (let ((default-directory (file-name-directory file1)))
                 (make-nearby-temp-file "git-msg")))))
+    (when patch-string
+      (let ((patch-file (make-temp-file "git-patch")))
+        (with-temp-file patch-file
+          (insert patch-string))
+        (unwind-protect
+            (apply #'vc-git-command nil 0 patch-file
+                   (list "apply" "--cached"))
+          (delete-file patch-file))))
     (cl-flet ((boolean-arg-fn
                (argument)
                (lambda (value) (when (equal value "yes") (list argument)))))
       ;; When operating on the whole tree, better pass "-a" than ".", since "."
       ;; fails when we're committing a merge.
-      (apply #'vc-git-command nil 0 (if only files)
+      (apply #'vc-git-command nil 0 (if (and only (not patch-string)) files)
              (nconc (if msg-file (list "commit" "-F"
                                        (file-local-name msg-file))
                       (list "commit" "-m"))
@@ -966,7 +974,8 @@ vc-git-checkin
                           (write-region (car args) nil msg-file))
                         (setq args (cdr args)))
                       args)
-		    (if only (list "--only" "--") '("-a")))))
+                    (unless patch-string
+                      (if only (list "--only" "--") '("-a"))))))
     (if (and msg-file (file-exists-p msg-file)) (delete-file msg-file))))
 
 (defun vc-git-find-revision (file rev buffer)
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index a6124acadd..6834de503f 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -239,7 +239,7 @@
 ;;   Unregister FILE from this backend.  This is only needed if this
 ;;   backend may be used as a "more local" backend for temporary editing.
 ;;
-;; * checkin (files comment &optional rev)
+;; * checkin (files comment &optional rev patch-string)
 ;;
 ;;   Commit changes in FILES to this backend.  COMMENT is used as a
 ;;   check-in comment.  The implementation should pass the value of
@@ -1102,6 +1102,8 @@ vc-deduce-fileset-1
       (vc-dir-deduce-fileset state-model-only-files))
      ((derived-mode-p 'dired-mode)
       (dired-vc-deduce-fileset state-model-only-files not-state-changing))
+     ((derived-mode-p 'diff-mode)
+      (diff-vc-deduce-fileset))
      ((setq backend (vc-backend buffer-file-name))
       (if state-model-only-files
 	(list backend (list buffer-file-name)
@@ -1114,7 +1116,8 @@ vc-deduce-fileset-1
            (or (buffer-file-name vc-parent-buffer)
 				(with-current-buffer vc-parent-buffer
 				  (or (derived-mode-p 'vc-dir-mode)
-				      (derived-mode-p 'dired-mode)))))
+				      (derived-mode-p 'dired-mode)
+				      (derived-mode-p 'diff-mode)))))
       (progn                  ;FIXME: Why not `with-current-buffer'? --Stef.
 	(set-buffer vc-parent-buffer)
 	(vc-deduce-fileset-1 not-state-changing allow-unregistered state-model-only-files)))
@@ -1230,6 +1233,8 @@ vc-next-action
       (error "Fileset files are missing, so cannot be operated on"))
      ((eq state 'ignored)
       (error "Fileset files are ignored by the version-control system"))
+     ((eq model 'patch)
+      (vc-checkin files backend nil nil nil (buffer-string)))
      ((or (null state) (eq state 'unregistered))
       (cond (verbose
              (let ((backend (vc-read-backend "Backend to register to: ")))
@@ -1615,7 +1620,7 @@ vc-steal-lock
      ".\n")
     (message "Please explain why you stole the lock.  Type C-c C-c when done.")))
 
-(defun vc-checkin (files backend &optional comment initial-contents rev)
+(defun vc-checkin (files backend &optional comment initial-contents rev patch-string)
   "Check in FILES. COMMENT is a comment string; if omitted, a
 buffer is popped up to accept a comment.  If INITIAL-CONTENTS is
 non-nil, then COMMENT is used as the initial contents of the log
@@ -1643,7 +1650,9 @@ vc-checkin
        ;; vc-checkin-switches, but 'the' local buffer is
        ;; not a well-defined concept for filesets.
        (progn
-         (vc-call-backend backend 'checkin files comment rev)
+         (if patch-string
+             (vc-call-backend backend 'checkin files comment rev patch-string)
+           (vc-call-backend backend 'checkin files comment rev))
          (mapc #'vc-delete-automatic-version-backups files))
        `((vc-state . up-to-date)
          (vc-checkout-time . ,(file-attribute-modification-time
@@ -1651,7 +1660,8 @@ vc-checkin
          (vc-working-revision . nil)))
      (message "Checking in %s...done" (vc-delistify files)))
    'vc-checkin-hook
-   backend))
+   backend
+   patch-string))
 
 ;;; Additional entry points for examining version histories
 
@@ -1779,6 +1789,23 @@ vc-diff-finish
 (defvar vc-diff-added-files nil
   "If non-nil, diff added files by comparing them to /dev/null.")
 
+(defvar vc-patch-buffer nil)
+
+(defun vc-diff-patch ()
+  (let ((buffer "*vc-diff*")
+        (patch-string vc-patch-string))
+    (vc-setup-buffer buffer)
+    (set-buffer buffer)
+    (let ((buffer-undo-list t)
+          (inhibit-read-only t))
+      (insert patch-string))
+    (setq buffer-read-only t)
+    (diff-mode)
+    (setq-local diff-vc-backend (vc-responsible-backend default-directory))
+    (setq-local revert-buffer-function (lambda (_ _) (vc-diff-patch)))
+    (pop-to-buffer (current-buffer))
+    (vc-run-delayed (vc-diff-finish (current-buffer) nil))))
+
 (defun vc-diff-internal (async vc-fileset rev1 rev2 &optional verbose buffer)
   "Report diffs between two revisions of a fileset.
 Output goes to the buffer BUFFER, which defaults to *vc-diff*.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

On 13.02.2022 21:48, Juri Linkov wrote:
> This is fine, so this patch does this:
> 
>           (if patch-string
>               (vc-call-backend backend 'checkin files comment rev patch-string)
>             (vc-call-backend backend 'checkin files comment rev))

Looks good, thanks!

Sorry, I haven't noticed your emails today before I wrote my own. See if 
you like the suggestion in there, but this patch is also fine with me. 
Though we might try catching the wrong-number-of-arguments error to 
report a friendlier explanation to the user.

> Also I added the new arg ‘patch-string’ to vc-checkin and vc-start-logentry.
> These are not API calls, so they could use a buffer-local variables instead
> of args, but I'm not sure about this less important technical detail.

Since the new args are optional, should be fine.

> +    (when patch-string
> +      (let ((patch-file (make-temp-file "git-patch")))
> +        (with-temp-file patch-file
> +          (insert patch-string))
> +        (unwind-protect
> +            (apply #'vc-git-command nil 0 patch-file
> +                   (list "apply" "--cached"))
> +          (delete-file patch-file))))

Perhaps we should also check that there are no existing staged changes 
for those files, and if so, abort? This way we won't commit anything 
else by accident.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

>> There is a new problem however.  After starting vc-log-edit from *vc-diff*,
>> and using log-edit-show-diff, it reuses the original buffer *vc-diff*.
>> This is not a problem, because the buffer-local variable ‘vc-patch-string’
>> is saved in the *vc-log* buffer.  But after deleting *vc-diff*, log-edit-done
>> fails on the deleted vc-parent-buffer with
>>    Debugger entered--Lisp error: (error "Selecting deleted buffer")
>>      vc-finish-logentry()
>>      funcall-interactively(vc-finish-logentry)
>>      log-edit-done()
>> But this is an old problem.  The same error is signaled
>> after typing ‘v’ in *vc-dir* buffer, then deleting the
>> original *vc-dir* buffer, and trying to type ‘C-c C-c’
>> (log-edit-done) in the *vc-log* buffer.
>
> Seems like a slightly different issue, though. Speaking of myself only,
> I can see myself casually killing the vc-diff buffer (especially if I just
> displayed it with C-c C-d), but it seems less likely that I would kill the
> vc-dir buffer when completing the commit.
>
> I suppose 'vc-diff-patch' could use a different buffer name than
> "*vc-diff*" and thus avoid reusing that buffer?
>
> If it brings other problems somehow, oh well. An old bug is something we
> can live with.

Indeed, other buffer names might break user configuration.

> But also note that if the patch string was passed as an argument to the
> backend action, this problem might be avoided as well.

I can't imagine how the problem might be avoided using an argument.
The problem is in these lines in vc-finish-logentry:

    (pop-to-buffer vc-parent-buffer)
    ;; OK, do it to it
    (save-excursion
      (funcall log-operation
	       log-fileset
	       log-entry))

It expects to pop to the original buffer where vc-next-action was initiated.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

On 13.02.2022 21:56, Juri Linkov wrote:
>> I suppose 'vc-diff-patch' could use a different buffer name than
>> "*vc-diff*" and thus avoid reusing that buffer?
>>
>> If it brings other problems somehow, oh well. An old bug is something we
>> can live with.
> Indeed, other buffer names might break user configuration.
> 
>> But also note that if the patch string was passed as an argument to the
>> backend action, this problem might be avoided as well.
> I can't imagine how the problem might be avoided using an argument.
> The problem is in these lines in vc-finish-logentry:
> 
>      (pop-to-buffer vc-parent-buffer)
>      ;; OK, do it to it
>      (save-excursion
>        (funcall log-operation
> 	       log-fileset
> 	       log-entry))
> 
> It expects to pop to the original buffer where vc-next-action was initiated.

OK, I see. Then I would try the other suggested option (using a 
different buffer name).

If you're sure it will cause problems, I'm not going to press the issue, 
but FWIW I don't have (or know of) any customizations that this would 
break. Worst case, people would end up adding an additional entry to 
display-buffer-alist. And maybe to some similar vars as well.

We could postpone solving this problem until later anyway, perhaps when 
someone complains. Or either of us gets bitten by it in practice.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

Hi Dmitry,

> We could postpone solving this problem until later anyway, perhaps when
> someone complains. Or either of us gets bitten by it in practice.

Sorry for the delay, I still can't find a solution, so let's postpone
this problem until later.  I'll prepare a new patch.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

On 13.02.2022 03:32, Dmitry Gutov wrote:
>> But changing the established API with a new argument doesn't look right.
> 
> We could make sure to call the function with the current number of 
> arguments when a patch-buffer is not used, and with the additional one 
> when it is used. Which would automatically force an abort when a backend 
> does not support this feature.

Another solution for making sure the current backend supports the new 
feature is to add a new backend function. Call it 'checkin-patch' or 
`checkin-diff`.

vc-git-checkin and vc-git-checking patch can share some implementation, 
so most of code duplication can be avoided.

But when a backend does not support this, the user will see a 
more-or-less understandable error like "not supported".

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

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

> Another solution for making sure the current backend supports the new
> feature is to add a new backend function. Call it 'checkin-patch' or
> `checkin-diff`.
>
> vc-git-checkin and vc-git-checking patch can share some implementation, so
> most of code duplication can be avoided.
>
> But when a backend does not support this, the user will see a more-or-less
> understandable error like "not supported".

So a new patch adds a new backend function 'checkin-patch':


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: diff-vc-deduce-fileset.patch --]
[-- Type: text/x-diff, Size: 9575 bytes --]

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index e4a1996c1b..5147f37e95 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2928,6 +2928,15 @@ diff-syntax-fontify-props
         (forward-line 1)))
     (nreverse props)))
 
+;;;###autoload
+(defun diff-vc-deduce-fileset ()
+  (let ((backend (vc-responsible-backend default-directory))
+        files)
+    (save-excursion
+      (goto-char (point-min))
+      (while (progn (diff-file-next) (not (eobp)))
+        (push (diff-find-file-name nil t) files)))
+    (list backend (nreverse files) nil nil 'patch)))
 
 (defun diff--filter-substring (str)
   (when diff-font-lock-prettify
diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index e2a490092b..93bcf4e976 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -624,6 +624,8 @@ vc-buffer-sync
 
 (declare-function log-edit-empty-buffer-p "log-edit" ())
 
+(defvar vc-patch-string)
+
 (defun vc-log-edit (fileset mode backend)
   "Set up `log-edit' for use on FILE."
   (setq default-directory
@@ -653,15 +655,17 @@ vc-log-edit
                       (mapcar
                        (lambda (file) (file-relative-name file root))
                        fileset))))
-	      (log-edit-diff-function . vc-diff)
+	      (log-edit-diff-function
+               . ,(if vc-patch-string 'vc-diff-patch 'vc-diff))
 	      (log-edit-vc-backend . ,backend)
-	      (vc-log-fileset . ,fileset))
+	      (vc-log-fileset . ,fileset)
+	      (vc-patch-string . ,vc-patch-string))
 	    nil
 	    mode)
   (set-buffer-modified-p nil)
   (setq buffer-file-name nil))
 
-(defun vc-start-logentry (files comment initial-contents msg logbuf mode action &optional after-hook backend)
+(defun vc-start-logentry (files comment initial-contents msg logbuf mode action &optional after-hook backend patch-string)
   "Accept a comment for an operation on FILES.
 If COMMENT is nil, pop up a LOGBUF buffer, emit MSG, and set the
 action on close to ACTION.  If COMMENT is a string and
@@ -688,6 +692,7 @@ vc-start-logentry
     (setq-local vc-parent-buffer parent)
     (setq-local vc-parent-buffer-name
                 (concat " from " (buffer-name vc-parent-buffer)))
+    (setq-local vc-patch-string patch-string)
     (vc-log-edit files mode backend)
     (make-local-variable 'vc-log-after-operation-hook)
     (when after-hook
@@ -753,7 +758,8 @@ vc-finish-logentry
 (defun vc-dispatcher-browsing ()
   "Are we in a directory browser buffer?"
   (or (derived-mode-p 'vc-dir-mode)
-      (derived-mode-p 'dired-mode)))
+      (derived-mode-p 'dired-mode)
+      (derived-mode-p 'diff-mode)))
 
 ;; These are unused.
 ;; (defun vc-dispatcher-in-fileset-p (fileset)
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 46a486a46c..8458b40714 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -53,7 +53,8 @@
 ;; - responsible-p (file)                          OK
 ;; - receive-file (file rev)                       NOT NEEDED
 ;; - unregister (file)                             OK
-;; * checkin (files rev comment)                   OK
+;; * checkin (files comment rev)                   OK
+;; - checkin-patch (patch-string comment)          OK
 ;; * find-revision (file rev buffer)               OK
 ;; * checkout (file &optional rev)                 OK
 ;; * revert (file &optional contents-done)         OK
@@ -914,6 +915,12 @@ vc-git-log-edit-mode
   "Major mode for editing Git log messages.
 It is based on `log-edit-mode', and has Git-specific extensions.")
 
+(defvar vc-git-patch-string nil)
+
+(defun vc-git-checkin-patch (patch-string comment)
+  (let ((vc-git-patch-string patch-string))
+    (vc-git-checkin nil comment nil)))
+
 (defun vc-git-checkin (files comment &optional _rev)
   (let* ((file1 (or (car files) default-directory))
          (root (vc-git-root file1))
@@ -936,12 +943,20 @@ vc-git-checkin
           (if (eq system-type 'windows-nt)
               (let ((default-directory (file-name-directory file1)))
                 (make-nearby-temp-file "git-msg")))))
+    (when vc-git-patch-string
+      (let ((patch-file (make-temp-file "git-patch")))
+        (with-temp-file patch-file
+          (insert vc-git-patch-string))
+        (unwind-protect
+            (apply #'vc-git-command nil 0 patch-file
+                   (list "apply" "--cached"))
+          (delete-file patch-file))))
     (cl-flet ((boolean-arg-fn
                (argument)
                (lambda (value) (when (equal value "yes") (list argument)))))
       ;; When operating on the whole tree, better pass "-a" than ".", since "."
       ;; fails when we're committing a merge.
-      (apply #'vc-git-command nil 0 (if only files)
+      (apply #'vc-git-command nil 0 (if (and only (not vc-git-patch-string)) files)
              (nconc (if msg-file (list "commit" "-F"
                                        (file-local-name msg-file))
                       (list "commit" "-m"))
@@ -959,7 +974,8 @@ vc-git-checkin
                           (write-region (car args) nil msg-file))
                         (setq args (cdr args)))
                       args)
-		    (if only (list "--only" "--") '("-a")))))
+                    (unless vc-git-patch-string
+                      (if only (list "--only" "--") '("-a"))))))
     (if (and msg-file (file-exists-p msg-file)) (delete-file msg-file))))
 
 (defun vc-git-find-revision (file rev buffer)
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index b05adfb2d5..c5c9835de9 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -247,6 +247,11 @@
 ;;   revision argument is only supported with some older VCSes, like
 ;;   RCS and CVS, and is otherwise silently ignored.
 ;;
+;; - checkin-patch (patch-string comment)
+;;
+;;   Commit a single patch PATCH-STRING to this backend, bypassing
+;;   the changes in filesets.  COMMENT is used as a check-in comment.
+;;
 ;; * find-revision (file rev buffer)
 ;;
 ;;   Fetch revision REV of file FILE and put it into BUFFER.
@@ -1102,6 +1107,8 @@ vc-deduce-fileset-1
       (vc-dir-deduce-fileset state-model-only-files))
      ((derived-mode-p 'dired-mode)
       (dired-vc-deduce-fileset state-model-only-files not-state-changing))
+     ((derived-mode-p 'diff-mode)
+      (diff-vc-deduce-fileset))
      ((setq backend (vc-backend buffer-file-name))
       (if state-model-only-files
 	(list backend (list buffer-file-name)
@@ -1114,7 +1121,8 @@ vc-deduce-fileset-1
            (or (buffer-file-name vc-parent-buffer)
 				(with-current-buffer vc-parent-buffer
 				  (or (derived-mode-p 'vc-dir-mode)
-				      (derived-mode-p 'dired-mode)))))
+				      (derived-mode-p 'dired-mode)
+				      (derived-mode-p 'diff-mode)))))
       (progn                  ;FIXME: Why not `with-current-buffer'? --Stef.
 	(set-buffer vc-parent-buffer)
 	(vc-deduce-fileset-1 not-state-changing allow-unregistered state-model-only-files)))
@@ -1230,6 +1238,8 @@ vc-next-action
       (error "Fileset files are missing, so cannot be operated on"))
      ((eq state 'ignored)
       (error "Fileset files are ignored by the version-control system"))
+     ((eq model 'patch)
+      (vc-checkin files backend nil nil nil (buffer-string)))
      ((or (null state) (eq state 'unregistered))
       (cond (verbose
              (let ((backend (vc-read-backend "Backend to register to: ")))
@@ -1615,7 +1625,9 @@ vc-steal-lock
      ".\n")
     (message "Please explain why you stole the lock.  Type C-c C-c when done.")))
 
-(defun vc-checkin (files backend &optional comment initial-contents rev)
+(defvar vc-patch-string nil)
+
+(defun vc-checkin (files backend &optional comment initial-contents rev patch-string)
   "Check in FILES. COMMENT is a comment string; if omitted, a
 buffer is popped up to accept a comment.  If INITIAL-CONTENTS is
 non-nil, then COMMENT is used as the initial contents of the log
@@ -1643,7 +1655,9 @@ vc-checkin
        ;; vc-checkin-switches, but 'the' local buffer is
        ;; not a well-defined concept for filesets.
        (progn
-         (vc-call-backend backend 'checkin files comment rev)
+         (if patch-string
+             (vc-call-backend backend 'checkin-patch patch-string comment)
+           (vc-call-backend backend 'checkin files comment rev))
          (mapc #'vc-delete-automatic-version-backups files))
        `((vc-state . up-to-date)
          (vc-checkout-time . ,(file-attribute-modification-time
@@ -1651,7 +1665,8 @@ vc-checkin
          (vc-working-revision . nil)))
      (message "Checking in %s...done" (vc-delistify files)))
    'vc-checkin-hook
-   backend))
+   backend
+   patch-string))
 
 ;;; Additional entry points for examining version histories
 
@@ -1779,6 +1794,23 @@ vc-diff-finish
 (defvar vc-diff-added-files nil
   "If non-nil, diff added files by comparing them to /dev/null.")
 
+(defvar vc-patch-buffer nil)
+
+(defun vc-diff-patch ()
+  (let ((buffer "*vc-diff*")
+        (patch-string vc-patch-string))
+    (vc-setup-buffer buffer)
+    (set-buffer buffer)
+    (let ((buffer-undo-list t)
+          (inhibit-read-only t))
+      (insert patch-string))
+    (setq buffer-read-only t)
+    (diff-mode)
+    (setq-local diff-vc-backend (vc-responsible-backend default-directory))
+    (setq-local revert-buffer-function (lambda (_ _) (vc-diff-patch)))
+    (pop-to-buffer (current-buffer))
+    (vc-run-delayed (vc-diff-finish (current-buffer) nil))))
+
 (defun vc-diff-internal (async vc-fileset rev1 rev2 &optional verbose buffer)
   "Report diffs between two revisions of a fileset.
 Output goes to the buffer BUFFER, which defaults to *vc-diff*.

[-- Attachment #3: Type: text/plain, Size: 181 bytes --]


> Perhaps we should also check that there are no existing staged changes for
> those files, and if so, abort? This way we won't commit anything else by
> accident.

Will do later.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

On 21.08.2022 21:53, Juri Linkov wrote:
>> Another solution for making sure the current backend supports the new
>> feature is to add a new backend function. Call it 'checkin-patch' or
>> `checkin-diff`.
>>
>> vc-git-checkin and vc-git-checking patch can share some implementation, so
>> most of code duplication can be avoided.
>>
>> But when a backend does not support this, the user will see a more-or-less
>> understandable error like "not supported".
> 
> So a new patch adds a new backend function 'checkin-patch':

Looks pretty good. Thank you.

>> Perhaps we should also check that there are no existing staged changes for
>> those files, and if so, abort? This way we won't commit anything else by
>> accident.
> 
> Will do later.

Shouldn't be too hard, if we don't try to do it eagerly. Just add a 
check inside (when vc-git-patch-string ...) in vc-git-checkin.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

>>> Perhaps we should also check that there are no existing staged changes for
>>> those files, and if so, abort? This way we won't commit anything else by
>>> accident.
>> Will do later.
>
> Shouldn't be too hard, if we don't try to do it eagerly. Just add a check
> inside (when vc-git-patch-string ...) in vc-git-checkin.

This uses the same command `git diff --cached --quiet` as in commit-patch
that you mentioned earlier
https://github.com/caldwell/commit-patch/blob/master/commit-patch#L215-L217
(but with an error message shorter than "Cowardly refusing to do anything
potentially harmful":)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 8458b40714..fa09ecf472 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -944,6 +944,8 @@ vc-git-checkin
               (let ((default-directory (file-name-directory file1)))
                 (make-nearby-temp-file "git-msg")))))
     (when vc-git-patch-string
+      (unless (eq (vc-git-command nil t nil "diff" "--cached" "--quiet") 0)
+        (error "Index not empty"))
       (let ((patch-file (make-temp-file "git-patch")))
         (with-temp-file patch-file
           (insert vc-git-patch-string))

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

On 24.08.2022 21:20, Juri Linkov wrote:
>>>> Perhaps we should also check that there are no existing staged changes for
>>>> those files, and if so, abort? This way we won't commit anything else by
>>>> accident.
>>> Will do later.
>> Shouldn't be too hard, if we don't try to do it eagerly. Just add a check
>> inside (when vc-git-patch-string ...) in vc-git-checkin.
> This uses the same command `git diff --cached --quiet` as in commit-patch
> that you mentioned earlier
> https://github.com/caldwell/commit-patch/blob/master/commit-patch#L215-L217
> (but with an error message shorter than "Cowardly refusing to do anything
> potentially harmful":)
> 
> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index 8458b40714..fa09ecf472 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -944,6 +944,8 @@ vc-git-checkin
>                 (let ((default-directory (file-name-directory file1)))
>                   (make-nearby-temp-file "git-msg")))))
>       (when vc-git-patch-string
> +      (unless (eq (vc-git-command nil t nil "diff" "--cached" "--quiet") 0)
> +        (error "Index not empty"))

user-error, maybe?

>         (let ((patch-file (make-temp-file "git-patch")))
>           (with-temp-file patch-file
>             (insert vc-git-patch-string))

Looking great otherwise. Please install whenever you think it's ready.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

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

>> @@ -944,6 +944,8 @@ vc-git-checkin
>>                 (let ((default-directory (file-name-directory file1)))
>>                   (make-nearby-temp-file "git-msg")))))
>>       (when vc-git-patch-string
>> +      (unless (eq (vc-git-command nil t nil "diff" "--cached" "--quiet") 0)
>> +        (error "Index not empty"))
>
> user-error, maybe?

Will be fixed.

>>         (let ((patch-file (make-temp-file "git-patch")))
>>           (with-temp-file patch-file
>>             (insert vc-git-patch-string))
>
> Looking great otherwise. Please install whenever you think it's ready.

Not yet great :)  I tried to fix another long-standing problem
because its solution should also define the names of new functions here.

The problem is that currently using 'C-c C-d' (log-edit-show-diff)
in the *vc-log* buffer doesn't show the same diff that will be really
committed when different files were marked in *vc-dir* parent buffer
before typing 'C-c C-c' in *vc-log*.

This is because currently log-edit-diff-function is set to vc-diff
that is not suitable here since it tries to deduce fileset again
from *vc-dir*, but files to commit are set in the buffer-local
'vc-log-fileset' in *vc-log*.  So I added a new function
'log-edit-diff-fileset' that shows the diff from 'vc-log-fileset'.
Using the same naming scheme, I renamed 'vc-diff-patch' to
'log-edit-diff-patch'.

This shows the difference from the previous patch,
but later these changes could be committed separately:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: log-edit-diff-patch.patch --]
[-- Type: text/x-diff, Size: 3049 bytes --]

diff --git a/lisp/vc/log-edit.el b/lisp/vc/log-edit.el
index e958673fea..5290616302 100644
--- a/lisp/vc/log-edit.el
+++ b/lisp/vc/log-edit.el
@@ -664,6 +664,19 @@ log-edit-set-common-indentation
       (indent-rigidly (point) (point-max)
 		      (- log-edit-common-indent common)))))
 
+(defvar vc-patch-string)
+
+(autoload 'vc-diff-patch-string "vc")
+(defun log-edit-diff-patch ()
+  (vc-diff-patch-string vc-patch-string))
+
+(defvar vc-log-fileset)
+
+(defun log-edit-diff-fileset ()
+  "Display diffs for the files to be committed."
+  (interactive)
+  (vc-diff nil nil (list log-edit-vc-backend vc-log-fileset)))
+
 (defun log-edit-show-diff ()
   "Show the diff for the files to be committed."
   (interactive)
diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index 74e624999f..36a6f27891 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -656,7 +656,7 @@ vc-log-edit
                        (lambda (file) (file-relative-name file root))
                        fileset))))
 	      (log-edit-diff-function
-               . ,(if vc-patch-string 'vc-diff-patch 'vc-diff))
+               . ,(if vc-patch-string 'log-edit-diff-patch 'log-edit-diff-fileset))
 	      (log-edit-vc-backend . ,backend)
 	      (vc-log-fileset . ,fileset)
 	      (vc-patch-string . ,vc-patch-string))
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index cce692f7ea..51ae10ae5c 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1795,18 +1795,16 @@ vc-diff-added-files
 
 (defvar vc-patch-string nil)
 
-(defun vc-diff-patch ()
-  (let ((buffer "*vc-diff*")
-        (patch-string vc-patch-string))
+(defun vc-diff-patch-string (patch-string)
+  (let ((buffer "*vc-diff*"))
     (vc-setup-buffer buffer)
-    (set-buffer buffer)
     (let ((buffer-undo-list t)
           (inhibit-read-only t))
       (insert patch-string))
     (setq buffer-read-only t)
     (diff-mode)
     (setq-local diff-vc-backend (vc-responsible-backend default-directory))
-    (setq-local revert-buffer-function (lambda (_ _) (vc-diff-patch)))
+    (setq-local revert-buffer-function (lambda (_ _) (vc-diff-patch-string)))
     (setq-local vc-patch-string patch-string)
     (pop-to-buffer (current-buffer))
     (vc-run-delayed (vc-diff-finish (current-buffer) nil))))
@@ -2000,7 +1998,7 @@ vc-maybe-buffer-sync
     (when buffer-file-name (vc-buffer-sync not-urgent))))
 
 ;;;###autoload
-(defun vc-diff (&optional historic not-urgent)
+(defun vc-diff (&optional historic not-urgent fileset)
   "Display diffs between file revisions.
 Normally this compares the currently selected fileset with their
 working revisions.  With a prefix argument HISTORIC, it reads two revision
@@ -2012,7 +2010,7 @@ vc-diff
   (if historic
       (call-interactively 'vc-version-diff)
     (vc-maybe-buffer-sync not-urgent)
-    (let ((fileset (vc-deduce-fileset t)))
+    (let ((fileset (or fileset (vc-deduce-fileset t))))
       (vc-buffer-sync-fileset fileset not-urgent)
       (vc-diff-internal t fileset nil nil
 			(called-interactively-p 'interactive)))))

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

On 27.08.2022 23:07, Juri Linkov wrote:
>>> @@ -944,6 +944,8 @@ vc-git-checkin
>>>                  (let ((default-directory (file-name-directory file1)))
>>>                    (make-nearby-temp-file "git-msg")))))
>>>        (when vc-git-patch-string
>>> +      (unless (eq (vc-git-command nil t nil "diff" "--cached" "--quiet") 0)
>>> +        (error "Index not empty"))
>>
>> user-error, maybe?
> 
> Will be fixed.
> 
>>>          (let ((patch-file (make-temp-file "git-patch")))
>>>            (with-temp-file patch-file
>>>              (insert vc-git-patch-string))
>>
>> Looking great otherwise. Please install whenever you think it's ready.
> 
> Not yet great :)  I tried to fix another long-standing problem
> because its solution should also define the names of new functions here.
> 
> The problem is that currently using 'C-c C-d' (log-edit-show-diff)
> in the *vc-log* buffer doesn't show the same diff that will be really
> committed when different files were marked in *vc-dir* parent buffer
> before typing 'C-c C-c' in *vc-log*.
> 
> This is because currently log-edit-diff-function is set to vc-diff
> that is not suitable here since it tries to deduce fileset again
> from *vc-dir*, but files to commit are set in the buffer-local
> 'vc-log-fileset' in *vc-log*.  So I added a new function
> 'log-edit-diff-fileset' that shows the diff from 'vc-log-fileset'.
> Using the same naming scheme, I renamed 'vc-diff-patch' to
> 'log-edit-diff-patch'.
> 
> This shows the difference from the previous patch,
> but later these changes could be committed separately:

Makes perfect sense, thanks.

Honestly, I think it's a long-requested advanced feature which we'd be 
happy to have even if there were rough edges remaining (as long as it 
doesn't corrupt users' files or otherwise loses changes, of course). But 
this seems to be coming along nicely.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

close 52349 29.0.50
thanks

>> The problem is that currently using 'C-c C-d' (log-edit-show-diff)
>> in the *vc-log* buffer doesn't show the same diff that will be really
>> committed when different files were marked in *vc-dir* parent buffer
>> before typing 'C-c C-c' in *vc-log*.
>> This is because currently log-edit-diff-function is set to vc-diff
>> that is not suitable here since it tries to deduce fileset again
>> from *vc-dir*, but files to commit are set in the buffer-local
>> 'vc-log-fileset' in *vc-log*.  So I added a new function
>> 'log-edit-diff-fileset' that shows the diff from 'vc-log-fileset'.
>> Using the same naming scheme, I renamed 'vc-diff-patch' to
>> 'log-edit-diff-patch'.
>> This shows the difference from the previous patch,
>> but later these changes could be committed separately:
>
> Makes perfect sense, thanks.

So now pushed and closed.  If more changes are needed,
this could be reopened or a new bug report opened.

> Honestly, I think it's a long-requested advanced feature which we'd be
> happy to have even if there were rough edges remaining (as long as it
> doesn't corrupt users' files or otherwise loses changes, of course). But
> this seems to be coming along nicely.

'vc-finish-logentry' has such FIXME:

  (let ((logbuf (current-buffer))
	(log-operation vc-log-operation)
        ;; FIXME: When coming from VC-Dir, we should check that the
        ;; set of selected files is still equal to vc-log-fileset,
        ;; to avoid surprises.
	(log-fileset vc-log-fileset)
    (pop-to-buffer vc-parent-buffer)

I tried to fix this, but it requires duplicating too much code
from vc-next-action, including code that prepares 'ready-for-commit'
list of files.  I'm afraid that such check often will raise 
a false alarm.

OTOH, maybe this check is not needed since I fixed 'C-c C-d' above
to show the same diff that will be committed using vc-log-fileset,
and 'C-c C-c' uses the same vc-log-fileset as well.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

On 28.08.2022 22:45, Juri Linkov wrote:
> close 52349 29.0.50
> thanks
> 
>>> The problem is that currently using 'C-c C-d' (log-edit-show-diff)
>>> in the *vc-log* buffer doesn't show the same diff that will be really
>>> committed when different files were marked in *vc-dir* parent buffer
>>> before typing 'C-c C-c' in *vc-log*.
>>> This is because currently log-edit-diff-function is set to vc-diff
>>> that is not suitable here since it tries to deduce fileset again
>>> from *vc-dir*, but files to commit are set in the buffer-local
>>> 'vc-log-fileset' in *vc-log*.  So I added a new function
>>> 'log-edit-diff-fileset' that shows the diff from 'vc-log-fileset'.
>>> Using the same naming scheme, I renamed 'vc-diff-patch' to
>>> 'log-edit-diff-patch'.
>>> This shows the difference from the previous patch,
>>> but later these changes could be committed separately:
>>
>> Makes perfect sense, thanks.
> 
> So now pushed and closed.  If more changes are needed,
> this could be reopened or a new bug report opened.

Seems to be working well. Thank you!

>> Honestly, I think it's a long-requested advanced feature which we'd be
>> happy to have even if there were rough edges remaining (as long as it
>> doesn't corrupt users' files or otherwise loses changes, of course). But
>> this seems to be coming along nicely.
> 
> 'vc-finish-logentry' has such FIXME:
> 
>    (let ((logbuf (current-buffer))
> 	(log-operation vc-log-operation)
>          ;; FIXME: When coming from VC-Dir, we should check that the
>          ;; set of selected files is still equal to vc-log-fileset,
>          ;; to avoid surprises.
> 	(log-fileset vc-log-fileset)
>      (pop-to-buffer vc-parent-buffer)
> 
> I tried to fix this, but it requires duplicating too much code
> from vc-next-action, including code that prepares 'ready-for-commit'
> list of files.  I'm afraid that such check often will raise
> a false alarm.
> 
> OTOH, maybe this check is not needed since I fixed 'C-c C-d' above
> to show the same diff that will be committed using vc-log-fileset,
> and 'C-c C-c' uses the same vc-log-fileset as well.

Hopefully, yes.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

>>>>> Perhaps we should also check that there are no existing staged changes for
>>>>> those files, and if so, abort? This way we won't commit anything else by
>>>>> accident.
>>>> Will do later.
>>> Shouldn't be too hard, if we don't try to do it eagerly. Just add a check
>>> inside (when vc-git-patch-string ...) in vc-git-checkin.
>> +      (unless (eq (vc-git-command nil t nil "diff" "--cached" "--quiet") 0)
>> +        (error "Index not empty"))
>
> user-error, maybe?

Oh, it has a sad shortcoming: this feature can't be used when a new file
is added with 'C-x v v' since new files get added directly to the index,
and checking for the empty index raises the error for the whole changeset
with modified old files and new files.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Sean Whitton]

Hello,

On Thu 08 Sep 2022 at 10:29PM +03, Juri Linkov wrote:

>>>>>> Perhaps we should also check that there are no existing staged changes for
>>>>>> those files, and if so, abort? This way we won't commit anything else by
>>>>>> accident.
>>>>> Will do later.
>>>> Shouldn't be too hard, if we don't try to do it eagerly. Just add a check
>>>> inside (when vc-git-patch-string ...) in vc-git-checkin.
>>> +      (unless (eq (vc-git-command nil t nil "diff" "--cached" "--quiet") 0)
>>> +        (error "Index not empty"))
>>
>> user-error, maybe?
>
> Oh, it has a sad shortcoming: this feature can't be used when a new file
> is added with 'C-x v v' since new files get added directly to the index,
> and checking for the empty index raises the error for the whole changeset
> with modified old files and new files.

VC is a bit strange with adding and deleting files with Git -- doing so
stages the additions or deletions, whereas in every other case VC
doesn't use the staging area at all, except as an implementation detail
when actually committing.  So when you add or delete files you end up in
a sort of transient state that you aren't otherwise in.

What I've been doing is immediately committing any additions or
deletions and then preparing --amend commits with the changes to
existing files, using your new feature.

Would it be too disruptive to make "registering" a new file with Git a
no-op, and then require the user to commit it by going to VC Dir and
marking it?  That's the only alternative I can think of, given that it's
common to have lots of untracked, unignored files around you don't
intend ever to commit.

-- 
Sean Whitton

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

On 09.09.2022 01:48, Sean Whitton wrote:
> Would it be too disruptive to make "registering" a new file with Git a
> no-op, and then require the user to commit it by going to VC Dir and
> marking it?  That's the only alternative I can think of, given that it's
> common to have lots of untracked, unignored files around you don't
> intend ever to commit.

I suppose we could use a more complex check in Git's case: if the diff 
is non-empty, check that it doesn't intersect with the diff we want to 
apply, or where it does, the contents are exactly the same. It's more 
complex than the existing one-liner, though.

The commit-and-amend thick is the first that came to my mind too, but 
the above would be more flexible, I guess.

And as for dropping the "registering" step in Git, I suppose that might 
speed up the overall workflow for some people (myself included, 
probably), but it still wouldn't allow you to check in new files 
together with a partial diff in existing one. I think.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

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

>> Would it be too disruptive to make "registering" a new file with Git a
>> no-op, and then require the user to commit it by going to VC Dir and
>> marking it?  That's the only alternative I can think of, given that it's
>> common to have lots of untracked, unignored files around you don't
>> intend ever to commit.
>
> I suppose we could use a more complex check in Git's case: if the diff is
> non-empty, check that it doesn't intersect with the diff we want to apply,
> or where it does, the contents are exactly the same. It's more complex than
> the existing one-liner, though.

Maybe like this?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-git-checkin-diff-cached.patch --]
[-- Type: text/x-diff, Size: 1180 bytes --]

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 2941cc75be..792981b142 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1018,7 +1018,20 @@ vc-git-checkin
                 (make-nearby-temp-file "git-msg")))))
     (when vc-git-patch-string
       (unless (zerop (vc-git-command nil t nil "diff" "--cached" "--quiet"))
-        (user-error "Index not empty"))
+        (with-temp-buffer
+          (vc-git-command (current-buffer) t nil "diff" "--cached")
+          (goto-char (point-min))
+          (let ((pos (point)) file-diff)
+            (forward-line 1)
+            (while (not (eobp))
+              (search-forward "diff --git" nil 'move)
+              (move-beginning-of-line 1)
+              (setq file-diff (buffer-substring pos (point)))
+              (if (string-search file-diff vc-git-patch-string)
+                  (setq vc-git-patch-string
+                        (string-replace file-diff "" vc-git-patch-string))
+                (user-error "Index not empty"))
+              (setq pos (point))))))
       (let ((patch-file (make-temp-file "git-patch")))
         (with-temp-file patch-file
           (insert vc-git-patch-string))

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

On 11.09.2022 18:05, Juri Linkov wrote:
> +              (if (string-search file-diff vc-git-patch-string)
> +                  (setq vc-git-patch-string
> +                        (string-replace file-diff "" vc-git-patch-string))

Not sure I understand this part. We're replacing some text (the diff 
file header) with empty text in the patch string? But not the whole hunk?

It might work if we removed the whole hunk as well, but we need to make 
sure that the staged contents for that file are exactly the same as what 
is contained for that file in vc-git-patch-string.

Perhaps if we called diff-file-next in addition to 
(move-beginning-of-line 1)? But we need to save both strings: and ensure 
that if there is a match for that file header, then all hunks are 
contained in the patch as well (and nothing extra, for that file).

It's complex logic, so if you manage to write a test as well, that would 
be excellent.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

>> +              (if (string-search file-diff vc-git-patch-string)
>> +                  (setq vc-git-patch-string
>> +                        (string-replace file-diff "" vc-git-patch-string))
>
> Not sure I understand this part. We're replacing some text (the diff file
> header) with empty text in the patch string? But not the whole hunk?

Actually it replaces the whole new/deleted file differences.

> It might work if we removed the whole hunk as well, but we need to make
> sure that the staged contents for that file are exactly the same as what is
> contained for that file in vc-git-patch-string.

This is exactly what this patch does.

> Perhaps if we called diff-file-next in addition to (move-beginning-of-line
> 1)?

diff-file-next doesn't work: it stops in the middle of the diff file header.
Therefore the patch navigates git diff file headers explicitly.

> But we need to save both strings: and ensure that if there is a match
> for that file header, then all hunks are contained in the patch as well
> (and nothing extra, for that file).

This is implemented by the patch.  Maybe it could help you to see how it works
by running it under debugger and stepping through it.

> It's complex logic, so if you manage to write a test as well, that would be
> excellent.

A test could written when someone will create infrastructure for testing
git commands with helpers to create a git repository and checking its content.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

On 12.09.2022 21:19, Juri Linkov wrote:
>>> +              (if (string-search file-diff vc-git-patch-string)
>>> +                  (setq vc-git-patch-string
>>> +                        (string-replace file-diff "" vc-git-patch-string))
>>
>> Not sure I understand this part. We're replacing some text (the diff file
>> header) with empty text in the patch string? But not the whole hunk?
> 
> Actually it replaces the whole new/deleted file differences.
> 
>> It might work if we removed the whole hunk as well, but we need to make
>> sure that the staged contents for that file are exactly the same as what is
>> contained for that file in vc-git-patch-string.
> 
> This is exactly what this patch does.

Right, sorry, I see it now.

Do you know if Git use a stable ordering for files?

If not, here's where the proposed implementation might fail:

Suppose we have the staging area with contents

a/bar b/bar
+bbb
a/foo b/foo
+aaa
+ccc

and the diff to check in with contents

a/foo b/foo
+aaa
a/bar b/bar
+bbb
+ccc

...then the check will succeed, I think.

Even if Git always sorts them the same, I suppose an externally-produced 
diff could trigger this scenario. It should be a pretty rare case, 
though, so it's probably fine.

A tweak like the following could fix it, though: instead of replacing 
the chunks with "", maybe keep the file headers. Then the remaining 
contents for the same file in vc-git-patch-string wouldn't be able to 
stick to the previous file's chunks.

>> Perhaps if we called diff-file-next in addition to (move-beginning-of-line
>> 1)?
> 
> diff-file-next doesn't work: it stops in the middle of the diff file header.
> Therefore the patch navigates git diff file headers explicitly.
> 
>> But we need to save both strings: and ensure that if there is a match
>> for that file header, then all hunks are contained in the patch as well
>> (and nothing extra, for that file).
> 
> This is implemented by the patch.  Maybe it could help you to see how it works
> by running it under debugger and stepping through it.
> 
>> It's complex logic, so if you manage to write a test as well, that would be
>> excellent.
> 
> A test could written when someone will create infrastructure for testing
> git commands with helpers to create a git repository and checking its content.

test/lisp/vc/vc-tests.el actually contains a helper like this.

Every scenario starts with calling vc-test--create-repo-function, and 
there are tests for 'version-diff' at the very end of the file. It's 
somewhat convoluted, so I don't blame you for missing it.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

> Do you know if Git use a stable ordering for files?

It seems the ordering is stable unless it's changed explicitly
by command line options.

> If not, here's where the proposed implementation might fail:
>
> Suppose we have the staging area with contents
>
> a/bar b/bar
> +bbb
> a/foo b/foo
> +aaa
> +ccc
>
> and the diff to check in with contents
>
> a/foo b/foo
> +aaa
> a/bar b/bar
> +bbb
> +ccc
>
> ...then the check will succeed, I think.

This check is intended to detect only added/deleted files
that get into the staging area.

> Even if Git always sorts them the same, I suppose an externally-produced
> diff could trigger this scenario. It should be a pretty rare case, though,
> so it's probably fine.
>
> A tweak like the following could fix it, though: instead of replacing the
> chunks with "", maybe keep the file headers. Then the remaining contents
> for the same file in vc-git-patch-string wouldn't be able to stick to the
> previous file's chunks.

This looks too complicated.  And indeed, this is a rare case, so maybe
something like this could be added when this will became a real problem.

>>> Perhaps if we called diff-file-next in addition to (move-beginning-of-line
>>> 1)?
>> diff-file-next doesn't work: it stops in the middle of the diff file
>> header.
>> Therefore the patch navigates git diff file headers explicitly.
>>
>>> But we need to save both strings: and ensure that if there is a match
>>> for that file header, then all hunks are contained in the patch as well
>>> (and nothing extra, for that file).
>> This is implemented by the patch.  Maybe it could help you to see how it
>> works
>> by running it under debugger and stepping through it.
>>
>>> It's complex logic, so if you manage to write a test as well, that would be
>>> excellent.
>> A test could written when someone will create infrastructure for testing
>> git commands with helpers to create a git repository and checking its content.
>
> test/lisp/vc/vc-tests.el actually contains a helper like this.
>
> Every scenario starts with calling vc-test--create-repo-function, and there
> are tests for 'version-diff' at the very end of the file. It's somewhat
> convoluted, so I don't blame you for missing it.

Actually, I looked into test/lisp/vc/vc-git-tests.el that is almost empty.
I expected that since this check is git-specific, it should be in
vc-git-tests.el.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

On 19.09.2022 09:50, Juri Linkov wrote:
>> Do you know if Git use a stable ordering for files?
> 
> It seems the ordering is stable unless it's changed explicitly
> by command line options.
> 
>> If not, here's where the proposed implementation might fail:
>>
>> Suppose we have the staging area with contents
>>
>> a/bar b/bar
>> +bbb
>> a/foo b/foo
>> +aaa
>> +ccc
>>
>> and the diff to check in with contents
>>
>> a/foo b/foo
>> +aaa
>> a/bar b/bar
>> +bbb
>> +ccc
>>
>> ...then the check will succeed, I think.
> 
> This check is intended to detect only added/deleted files
> that get into the staging area.

It doesn't seem like it's going to differentiate between "add whole 
file" chunks and any other kinds of chunks.

Which is not a bad thing by itself, probably. But can increase the odds 
of something like the above happening.

>> Even if Git always sorts them the same, I suppose an externally-produced
>> diff could trigger this scenario. It should be a pretty rare case, though,
>> so it's probably fine.
>>
>> A tweak like the following could fix it, though: instead of replacing the
>> chunks with "", maybe keep the file headers. Then the remaining contents
>> for the same file in vc-git-patch-string wouldn't be able to stick to the
>> previous file's chunks.
> 
> This looks too complicated.  And indeed, this is a rare case, so maybe
> something like this could be added when this will became a real problem.

Sure, sounds good to me.

>>>> It's complex logic, so if you manage to write a test as well, that would be
>>>> excellent.
>>> A test could written when someone will create infrastructure for testing
>>> git commands with helpers to create a git repository and checking its content.
>>
>> test/lisp/vc/vc-tests.el actually contains a helper like this.
>>
>> Every scenario starts with calling vc-test--create-repo-function, and there
>> are tests for 'version-diff' at the very end of the file. It's somewhat
>> convoluted, so I don't blame you for missing it.
> 
> Actually, I looked into test/lisp/vc/vc-git-tests.el that is almost empty.
> I expected that since this check is git-specific, it should be in
> vc-git-tests.el.

Not sure how to best share the setup/teardown logic between vc-tests.el 
and vc-git-test.el.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

> It doesn't seem like it's going to differentiate between "add whole file"
> chunks and any other kinds of chunks.
>
> Which is not a bad thing by itself, probably. But can increase the odds of
> something like the above happening.

I added more checks to detect the whole files boundaries, and pushed.

>>>>> It's complex logic, so if you manage to write a test as well, that would be
>>>>> excellent.
>>>> A test could written when someone will create infrastructure for testing
>>>> git commands with helpers to create a git repository and checking its content.
>>>
>>> test/lisp/vc/vc-tests.el actually contains a helper like this.
>>>
>>> Every scenario starts with calling vc-test--create-repo-function, and there
>>> are tests for 'version-diff' at the very end of the file. It's somewhat
>>> convoluted, so I don't blame you for missing it.
>> Actually, I looked into test/lisp/vc/vc-git-tests.el that is almost
>> empty.
>> I expected that since this check is git-specific, it should be in
>> vc-git-tests.el.
>
> Not sure how to best share the setup/teardown logic between vc-tests.el and
> vc-git-test.el.

I looked into this, but this is a too big task.  It would be nice if
Someone (TM) created the git-specific setup/teardown logic, and other
helper functions in test/lisp/vc/vc-git-tests.el.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov]

(Sorry, resending after unarchiving).

On 02.10.2022 21:51, Juri Linkov wrote:
>> It doesn't seem like it's going to differentiate between "add whole file"
>> chunks and any other kinds of chunks.
>>
>> Which is not a bad thing by itself, probably. But can increase the odds of
>> something like the above happening.
> I added more checks to detect the whole files boundaries, and pushed.

Thanks.

>>>>>> It's complex logic, so if you manage to write a test as well, that would be
>>>>>> excellent.
>>>>> A test could written when someone will create infrastructure for testing
>>>>> git commands with helpers to create a git repository and checking its content.
>>>> test/lisp/vc/vc-tests.el actually contains a helper like this.
>>>>
>>>> Every scenario starts with calling vc-test--create-repo-function, and there
>>>> are tests for 'version-diff' at the very end of the file. It's somewhat
>>>> convoluted, so I don't blame you for missing it.
>>> Actually, I looked into test/lisp/vc/vc-git-tests.el that is almost
>>> empty.
>>> I expected that since this check is git-specific, it should be in
>>> vc-git-tests.el.
>> Not sure how to best share the setup/teardown logic between vc-tests.el and
>> vc-git-test.el.
> I looked into this, but this is a too big task.  It would be nice if
> Someone (TM) created the git-specific setup/teardown logic, and other
> helper functions in test/lisp/vc/vc-git-tests.el.

FWIW, now that the feature is supported across backends, we can add a 
corresponding test in vc-tests.el. ;-)

Some backends will probably need to be skipped anyway, but the most 
popular ones will hopefully work.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Filipp Gunbin]

On 21/08/2022 21:53 +0300, Juri Linkov wrote:

A minor comment:

> +(defun vc-diff-patch ()
> +  (let ((buffer "*vc-diff*")
> +        (patch-string vc-patch-string))
> +    (vc-setup-buffer buffer)
> +    (set-buffer buffer)

vc-setup-buffer already did set-buffer?

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Juri Linkov]

> A minor comment:
>
>> +(defun vc-diff-patch ()
>> +  (let ((buffer "*vc-diff*")
>> +        (patch-string vc-patch-string))
>> +    (vc-setup-buffer buffer)
>> +    (set-buffer buffer)
>
> vc-setup-buffer already did set-buffer?

Thanks for noticing, will be fixed in the next version.
This is because vc-diff-patch tries to duplicate code
from vc-diff-internal since the same function can't
be reused for vc-diff-patch.

bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Lars Ingebrigtsen]

Manuel Uberti via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> I feel like it'd be a nice improvement to VC, what do you think?

Yes, it'd be a very welcome feature.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no