Re: master 4803fba487 1/2: 'C-x v v' on a diff buffer commits it as a patch (bug#52349)

[Dmitry Gutov <dgutov@yandex.ru>]

On 30.08.2022 14:25, Eli Zaretskii wrote:
>> Date: Mon, 29 Aug 2022 23:36:32 +0300
>> Cc: larsi@gnus.org, juri@jurta.org, emacs-devel@gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>>
>>> The implementation in vc-git.el does this, AFAICT:
>>>
>>>     . creates a temporary file
>>>     . inserts the patch into the temporary file
>>
>> Note that "the patch" will usually be a subset of the current 'git diff'
>> output. But not always (the patch can be additionally modified by hand).
> 
> I understand.  But how is this significant?  Doesn't "git apply"
> require a valid patch as well?

It's significant in that the starting conditions is that the files are 
already modified compared to the repository head, and the end goal is to 
be able to pick only some of those changes for commit. To partition a 
complex change into several commits, usually. Or to simply keep 
unfinished changes separate without resorting to 'git stash'. (**)

>>>     . calls "git apply --cached" on the patch in the temporary file
>>>     . deletes the temporary file
>>>
>>> This doesn't seem to be very different from invoking Patch on the
>>> diffs.  (And using shell-command-on-region avoids the need to
>>> explicitly create a temporary file.)
>>
>> 'patch < file' edits the working directory. 'git apply --cached' edits
>> the index, keeping the files themselves unchanged.
> 
> I'm not sure why is this significant.  Doesn't committing the patch
> from the index eventually update the files on disk as well?

Nope.

> And if
> not, how is this feature useful? you get a repository where the
> contents of the .git/ directory doesn't match the worktree, right?
> When is this needed or useful?

See above. The two sentences near near the (**) sign.

You can also commit a patch with changes that aren't present in the 
files and then 'vc-revert' in them. But doing that kind of thing wasn't 
particularly difficult even before this feature landed IMHO.

>>>> Personally I hope we discover some popular extension to Mercurial which
>>>> we'll be able to use in the same way as we do Git's index area here. And
>>>> then say job well done and keep the less-popular and outdated backends
>>>> unsupported.
>>>
>>> The index thing being the problem because Git needs to have the
>>> changes in the index before you can commit?  Or is there any other
>>> reason?
>>
>> Index is a solution, not the problem. The problem is other changes being
>> present on disk.
> 
> For other changes present on disk, we cam:
> 
>    . use the equivalent of the "stash" command, where the VCS supports it
>    . otherwise, commit specific files by name, where the VCS supports that
>    . otherwise signal an error, asking the user to clean up the tree

Like I said, I'm worried about the resulting differences in behavior. 
That will make it much harder for us to document the new feature 
properly, and the users who work with different backends from time to 
time are likely to stumble over those differences.

So far using /tmp and copying seems like a better approach.

>> I suppose we could implement a scaled-down version of this feature as a
>> fallback (the one that ensures that there are no existing changes in
>> affected files), but I fear the difference between the backends would
>> trip up the users.
> 
> AFAICT, Mercurial, Bazaar, RCS, CVS, and SVN all support committing an
> explicit list of files, so dirty working tree shouldn't be a problem
> for them.  The first two also have a "stash"-like functionality.

Not dirty working tree. "Dirty" specific files which we want to commit 
certain changes in.

> But even if we initially only support this in otherwise clean trees,
> it will be a huge advantage vs the current situation, where this
> feature is simply not available.

Will the advantage be significant? I really have to ask, because IME 
applying an external patch and them committing the files as a whole 
never felt particularly painful before (aside from resolving any 
conflicts or corrupted whitespace, which the new feature doesn't help 
with). And 'diff-mode' has had the 'diff-apply-hunk' command for ages -- 
I use it often.

OTOH, the lack of support for workflow corresponding to 'git add -p' has 
been complained about frequently, it's one of the reasons people use 
Magit, and I felt it as well many times.

To improve your workflow, though, might it help to add a new command 
'diff-apply-buffer'?