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

From: Dmitry Gutov <[email-masked]>
Date: Sat, 12 Feb 2022 03:43:49 +0200

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.