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

[Dmitry Gutov <dgutov@yandex.ru>]

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.