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

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

[Sean Whitton]

Hello,

On Sun 28 Aug 2022 at 03:45PM -04, Juri Linkov wrote:

>
> branch: master
> commit 4803fba487d41f0817feab48b5095ef4b4940ff6
> Author: Juri Linkov <juri@linkov.net>
> Commit: Juri Linkov <juri@linkov.net>
>
>     'C-x v v' on a diff buffer commits it as a patch (bug#52349)

Very cool!  Thank you!

Lots of different workflows for constructing the diff are enabled by
doing it in this generalised way.

-- 
Sean Whitton

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

[Manuel Uberti]

On 28/08/22 23:18, Sean Whitton wrote:
> Hello,
> 
> On Sun 28 Aug 2022 at 03:45PM -04, Juri Linkov wrote:
> 
>>
>> branch: master
>> commit 4803fba487d41f0817feab48b5095ef4b4940ff6
>> Author: Juri Linkov <juri@linkov.net>
>> Commit: Juri Linkov <juri@linkov.net>
>>
>>      'C-x v v' on a diff buffer commits it as a patch (bug#52349)
> 
> Very cool!  Thank you!
> 
> Lots of different workflows for constructing the diff are enabled by
> doing it in this generalised way.
> 

Indeed, thank you for this.

-- 
Manuel Uberti
https://manueluberti.eu

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

[Lars Ingebrigtsen]

Juri Linkov <juri@jurta.org> writes:

>     'C-x v v' on a diff buffer commits it as a patch (bug#52349)

Excellent!  This is going to save me a lot of work in the future.

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

[Daniel Martín]

Sean Whitton <spwhitton@spwhitton.name> writes:

> Hello,
>
> On Sun 28 Aug 2022 at 03:45PM -04, Juri Linkov wrote:
>
>>
>> branch: master
>> commit 4803fba487d41f0817feab48b5095ef4b4940ff6
>> Author: Juri Linkov <juri@linkov.net>
>> Commit: Juri Linkov <juri@linkov.net>
>>
>>     'C-x v v' on a diff buffer commits it as a patch (bug#52349)
>
> Very cool!  Thank you!
>
> Lots of different workflows for constructing the diff are enabled by
> doing it in this generalised way.

+1.  Thank you for working on this.

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Mon, 29 Aug 2022 16:48:29 +0200
> 
> Juri Linkov <juri@jurta.org> writes:
> 
> >     'C-x v v' on a diff buffer commits it as a patch (bug#52349)
> 
> Excellent!  This is going to save me a lot of work in the future.

Bother: This is only supported for Git, which is against the spirit of
VC, and definitely against the spirit of "C-x v v".

Why cannot this be implemented for every other VCS?  Given diffs in a
buffer, it should be as simple as running the Patch utility via
shell-command-on-region, then committing the results.  Git has a
special command for that, but we don't need a special command for
other VCSes, if they don't have the equivalent of "git apply".  (I'm
guessing that "git apply" simply runs Patch under the hood.)

Can we please implement this for other VCSes as well?

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

[Dmitry Gutov]

On 29.08.2022 19:46, Eli Zaretskii wrote:
> Why cannot this be implemented for every other VCS?

It can.

> Given diffs in a
> buffer, it should be as simple as running the Patch utility via
> shell-command-on-region, then committing the results.  Git has a
> special command for that, but we don't need a special command for
> other VCSes, if they don't have the equivalent of "git apply".  (I'm
> guessing that "git apply" simply runs Patch under the hood.)

No, 'git apply' puts the patch in a different place (index area), which 
means our implementation doesn't need to bother with moving all existing 
changes in the selected files somewhere else, then committing, and then 
restoring the previously-hidden changes.

The best way to implement the latter is not obvious to me. I suppose 
simply copying the files to /tmp and then overwriting them back at the 
end might do the trick, but can cause problems if the user edits the 
file buffer between creating the diff and finishing the checkin.

 > Can we please implement this for other VCSes as well?

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.

'man hg' didn't give me enough clues, though.

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

[Eli Zaretskii]

> Date: Mon, 29 Aug 2022 20:05:10 +0300
> Cc: juri@jurta.org, emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> > Given diffs in a
> > buffer, it should be as simple as running the Patch utility via
> > shell-command-on-region, then committing the results.  Git has a
> > special command for that, but we don't need a special command for
> > other VCSes, if they don't have the equivalent of "git apply".  (I'm
> > guessing that "git apply" simply runs Patch under the hood.)
> 
> No, 'git apply' puts the patch in a different place (index area), which 
> means our implementation doesn't need to bother with moving all existing 
> changes in the selected files somewhere else, then committing, and then 
> restoring the previously-hidden changes.

I don't follow, sorry.

The implementation in vc-git.el does this, AFAICT:

  . creates a temporary file
  . inserts the patch into the temporary file
  . 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.)

> 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?

IOW, why cannot we simply patch the files, and then run the equivalent
of vc-checkin?

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

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

>> No, 'git apply' puts the patch in a different place (index area),
>> which means our implementation doesn't need to bother with moving all
>> existing changes in the selected files somewhere else, then
>> committing, and then restoring the previously-hidden changes.
>
> I don't follow, sorry.
>
> The implementation in vc-git.el does this, AFAICT:
>
>   . creates a temporary file
>   . inserts the patch into the temporary file
>   . 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.

I think the difference is that "git apply --cached foo.patch" exits
non-zero if the index isn't empty and vc-git-checkin errors when it's
not.  That ensures the commit will include only the changes in the patch
and nothing else.

I guess other VCS impls could just test if the checkout is completely
unmodified and signal an error otherwise.  That would be a bit more
restrictive but still useful.

>> 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?

I think the git benefit is that you can have a completely dirty checkout
(hundreds of modified files) but the command will still work fine as
long as you haven't staged other changes yet.

> IOW, why cannot we simply patch the files, and then run the equivalent
> of vc-checkin?

See above, I think we can at least after a "is the checkout clean?"
check.

Bye,
Tassilo

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

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: Dmitry Gutov <dgutov@yandex.ru>, larsi@gnus.org, juri@jurta.org,
>  emacs-devel@gnu.org
> Date: Mon, 29 Aug 2022 21:03:50 +0200
> 
> I think the difference is that "git apply --cached foo.patch" exits
> non-zero if the index isn't empty and vc-git-checkin errors when it's
> not.  That ensures the commit will include only the changes in the patch
> and nothing else.
> 
> I guess other VCS impls could just test if the checkout is completely
> unmodified and signal an error otherwise.

Either that, or compare the list of modified files with the list of
patched files produced by Patch.

> That would be a bit more restrictive but still useful.

Yes, indeed.

> >> 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?
> 
> I think the git benefit is that you can have a completely dirty checkout
> (hundreds of modified files) but the command will still work fine as
> long as you haven't staged other changes yet.

It is perfectly fine to have the Git implementation be more flexible
and convenient than with other VCSes, which don't support the same
granularity/precision.  It would still be much better than having no
implementation at all for the other VCSes.

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

[Dmitry Gutov]

On 29.08.2022 20:41, Eli Zaretskii wrote:
>> Date: Mon, 29 Aug 2022 20:05:10 +0300
>> Cc: juri@jurta.org, emacs-devel@gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>>
>>> Given diffs in a
>>> buffer, it should be as simple as running the Patch utility via
>>> shell-command-on-region, then committing the results.  Git has a
>>> special command for that, but we don't need a special command for
>>> other VCSes, if they don't have the equivalent of "git apply".  (I'm
>>> guessing that "git apply" simply runs Patch under the hood.)
>>
>> No, 'git apply' puts the patch in a different place (index area), which
>> means our implementation doesn't need to bother with moving all existing
>> changes in the selected files somewhere else, then committing, and then
>> restoring the previously-hidden changes.
> 
> I don't follow, sorry.
> 
> 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).

This feature is a replacement for the popular workflow 'git add -p; git 
commit', not for simply committing a patch, which is pretty easy to do 
already.

Though the beauty of the current approach with Git is that it supports 
either.

>    . 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.

>> 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.

> IOW, why cannot we simply patch the files, and then run the equivalent
> of vc-checkin?

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.

So we should start with trying to implement the "full" version first: 
for individual backends or (harder) in general.

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

[Juri Linkov]

>> Can we please implement this for other VCSes as well?
>
> 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.
>
> 'man hg' didn't give me enough clues, though.

I am unfamiliar with Mercurial, but looking at
https://www.mercurial-scm.org/wiki/GitConcepts#Git.27s_staging_area
where the table says that index/staging area is not necessary in Mercurial
and suggests to use shelve instead that corresponds to Git's stash,
it would be possible to do the same with Hg as we first tried to do
with Git using stash: put changes to stash/shelve, commit and restore
the previous state.  In older VCSes that have neither index/staging area
nor stash/shelve, working area should be cleared by temporarily moving
changed files somewhere.

Anyway, the main point is that adepts of any existing VCS or any future
VCS can easily adapt their VCSes for this feature by just implementing
the backend call 'checkin-patch'.

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

[Eli Zaretskii]

> 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?

> This feature is a replacement for the popular workflow 'git add -p; git 
> commit', not for simply committing a patch, which is pretty easy to do 
> already.

Yes, I understand that.

> >    . 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?  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?

> >> 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

> 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.

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.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@jurta.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Lars Ingebrigtsen <larsi@gnus.org>,
>   emacs-devel@gnu.org
> Date: Tue, 30 Aug 2022 10:20:10 +0300
> 
> Anyway, the main point is that adepts of any existing VCS or any future
> VCS can easily adapt their VCSes for this feature by just implementing
> the backend call 'checkin-patch'.

I very much hope this is not going to be the official policy of VC
development in Emacs.  VC should IMO remain VCS-agnostic in its most
important features, and "C-x v v" is definitely such a feature of VC.
That's basically the only real raison d'être of VC nowadays, because
otherwise let's just obsolete VC and switch to Magit.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@jurta.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Lars Ingebrigtsen <larsi@gnus.org>,
>  emacs-devel@gnu.org
> Date: Tue, 30 Aug 2022 10:20:10 +0300
> 
> In older VCSes that have neither index/staging area nor
> stash/shelve, working area should be cleared by temporarily moving
> changed files somewhere.

There should be no need to move anything, as those older VCSes (and
the new ones as well) can accept a list of files to commit in their
commit command.

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

[Dmitry Gutov]

On 30.08.2022 10:20, Juri Linkov wrote:
> I am unfamiliar with Mercurial, but looking at
> https://www.mercurial-scm.org/wiki/GitConcepts#Git.27s_staging_area
> where the table says that index/staging area is not necessary in Mercurial
> and suggests to use shelve instead that corresponds to Git's stash,
> it would be possible to do the same with Hg as we first tried to do
> with Git using stash: put changes to stash/shelve, commit and restore
> the previous state.

I suppose it might work, but then what happens when stashed/shelved 
changes start to conflict with the ones just committed? The stash can 
pop up with conflict markers.

Maybe that's okay, I don't know.

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

[Dmitry Gutov]

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'?

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

[Eli Zaretskii]

> Date: Tue, 30 Aug 2022 15:35:31 +0300
> Cc: larsi@gnus.org, juri@jurta.org, emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> >> 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.

Ouch!  Then this feature will be useless for me, and I'm sorry I
wasted everyone's time based on the description in NEWS, which doesn't
make a point of emphasizing this basic assumption.

> 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.

I cannot agree that this feature is a reasonable solution for the
"git add -p" workflow.  It is not interactive enough for that.

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

[Alfred M. Szmidt]

   > >     'C-x v v' on a diff buffer commits it as a patch (bug#52349)
   > 
   > Excellent!  This is going to save me a lot of work in the future.

   Bother: This is only supported for Git, which is against the spirit of
   VC, and definitely against the spirit of "C-x v v".

This is a pitty indeed.  Specially seeing that it is easy enough to
get this working for all VCSs.  Here is a hack I wrote ages ago, and
this works for just about everything, it doesn't do exactly what is
needed (e.g, this works on a single file basis) but it isn't too hard
to get that working.

===File ~/diff-commit-hunk.el===============================
;;;; diff-commit-hunk.el --- commit parts of a hunk in `diff-mode'

(require 'diff-mode)

(defun current-buffer-file-name ()
  (buffer-file-name (current-buffer)))

(defun restore-source-file ()
  (with-current-buffer (current-buffer)
    (erase-buffer)
    (insert-buffer "*diff-commit-hunk*")
    (write-file (current-buffer-file-name)))
  (remove-hook 'vc-checkin-hook 'restore-source-file))

(defmacro with-original-file (&rest body)
  "Reverts associated source file temporarily in a `diff-mode'
buffer to the latest found in VCS, executes BODY and commits the
changes back VCS."
  `(progn
     (save-excursion
       (diff-goto-source)
       (let ((buf (current-buffer)))
	 (with-current-buffer (get-buffer-create "*diff-commit-hunk*")
	   (erase-buffer)
	   (insert-buffer buf)))
       (vc-revert-file (current-buffer-file-name)))
     ,@body
     (save-excursion
       (diff-goto-source)
       (write-file (current-buffer-file-name))
       (add-hook 'vc-checkin-hook 'restore-source-file)
       (vc-checkin (list (current-buffer-file-name))
		   (vc-backend (current-buffer-file-name))))))

;;;###autoload
(defun diff-commit-hunk ()
  "Revert associated source file to the latest version from VCS,
and apply (and commit) current hunk."
  (interactive)
  (with-original-file
   (let ((diff-advance-after-apply-hunk nil))
     (diff-apply-hunk))))

;;;###autoload
(defun diff-commit-all ()
  "Like `diff-commit-hunk', but applies all hunks in the current
diff buffer."
  (interactive)
  (with-original-file
   (goto-char (point-min))
   (diff-hunk-next)			;Skip header.
   (while (not (eobp))
     (diff-apply-hunk))))

(define-key diff-mode-map (kbd "s-a") #'diff-commit-hunk)
(define-key diff-mode-map (kbd "H-a") #'diff-commit-all)

;;;; diff-commit-hunk.el ends here.
============================================================

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

[Po Lu]

"Alfred M. Szmidt" <ams@gnu.org> writes:

> This is a pitty indeed.  Specially seeing that it is easy enough to
> get this working for all VCSs.  Here is a hack I wrote ages ago, and
> this works for just about everything, it doesn't do exactly what is
> needed (e.g, this works on a single file basis) but it isn't too hard
> to get that working.

If you have time, could you work on integrating that code with VC?

I would, but currently I'm heavily occupied with some other code (which
happens to use Subversion), and the feature would be quite convenient.

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

[Dmitry Gutov]

On 30.08.2022 16:01, Eli Zaretskii wrote:
>> Date: Tue, 30 Aug 2022 15:35:31 +0300
>> Cc: larsi@gnus.org, juri@jurta.org, emacs-devel@gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>>
>>>> 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.
> 
> Ouch!  Then this feature will be useless for me, and I'm sorry I
> wasted everyone's time based on the description in NEWS, which doesn't
> make a point of emphasizing this basic assumption.

Perhaps we could make it more useful for you as well, e.g. by applying 
all the changes from the patch first when called with 'C-u'.

I guess the problem is that the first step is not so easily automated: 
at least in my experience 'diff-apply-hunk' will either fail to find the 
files to patch (and prompt me to find them manually), or fail because of 
whitespace problems, or because it was made against some previous 
version of the code, and now has conflicts. And after I resolve all of 
this by hand, the files now have all the changes applied, and we're in 
the situation where 'C-x v D' followed by the new 'C-x v v' in diff-mode 
works just fine.

But I suppose if your patches apply cleanly most of the time, it can be 
less of a problem.

>> 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.
> 
> I cannot agree that this feature is a reasonable solution for the
> "git add -p" workflow.  It is not interactive enough for that.

It's... a more power-user approach to the same, with a different UI. 
Experience shows that this workflow can work as an alternative, 
evidenced by the userbase of the commit-patch package.

diff-mode provides commands to delete hunks, split them, and even allows 
one to edit individual characters (something that 'git add -p' doesn't 
provide). So I'd argue this way might even be more powerful.

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

[Alfred M. Szmidt]

   > This is a pitty indeed.  Specially seeing that it is easy enough to
   > get this working for all VCSs.  Here is a hack I wrote ages ago, and
   > this works for just about everything, it doesn't do exactly what is
   > needed (e.g, this works on a single file basis) but it isn't too hard
   > to get that working.

   If you have time, could you work on integrating that code with VC?

   I would, but currently I'm heavily occupied with some other code (which
   happens to use Subversion), and the feature would be quite convenient.

I wish, I'm swamped at work :-( The feature as such is indispensible,
and not having to care about the VC probobly even more so.

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

[Dmitry Gutov]

On 30.08.2022 16:25, Alfred M. Szmidt wrote:
> This is a pitty indeed.  Specially seeing that it is easy enough to
> get this working for all VCSs.  Here is a hack I wrote ages ago, and
> this works for just about everything, it doesn't do exactly what is
> needed (e.g, this works on a single file basis) but it isn't too hard
> to get that working.

This indeed could work as an approach (I suggested /tmp, you are using a 
single buffer to back up contents, but the effect should be the same).

Do you like the current behavior with Git, though? Or do you perhaps 
have the same misgivings as Eli?

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

[Alfred M. Szmidt]

   On 30.08.2022 16:25, Alfred M. Szmidt wrote:
   > This is a pitty indeed.  Specially seeing that it is easy enough to
   > get this working for all VCSs.  Here is a hack I wrote ages ago, and
   > this works for just about everything, it doesn't do exactly what is
   > needed (e.g, this works on a single file basis) but it isn't too hard
   > to get that working.

   This indeed could work as an approach (I suggested /tmp, you are using a 
   single buffer to back up contents, but the effect should be the same).

Yup, a temp file would make it work for multiple files.

   Do you like the current behavior with Git, though? Or do you perhaps 
   have the same misgivings as Eli?

I've not tried Juri's change, but from the point of maintaining
vc-fossil it would mean that I need to figure out how to implement
this vc-checkin-patch function (and likewise for anyone else),
specially when there is no need to make this git specific for the most
common case.

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

[Dmitry Gutov]

On 30.08.2022 17:20, Alfred M. Szmidt wrote:
>     On 30.08.2022 16:25, Alfred M. Szmidt wrote:
>     > This is a pitty indeed.  Specially seeing that it is easy enough to
>     > get this working for all VCSs.  Here is a hack I wrote ages ago, and
>     > this works for just about everything, it doesn't do exactly what is
>     > needed (e.g, this works on a single file basis) but it isn't too hard
>     > to get that working.
> 
>     This indeed could work as an approach (I suggested /tmp, you are using a
>     single buffer to back up contents, but the effect should be the same).
> 
> Yup, a temp file would make it work for multiple files.
> 
>     Do you like the current behavior with Git, though? Or do you perhaps
>     have the same misgivings as Eli?
> 
> I've not tried Juri's change, but from the point of maintaining
> vc-fossil it would mean that I need to figure out how to implement
> this vc-checkin-patch function (and likewise for anyone else),
> specially when there is no need to make this git specific for the most
> common case.

I can look into adapting this approach for other VCS if nobody beats me 
to it. As long as we fundamentally agree on the behavior.

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

[Sean Whitton]

Hello,

On Tue 30 Aug 2022 at 02:54PM +03, Eli Zaretskii wrote:

>> From: Juri Linkov <juri@jurta.org>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  Lars Ingebrigtsen <larsi@gnus.org>,
>>   emacs-devel@gnu.org
>> Date: Tue, 30 Aug 2022 10:20:10 +0300
>>
>> Anyway, the main point is that adepts of any existing VCS or any future
>> VCS can easily adapt their VCSes for this feature by just implementing
>> the backend call 'checkin-patch'.
>
> I very much hope this is not going to be the official policy of VC
> development in Emacs.  VC should IMO remain VCS-agnostic in its most
> important features, and "C-x v v" is definitely such a feature of VC.

Surely leaving room for things to be implemented for other VCs means
that it remains VC-agnostic.  With e.g. Magit there isn't even that room.

> That's basically the only real raison d'être of VC nowadays, because
> otherwise let's just obsolete VC and switch to Magit.

VC is far more flexible and Emacs-y than Magit in how it handles
buffers.  E.g. in Magit if you C-x x u a log buffer, because you want to
log something else at the same time, Magit will re-use the first log
buffer anyway.  So for those of us who like it when things are more
buffer-oriented, it remains a good alternative.

-- 
Sean Whitton

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

[Stefan Monnier]

> I can look into adapting this approach for other VCS if nobody beats me to
> it. As long as we fundamentally agree on the behavior.

Fine by me.  I'll just mention that I think we should be careful to make
sure no info is lost if Emacs crashes in the middle of the operation.


        Stefan

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

[Dmitry Gutov]

On 30.08.2022 18:16, Stefan Monnier wrote:
>> I can look into adapting this approach for other VCS if nobody beats me to
>> it. As long as we fundamentally agree on the behavior.
> Fine by me.  I'll just mention that I think we should be careful to make
> sure no info is lost if Emacs crashes in the middle of the operation.

File contents' backups should remain somewhere in /tmp, *shrug*.

It might take some effort to find them, though.

I think it's hard to implement this with even more safety somehow. Git 
makes it uniquely easier to do.

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

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> File contents' backups should remain somewhere in /tmp, *shrug*.

What about stashing the files using the auto-save machinery?  Then Emacs
should be able to find them again if Emacs crashed during the operation.

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

[Dmitry Gutov]

On 30.08.2022 18:33, Lars Ingebrigtsen wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>> File contents' backups should remain somewhere in /tmp,*shrug*.
> What about stashing the files using the auto-save machinery?  Then Emacs
> should be able to find them again if Emacs crashed during the operation.

The user might have backups disabled, though. That seems to require an 
additional fallback...

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

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> The user might have backups disabled, though. That seems to require an
> additional fallback...

We could force-enable them for this operation.

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

[Eli Zaretskii]

> Date: Tue, 30 Aug 2022 16:52:10 +0300
> Cc: larsi@gnus.org, juri@jurta.org, emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> >> 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.
> > 
> > Ouch!  Then this feature will be useless for me, and I'm sorry I
> > wasted everyone's time based on the description in NEWS, which doesn't
> > make a point of emphasizing this basic assumption.
> 
> Perhaps we could make it more useful for you as well, e.g. by applying 
> all the changes from the patch first when called with 'C-u'.

Alas, C-u already has a meaning with "C-x v v".

> But I suppose if your patches apply cleanly most of the time, it can be 
> less of a problem.

They do, when I choose them not to be rejected ;-)

> diff-mode provides commands to delete hunks, split them, and even allows 
> one to edit individual characters (something that 'git add -p' doesn't 
> provide). So I'd argue this way might even be more powerful.

From my POV, a better UI would be to enter diff-mode _after_ the user
invoked VC to commit the changes, but asked specifically to commit
them selectively.  Not the other way around.  IOW, the process of
generating the diffs buffer should be part of the command execution,
not a prerequisite for it.

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

[Eli Zaretskii]

> Date: Tue, 30 Aug 2022 17:38:27 +0300
> Cc: eliz@gnu.org, larsi@gnus.org, juri@jurta.org, emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> I can look into adapting this approach for other VCS if nobody beats me 
> to it.

Thanks.

>  As long as we fundamentally agree on the behavior.

I think the "C-x v D" step should be part of the command's execution.
It makes little sense to me to ask the user to prepare the diffs in
advance, since conceptually the changes are already on disk.  Starting
from diffs makes sense when you want to apply them first, and only
after that commit.  Which is not the case here.

I understand that typing "C-x v v" in a diffs buffer is used here as
an indication that this particular variant of "committing" is
requested, but that's a weak justification, IMO.  It could even
backfire: the user could be in the diffs by sheer luck (or lack
thereof), and type "C-x v v" without any intention to select parts of
the changes.

I think a special numeric argument (like "C-u 0", perhaps?) is more
appropriate to signal the interactive selection of changes.

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

[Juri Linkov]

> VC is far more flexible and Emacs-y than Magit in how it handles
> buffers.  E.g. in Magit if you C-x x u a log buffer, because you want to
> log something else at the same time, Magit will re-use the first log
> buffer anyway.  So for those of us who like it when things are more
> buffer-oriented, it remains a good alternative.

Indeed, everything works fine in VC with such customization:

  (add-hook 'diff-mode-hook 'rename-uniquely)
  (add-hook 'log-view-mode-hook 'rename-uniquely)
  (add-hook 'log-edit-mode-hook 'rename-uniquely)

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I think the "C-x v D" step should be part of the command's execution.
> It makes little sense to me to ask the user to prepare the diffs in
> advance, since conceptually the changes are already on disk.

It's just an easy way to decide which changes are part of this commit.

> I think a special numeric argument (like "C-u 0", perhaps?) is more
> appropriate to signal the interactive selection of changes.

And then it asks you for every hunk whether to include it in the commit
or not?  Or did you have something else in mind?

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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Tue, 30 Aug 2022 07:49:56 -0700
> 
> >> Anyway, the main point is that adepts of any existing VCS or any future
> >> VCS can easily adapt their VCSes for this feature by just implementing
> >> the backend call 'checkin-patch'.
> >
> > I very much hope this is not going to be the official policy of VC
> > development in Emacs.  VC should IMO remain VCS-agnostic in its most
> > important features, and "C-x v v" is definitely such a feature of VC.
> 
> Surely leaving room for things to be implemented for other VCs means
> that it remains VC-agnostic.  With e.g. Magit there isn't even that room.

Yeah, and then let's "leave room" for adepts of documentation to add
it if they feel like it, and for adepts of Lisp style to fix the style
of the code someone else installed, and for adepts of clean design and
implementation to clean up after others, etc. etc.

We have some conventions and some standards in Emacs development, so
I'm surprised to hear that some of them are left to "adepts" to
uphold.  If we go that way, there are good chances that some things
will never happen, because no adepts were in supply, or because we
forgot, or for any number of other reasons.

> > That's basically the only real raison d'être of VC nowadays, because
> > otherwise let's just obsolete VC and switch to Magit.
> 
> VC is far more flexible and Emacs-y than Magit in how it handles
> buffers.

I'm sure frequent users of Magit disagree.

Of course, that's not why I mentioned Magit: I mentioned it because if
we want to cater to a single VCS, a very good solution for that
already exists.

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

[Juri Linkov]

>> 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).

Indeed, just 'M-! git apply' before using 'vc-checkin-patch' will do the job
in case of an external patch.

> 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'?

There is 'ediff-patch-buffer' that does this interactively like 'git add -p'.

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: "Alfred M. Szmidt" <ams@gnu.org>,  eliz@gnu.org,  larsi@gnus.org,
>  juri@jurta.org,  emacs-devel@gnu.org
> Date: Tue, 30 Aug 2022 11:16:17 -0400
> 
> > I can look into adapting this approach for other VCS if nobody beats me to
> > it. As long as we fundamentally agree on the behavior.
> 
> Fine by me.  I'll just mention that I think we should be careful to make
> sure no info is lost if Emacs crashes in the middle of the operation.

Emacs never crashes.

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Dmitry Gutov <dgutov@yandex.ru>,  ams@gnu.org,  juri@jurta.org,
>   emacs-devel@gnu.org
> Date: Tue, 30 Aug 2022 18:45:18 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I think the "C-x v D" step should be part of the command's execution.
> > It makes little sense to me to ask the user to prepare the diffs in
> > advance, since conceptually the changes are already on disk.
> 
> It's just an easy way to decide which changes are part of this commit.

I didn't say it shouldn't be displayed, I said the user shouldn't be
required to prepare this display manually; the command should do it
instead.

> > I think a special numeric argument (like "C-u 0", perhaps?) is more
> > appropriate to signal the interactive selection of changes.
> 
> And then it asks you for every hunk whether to include it in the commit
> or not?

And then show a message to the user saying something like

  Edit the diffs to leave only the hunks you want to commit,
  then type C-c to go ahead and commit them.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@jurta.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  larsi@gnus.org,  emacs-devel@gnu.org
> Date: Tue, 30 Aug 2022 19:47:35 +0300
> 
> >> 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).
> 
> Indeed, just 'M-! git apply' before using 'vc-checkin-patch' will do the job
> in case of an external patch.

Unless some of the hunks are wrong or produce conflicts or are
rejected due to 1001 reasons.  Which is exactly the use case I thought
this enabled.  Sorry for misreading.

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

[Dmitry Gutov]

On 30.08.2022 19:37, Eli Zaretskii wrote:
>>   As long as we fundamentally agree on the behavior.
> I think the "C-x v D" step should be part of the command's execution.
> It makes little sense to me to ask the user to prepare the diffs in
> advance, since conceptually the changes are already on disk.  Starting
> from diffs makes sense when you want to apply them first, and only
> after that commit.  Which is not the case here.

The user can choose the files first (e.g. choose a directory from 
VC-Dir), or just pick one file they are currently editing. Then see the 
diff, and make the choice of making a commit from it.

What you're suggesting could possibly improve discoverability (if the 
user manages to read the doc and find out about the meaning of C-0 
prefix), but WRT flexibility, the effect can be negative. That is, 
unless we keep the new meaning of 'C-x v v' in diff buffers, the ability 
to commit an arbitrary diff (not applied yet) will go away. And it's 
what the author of the top message in this tree was particularly happy 
about.

And as for the length of key sequences, in VC-Dir, 'D C-x v v' is about 
the same as 'C-0 C-x v v'.

> I understand that typing "C-x v v" in a diffs buffer is used here as
> an indication that this particular variant of "committing" is
> requested, but that's a weak justification, IMO.  It could even
> backfire: the user could be in the diffs by sheer luck (or lack
> thereof), and type "C-x v v" without any intention to select parts of
> the changes.

If they have no intention of creating a commit from that diff and press 
'C-x v v' by accident, they can simply 'C-c C-k' out of the resulting 
Log-Edit buffer. That will bring them back to the diff, restoring the 
previous window configuration.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> And then show a message to the user saying something like
>
>   Edit the diffs to leave only the hunks you want to commit,
>   then type C-c to go ahead and commit them.

In a recursive edit?

Sure, that's a possible interface, but I don't really see much practical
difference, except being less flexible.  You're proposing a
"start-a-selective-commit" command followed by `C-c C-c'.

But today you can say `C-x v D' to make the diff buffer, but you can
also make it in other ways, like `v' in the vc-dir buffer, or your own
self-composed command to get the current tree's diff (I've got one on
`H-d' that does some other stuff first and then presents me with the
diff buffer).  And then you an edit the buffer, and `C-x v v' to commit
just the selected bits.

It's more flexible, and not more work for the user than what you're
proposing, as far as I can tell.

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

[Dmitry Gutov]

On 30.08.2022 20:05, Eli Zaretskii wrote:
>> Indeed, just 'M-! git apply' before using 'vc-checkin-patch' will do the job
>> in case of an external patch.
> Unless some of the hunks are wrong or produce conflicts or are
> rejected due to 1001 reasons.  Which is exactly the use case I thought
> this enabled.  Sorry for misreading.

Not sure how we could possibly automate that problem away.

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

[Juri Linkov]

>> >> 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).
>>
>> Indeed, just 'M-! git apply' before using 'vc-checkin-patch' will do the job
>> in case of an external patch.
>
> Unless some of the hunks are wrong or produce conflicts or are
> rejected due to 1001 reasons.  Which is exactly the use case I thought
> this enabled.  Sorry for misreading.

'vc-git-checkin-patch' is conflict-free.  It bypasses the working tree.
OTOH, applying a patch to the working tree often requires manual resolution.

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

[Dmitry Gutov]

On 30.08.2022 19:20, Eli Zaretskii wrote:
>> Date: Tue, 30 Aug 2022 16:52:10 +0300
>> Cc: larsi@gnus.org, juri@jurta.org, emacs-devel@gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>>
>>>> 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.
>>>
>>> Ouch!  Then this feature will be useless for me, and I'm sorry I
>>> wasted everyone's time based on the description in NEWS, which doesn't
>>> make a point of emphasizing this basic assumption.
>>
>> Perhaps we could make it more useful for you as well, e.g. by applying
>> all the changes from the patch first when called with 'C-u'.
> 
> Alas, C-u already has a meaning with "C-x v v".

For better or worse, it doesn't seem to do anything in diff-mode 
buffers. But we could also use 'C-0'. Or something similar.

>> But I suppose if your patches apply cleanly most of the time, it can be
>> less of a problem.
> 
> They do, when I choose them not to be rejected ;-)

Admirable.

>> diff-mode provides commands to delete hunks, split them, and even allows
>> one to edit individual characters (something that 'git add -p' doesn't
>> provide). So I'd argue this way might even be more powerful.
> 
>  From my POV, a better UI would be to enter diff-mode _after_ the user
> invoked VC to commit the changes, but asked specifically to commit
> them selectively.  Not the other way around.  IOW, the process of
> generating the diffs buffer should be part of the command execution,
> not a prerequisite for it.

I suppose this UI could be implemented on top of the current behavior as 
well.

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: dgutov@yandex.ru,  ams@gnu.org,  juri@jurta.org,  emacs-devel@gnu.org
> Date: Tue, 30 Aug 2022 19:11:27 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > And then show a message to the user saying something like
> >
> >   Edit the diffs to leave only the hunks you want to commit,
> >   then type C-c to go ahead and commit them.
> 
> In a recursive edit?

Yes.

> Sure, that's a possible interface, but I don't really see much practical
> difference, except being less flexible.

It's a more natural UI, IMO.

> It's more flexible, and not more work for the user than what you're
> proposing, as far as I can tell.

But it's backward from the UI POV.

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

[Eli Zaretskii]

> Date: Tue, 30 Aug 2022 20:11:52 +0300
> Cc: larsi@gnus.org, emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> On 30.08.2022 20:05, Eli Zaretskii wrote:
> >> Indeed, just 'M-! git apply' before using 'vc-checkin-patch' will do the job
> >> in case of an external patch.
> > Unless some of the hunks are wrong or produce conflicts or are
> > rejected due to 1001 reasons.  Which is exactly the use case I thought
> > this enabled.  Sorry for misreading.
> 
> Not sure how we could possibly automate that problem away.

You don't need to.  Aborting a botched patch is easy; once the
problematic hunks are known (they are announced by Patch or by
"git apply"), you use this facility to filter them out, and
patch/commit again.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> It's more flexible, and not more work for the user than what you're
>> proposing, as far as I can tell.
>
> But it's backward from the UI POV.

We could implement your proposed "start a selective commit of the root
of the tree" command on top of what we have now pretty trivially.

But what's implemented now is what I want to have.

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

[Dmitry Gutov]

On 30.08.2022 20:37, Eli Zaretskii wrote:
>> Date: Tue, 30 Aug 2022 20:11:52 +0300
>> Cc:larsi@gnus.org,emacs-devel@gnu.org
>> From: Dmitry Gutov<dgutov@yandex.ru>
>>
>> On 30.08.2022 20:05, Eli Zaretskii wrote:
>>>> Indeed, just 'M-! git apply' before using 'vc-checkin-patch' will do the job
>>>> in case of an external patch.
>>> Unless some of the hunks are wrong or produce conflicts or are
>>> rejected due to 1001 reasons.  Which is exactly the use case I thought
>>> this enabled.  Sorry for misreading.
>> Not sure how we could possibly automate that problem away.
> You don't need to.  Aborting a botched patch is easy; once the
> problematic hunks are known (they are announced by Patch or by
> "git apply"), you use this facility to filter them out, and
> patch/commit again.

But filtering those out is not what one usually wants to do.

It's more common to want to install all the included changes, even if 
some might require fixing.

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

[Stefan Monnier]

Dmitry Gutov [2022-08-30 18:26:41] wrote:
> On 30.08.2022 18:16, Stefan Monnier wrote:
>>> I can look into adapting this approach for other VCS if nobody beats me to
>>> it. As long as we fundamentally agree on the behavior.
>> Fine by me.  I'll just mention that I think we should be careful to make
>> sure no info is lost if Emacs crashes in the middle of the operation.
> File contents' backups should remain somewhere in /tmp, *shrug*.
> It might take some effort to find them, though.

We could do something like:

    run-the-vcs-diff-on-the FILES >VC-pending-changes
    patch -R <VC-pending-changes
    ...do the vc-checkin-patch...
    patch <VC-pending-changes
    rm VC-pending-changes

or (closer to Git's behavior):

    tar -cf VC-pending-changes.tar FILES
    vc-revert FILES
    ...do the vc-checkin-patch...
    tar xf VC-pending-changes.tar
    rm VC-pending-changes.tar

> I think it's hard to implement this with even more safety somehow.

I'm just worried that the uncommmitted changes we need to temporarily
remove are the rare parts that contain information that's not yet
duplicated in the VCS's data, so if we only store it in an Emacs buffer
it's vulnerable to an Emacs crash (which is something that *does*
happen, especially for us poor souls who run with experimental code and
with all assertions enabled :-).

> Git makes it uniquely easier to do.

Indeed.


        Stefan

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

[Sean Whitton]

Hello,

On Tue 30 Aug 2022 at 07:11PM +02, Lars Ingebrigtsen wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> And then show a message to the user saying something like
>>
>>   Edit the diffs to leave only the hunks you want to commit,
>>   then type C-c to go ahead and commit them.
>
> In a recursive edit?
>
> Sure, that's a possible interface, but I don't really see much practical
> difference, except being less flexible.  You're proposing a
> "start-a-selective-commit" command followed by `C-c C-c'.
>
> But today you can say `C-x v D' to make the diff buffer, but you can
> also make it in other ways, like `v' in the vc-dir buffer, or your own
> self-composed command to get the current tree's diff (I've got one on
> `H-d' that does some other stuff first and then presents me with the
> diff buffer).  And then you an edit the buffer, and `C-x v v' to commit
> just the selected bits.
>
> It's more flexible, and not more work for the user than what you're
> proposing, as far as I can tell.

I agree, and prefer how with the C-x v D approach you aren't in some
incomplete or transient state, waiting for you to trim the diff.  You
can just do it when and if you want to.

-- 
Sean Whitton

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

[Dmitry Gutov]

On 30.08.2022 21:36, Stefan Monnier wrote:
> Dmitry Gutov [2022-08-30 18:26:41] wrote:
>> On 30.08.2022 18:16, Stefan Monnier wrote:
>>>> I can look into adapting this approach for other VCS if nobody beats me to
>>>> it. As long as we fundamentally agree on the behavior.
>>> Fine by me.  I'll just mention that I think we should be careful to make
>>> sure no info is lost if Emacs crashes in the middle of the operation.
>> File contents' backups should remain somewhere in /tmp, *shrug*.
>> It might take some effort to find them, though.
> 
> We could do something like:
> 
>      run-the-vcs-diff-on-the FILES >VC-pending-changes
>      patch -R <VC-pending-changes
>      ...do the vc-checkin-patch...
>      patch <VC-pending-changes
>      rm VC-pending-changes
> 
> or (closer to Git's behavior):
> 
>      tar -cf VC-pending-changes.tar FILES
>      vc-revert FILES
>      ...do the vc-checkin-patch...
>      tar xf VC-pending-changes.tar
>      rm VC-pending-changes.tar

Except we probably can't rely on 'patch' or 'tar' being installed on the 
user's system.

>> I think it's hard to implement this with even more safety somehow.
> 
> I'm just worried that the uncommmitted changes we need to temporarily
> remove are the rare parts that contain information that's not yet
> duplicated in the VCS's data, so if we only store it in an Emacs buffer
> it's vulnerable to an Emacs crash (which is something that *does*
> happen, especially for us poor souls who run with experimental code and
> with all assertions enabled :-).

The /tmp approach implied 'cp', not storing contents of files in 
background buffers.

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

[Po Lu]

Dmitry Gutov <dgutov@yandex.ru> writes:

> I can look into adapting this approach for other VCS if nobody beats
> me to it. As long as we fundamentally agree on the behavior.

Thank you.

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

[Dmitry Gutov]

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

On 31.08.2022 04:28, Po Lu wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>> I can look into adapting this approach for other VCS if nobody beats
>> me to it. As long as we fundamentally agree on the behavior.
> Thank you.

This seems to work.

Might use some extra polish, though, if people also encounter the one 
weird error about "buffer already killed" that I saw a few times but 
then stopped after a restart.

[-- Attachment #2: vc-default-checkin-patch.diff --]
[-- Type: text/x-patch, Size: 1704 bytes --]

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index d93be951a3..d6eda2b688 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1667,6 +1667,41 @@ vc-checkin
    backend
    patch-string))
 
+(declare-function diff-apply-hunk "diff-mode" (&optional reverse))
+
+(defun vc-default-checkin-patch (backend patch-string comment)
+  (with-current-buffer (get-buffer-create " *vc-checkin-patch*")
+    (erase-buffer)
+    (insert patch-string)
+    (diff-mode)
+    (pcase-let ((`(,backend ,files) (diff-vc-deduce-fileset))
+                (root (vc-call-backend backend 'root default-directory))
+                (tmpdir (make-temp-file "vc-checkin-patch" t)))
+      (dolist (f files)
+        (copy-file (expand-file-name f root)
+                   (expand-file-name f tmpdir)))
+      (unwind-protect
+          (let ((default-directory root))
+            (dolist (f files)
+              (with-current-buffer (get-file-buffer f)
+                (vc-revert-file f)
+                (revert-buffer t t t)))
+            (goto-char (point-min))
+            (while (not (eobp))
+              (diff-apply-hunk))
+            (dolist (f files)
+              (with-current-buffer (get-file-buffer f)
+                (save-buffer 0)))
+            (vc-checkin files backend comment))
+        (dolist (f files)
+          (copy-file (expand-file-name f tmpdir)
+                     (expand-file-name f root)
+                     t)
+          (with-current-buffer (get-file-buffer f)
+            (revert-buffer t t t))
+          (delete-directory tmpdir t))
+        (bury-buffer)))))
+
 ;;; Additional entry points for examining version histories
 
 ;; (defun vc-default-diff-tree (backend dir rev1 rev2)

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

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > VC is far more flexible and Emacs-y than Magit in how it handles
  > buffers.  E.g. in Magit if you C-x x u a log buffer, because you want to
  > log something else at the same time, Magit will re-use the first log
  > buffer anyway.

Isn't that a bug in Magit?  It could be fixed
to handle special buffers in the usual Emacs fashion that other packages use.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

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

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > Surely leaving room for things to be implemented for other VCs means
  > > that it remains VC-agnostic.  With e.g. Magit there isn't even that room.

  > Yeah, and then let's "leave room" for adepts of documentation to add
  > it if they feel like it, and for adepts of Lisp style to fix the style
  > of the code someone else installed, and for adepts of clean design and
  > implementation to clean up after others, etc. etc.

Hear, hear!  Finishing a change fully is a necessary part of starting
it.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

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

[Alfred M. Szmidt]

   > From: Stefan Monnier <monnier@iro.umontreal.ca>
   > Cc: "Alfred M. Szmidt" <ams@gnu.org>,  eliz@gnu.org,  larsi@gnus.org,
   >  juri@jurta.org,  emacs-devel@gnu.org
   > Date: Tue, 30 Aug 2022 11:16:17 -0400
   > 
   > > I can look into adapting this approach for other VCS if nobody beats me to
   > > it. As long as we fundamentally agree on the behavior.
   > 
   > Fine by me.  I'll just mention that I think we should be careful to make
   > sure no info is lost if Emacs crashes in the middle of the operation.

   Emacs never crashes.

:-)

But why is it important that no info is lost during a crash? *vc-diff*
buffers, and in general any non-file associated buffer is always lost
in the case of a crash.

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

[Juri Linkov]

>>> I can look into adapting this approach for other VCS if nobody beats
>>> me to it. As long as we fundamentally agree on the behavior.
>> Thank you.
>
> This seems to work.
>
> Might use some extra polish, though, if people also encounter the one weird
> error about "buffer already killed" that I saw a few times but then stopped
> after a restart.

Thanks, looks universal enough to handle all backends.
I've tried it out, but it gets stuck asking the same question:

  Hunk has already been applied; undo it? (y or n)

because of the infinite loop in:

            (while (not (eobp))
              (diff-apply-hunk))

After removing the loop, everything works fine.

> +(defun vc-default-checkin-patch (backend patch-string comment)
> +  (with-current-buffer (get-buffer-create " *vc-checkin-patch*")
> +    (erase-buffer)
> +    (insert patch-string)
> +    (diff-mode)
> +    (pcase-let ((`(,backend ,files) (diff-vc-deduce-fileset))
> +                (root (vc-call-backend backend 'root default-directory))
> +                (tmpdir (make-temp-file "vc-checkin-patch" t)))

I'm worried that after a crash, reboot might wipe out the /tmp dir.

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

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

>> Might use some extra polish, though, if people also encounter the one weird
>> error about "buffer already killed" that I saw a few times but then stopped
>> after a restart.
>
> Thanks, looks universal enough to handle all backends.

I hope that the already-implemented safe version for Git doesn't get
lost, though.

Emacs' number one job is to never lose people's data, and the current
code is safe in that regard.  This new, more general code, does not look
safe: If something goes wrong (like the computer going down) in the
middle of this operation, we lose people's files, and that's not
acceptable.

And that's not unlikely, because a checkin in some VCs means talking
over the network, so anything can fail (or hang), basically.

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

[Dmitry Gutov]

On 31.08.2022 12:55, Lars Ingebrigtsen wrote:
> Juri Linkov <juri@linkov.net> writes:
> 
>>> Might use some extra polish, though, if people also encounter the one weird
>>> error about "buffer already killed" that I saw a few times but then stopped
>>> after a restart.
>>
>> Thanks, looks universal enough to handle all backends.
> 
> I hope that the already-implemented safe version for Git doesn't get
> lost, though.

Definitely not. It's simpler, faster and, by design, more reliable.

> Emacs' number one job is to never lose people's data, and the current
> code is safe in that regard.  This new, more general code, does not look
> safe: If something goes wrong (like the computer going down) in the
> middle of this operation, we lose people's files, and that's not
> acceptable.

Those files have been copied to a directory, so they should always be 
recoverable, no?

> And that's not unlikely, because a checkin in some VCs means talking
> over the network, so anything can fail (or hang), basically.

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

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> > I hope that the already-implemented safe version for Git doesn't get
> > lost, though.
> 
> Definitely not. It's simpler, faster and, by design, more reliable.

*phew*

> Those files have been copied to a directory, so they should always be
> recoverable, no?

The /tmp file is routinely wiped, so the files should be copied to
somewhere under ~/.emacs.d/ instead, or in the current directory, if
possible.  And there should be a mechanism to let you know that you can
recover the files if something went wrong.

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

[Dmitry Gutov]

On 31.08.2022 13:37, Lars Ingebrigtsen wrote:
>> Those files have been copied to a directory, so they should always be
>> recoverable, no?
> The /tmp file is routinely wiped, so the files should be copied to
> somewhere under ~/.emacs.d/ instead, or in the current directory, if
> possible.  And there should be a mechanism to let you know that you can
> recover the files if something went wrong.

A "hidden" directory under the project root is one option.

Reusing the backups mechanism should also work but seemed like more of a 
pain to implement. But improvements welcome, of course.

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

[Dmitry Gutov]

On 31.08.2022 09:53, Juri Linkov wrote:
>>>> I can look into adapting this approach for other VCS if nobody beats
>>>> me to it. As long as we fundamentally agree on the behavior.
>>> Thank you.
>> This seems to work.
>>
>> Might use some extra polish, though, if people also encounter the one weird
>> error about "buffer already killed" that I saw a few times but then stopped
>> after a restart.
> Thanks, looks universal enough to handle all backends.
> I've tried it out, but it gets stuck asking the same question:
> 
>    Hunk has already been applied; undo it? (y or n)
> 
> because of the infinite loop in:
> 
>              (while (not (eobp))
>                (diff-apply-hunk))
> 
> After removing the loop, everything works fine.

Do you perhaps have customized diff-advance-after-apply-hunk to nil? I 
guess a let-binding is in order.

But a loop is necessary because the diff can have more than one hunk, 
isn't it?

>> +(defun vc-default-checkin-patch (backend patch-string comment)
>> +  (with-current-buffer (get-buffer-create "*vc-checkin-patch*")
>> +    (erase-buffer)
>> +    (insert patch-string)
>> +    (diff-mode)
>> +    (pcase-let ((`(,backend ,files) (diff-vc-deduce-fileset))
>> +                (root (vc-call-backend backend 'root default-directory))
>> +                (tmpdir (make-temp-file "vc-checkin-patch" t)))
> I'm worried that after a crash, reboot might wipe out the /tmp dir.

Creating the tmp directory inside root is also an option.

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

[Eli Zaretskii]

> Date: Wed, 31 Aug 2022 05:32:42 +0300
> Cc: "Alfred M. Szmidt" <ams@gnu.org>, eliz@gnu.org, larsi@gnus.org,
>  juri@jurta.org, emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> >> I can look into adapting this approach for other VCS if nobody beats
> >> me to it. As long as we fundamentally agree on the behavior.
> > Thank you.
> 
> This seems to work.

Thanks.

> +      (dolist (f files)
> +        (copy-file (expand-file-name f root)
> +                   (expand-file-name f tmpdir)))

What if f includes leading directories, as in "lisp/foo.el"?  If
tmpdir doesn't have a subdirectory 'lisp', the copy-file will fail,
no?

> +      (unwind-protect
> +          (let ((default-directory root))
> +            (dolist (f files)
> +              (with-current-buffer (get-file-buffer f)
                                      ^^^^^^^^^^^^^^^^^^^
This assumes that all the files are visited in buffers?  Is that
guaranteed?

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

[Dmitry Gutov]

On 31.08.2022 14:23, Eli Zaretskii wrote:
>> Date: Wed, 31 Aug 2022 05:32:42 +0300
>> Cc: "Alfred M. Szmidt" <ams@gnu.org>, eliz@gnu.org, larsi@gnus.org,
>>   juri@jurta.org, emacs-devel@gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>>
>>>> I can look into adapting this approach for other VCS if nobody beats
>>>> me to it. As long as we fundamentally agree on the behavior.
>>> Thank you.
>>
>> This seems to work.
> 
> Thanks.
> 
>> +      (dolist (f files)
>> +        (copy-file (expand-file-name f root)
>> +                   (expand-file-name f tmpdir)))
> 
> What if f includes leading directories, as in "lisp/foo.el"?  If
> tmpdir doesn't have a subdirectory 'lisp', the copy-file will fail,
> no?

That's unfortunate.

>> +      (unwind-protect
>> +          (let ((default-directory root))
>> +            (dolist (f files)
>> +              (with-current-buffer (get-file-buffer f)
>                                        ^^^^^^^^^^^^^^^^^^^
> This assumes that all the files are visited in buffers?  Is that
> guaranteed?

Right. That might account for that one-weird-bug I've seen once.

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

[Eli Zaretskii]

> Date: Wed, 31 Aug 2022 14:57:38 +0300
> Cc: luangruo@yahoo.com, ams@gnu.org, larsi@gnus.org, juri@jurta.org,
>  emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> >> +      (dolist (f files)
> >> +        (copy-file (expand-file-name f root)
> >> +                   (expand-file-name f tmpdir)))
> > 
> > What if f includes leading directories, as in "lisp/foo.el"?  If
> > tmpdir doesn't have a subdirectory 'lisp', the copy-file will fail,
> > no?
> 
> That's unfortunate.

I think you need to precede the copy-file call by

  (make-directory (file-name-directory (expand-file-name f tmpdir)) t)

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

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> A "hidden" directory under the project root is one option.

It occurs to me that if we (i.e., you 😀) do this, you've almost
implemented a general mechanism for stashing, so it'd be great if that
was implemented fully.  I.e., stashing the files somewhere handy, and
give the stash a name, and there you are.  (In this case the name would
be automatically generated, of course.)

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

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:

>> VC is far more flexible and Emacs-y than Magit in how it handles
>> buffers.
>
> I'm sure frequent users of Magit disagree.

Maybe, but as its maintainer, I increasingly agree. ;)

A quick hack to make it use more buffers:

  (add-hook 'magit-create-buffer-hook 'magit-create-buffer-lock)

  (defun magit-create-buffer-lock ()
    (and (magit-buffer-value)
         (magit-toggle-buffer-lock)))

(I wrote that just now and didn't test much. Things might blow up.)

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

[Juri Linkov]

>> I've tried it out, but it gets stuck asking the same question:
>>    Hunk has already been applied; undo it? (y or n)
>> because of the infinite loop in:
>>              (while (not (eobp))
>>                (diff-apply-hunk))
>> After removing the loop, everything works fine.
>
> Do you perhaps have customized diff-advance-after-apply-hunk to nil?
> I guess a let-binding is in order.

diff-advance-after-apply-hunk is still t.

> But a loop is necessary because the diff can have more than one hunk, isn't
> it?

A loop is necessary indeed.  The problem is that I tried it with Bzr,
but it has such a bug that `bzr diff` adds an extra line at the end,
so it's not (eobp) after the last diff hunk.  After removing the
final newline in the *vc-diff* buffer the loop can be finished.

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

[Dmitry Gutov]

On 31.08.2022 19:06, Juri Linkov wrote:
>> But a loop is necessary because the diff can have more than one hunk, isn't
>> it?
> A loop is necessary indeed.  The problem is that I tried it with Bzr,
> but it has such a bug that `bzr diff` adds an extra line at the end,
> so it's not (eobp) after the last diff hunk.  After removing the
> final newline in the*vc-diff*  buffer the loop can be finished.

Hmm, so how do we apply the full diff in a backend-agnostic fashion?

Should we delegate to 'patch', then? And require it to be installed.

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

[Dmitry Gutov]

On 31.08.2022 18:42, Lars Ingebrigtsen wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>> A "hidden" directory under the project root is one option.
> It occurs to me that if we (i.e., you 😀) do this, you've almost

I prefer "we" in this case. ;-)

> implemented a general mechanism for stashing, so it'd be great if that
> was implemented fully.  I.e., stashing the files somewhere handy, and
> give the stash a name, and there you are.  (In this case the name would
> be automatically generated, of course.)

I don't use stashing commands from VC myself, but the stash mechanism is 
usually more complex, e.g. it usually knows how to merge changes (when 
the user has edited the file in some place and then pops the stash) and 
indicate conflicts.

That's why a stash is usually represented as a diff, I guess.

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

[Eli Zaretskii]

> Date: Wed, 31 Aug 2022 19:21:12 +0300
> Cc: Po Lu <luangruo@yahoo.com>, "Alfred M. Szmidt" <ams@gnu.org>,
>  eliz@gnu.org, larsi@gnus.org, emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> On 31.08.2022 19:06, Juri Linkov wrote:
> >> But a loop is necessary because the diff can have more than one hunk, isn't
> >> it?
> > A loop is necessary indeed.  The problem is that I tried it with Bzr,
> > but it has such a bug that `bzr diff` adds an extra line at the end,
> > so it's not (eobp) after the last diff hunk.  After removing the
> > final newline in the*vc-diff*  buffer the loop can be finished.
> 
> Hmm, so how do we apply the full diff in a backend-agnostic fashion?
> 
> Should we delegate to 'patch', then? And require it to be installed.

Shouldn't the code which looks for hunks be resilient to such minor
differences?  Why does it fail if there's an extra line at the end?
Does that line look anything like another hunk?

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

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> I don't use stashing commands from VC myself, but the stash mechanism
> is usually more complex, e.g. it usually knows how to merge changes
> (when the user has edited the file in some place and then pops the
> stash) and indicate conflicts.
>
> That's why a stash is usually represented as a diff, I guess.

Yes.  But I think you could just do that here, too?  I.e., instead of
copying and managing files, you can just write the `C-x v D' diff to a
file, apply reverted diff, apply user-edited patch, commit, reverse
edited patch, apply diff from file.

It seems easier, even -- you don't have to manage a bunch of files.

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

[Dmitry Gutov]

On 31.08.2022 19:21, Dmitry Gutov wrote:
> On 31.08.2022 19:06, Juri Linkov wrote:
>>> But a loop is necessary because the diff can have more than one hunk, 
>>> isn't
>>> it?
>> A loop is necessary indeed.  The problem is that I tried it with Bzr,
>> but it has such a bug that `bzr diff` adds an extra line at the end,
>> so it's not (eobp) after the last diff hunk.  After removing the
>> final newline in the*vc-diff*  buffer the loop can be finished.
> 
> Hmm, so how do we apply the full diff in a backend-agnostic fashion?
> 
> Should we delegate to 'patch', then? And require it to be installed.

I suppose we could check whether every diff-apply-hunk advances point, 
though.

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

[Dmitry Gutov]

On 31.08.2022 19:39, Lars Ingebrigtsen wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> I don't use stashing commands from VC myself, but the stash mechanism
>> is usually more complex, e.g. it usually knows how to merge changes
>> (when the user has edited the file in some place and then pops the
>> stash) and indicate conflicts.
>>
>> That's why a stash is usually represented as a diff, I guess.
> 
> Yes.  But I think you could just do that here, too?  I.e., instead of
> copying and managing files, you can just write the `C-x v D' diff to a
> file, apply reverted diff, apply user-edited patch, commit, reverse
> edited patch, apply diff from file.

If you like, I can take this route as well. It seems a bit more 
error-prone, though: some unknown bug in diff-apply-hunk might make it 
fail at the end, or even corrupt the file contents silently.

'cp' seems more bullet-proof in that regard.

> It seems easier, even -- you don't have to manage a bunch of files.

True.

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

[Sean Whitton]

Hello,

On Tue 30 Aug 2022 at 10:38PM -04, Richard Stallman wrote:

> [[[ To any NSA and FBI agents reading my email: please consider    ]]]
> [[[ whether defending the US Constitution against all enemies,     ]]]
> [[[ foreign or domestic, requires you to follow Snowden's example. ]]]
>
>   > VC is far more flexible and Emacs-y than Magit in how it handles
>   > buffers.  E.g. in Magit if you C-x x u a log buffer, because you want to
>   > log something else at the same time, Magit will re-use the first log
>   > buffer anyway.
>
> Isn't that a bug in Magit?  It could be fixed
> to handle special buffers in the usual Emacs fashion that other packages use.

It may well be a deliberate UI choice.

-- 
Sean Whitton

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

[Sean Whitton]

Hello,

On Wed 31 Aug 2022 at 05:32PM -07, Sean Whitton wrote:

> Hello,
>
> On Tue 30 Aug 2022 at 10:38PM -04, Richard Stallman wrote:
>
>> [[[ To any NSA and FBI agents reading my email: please consider    ]]]
>> [[[ whether defending the US Constitution against all enemies,     ]]]
>> [[[ foreign or domestic, requires you to follow Snowden's example. ]]]
>>
>>   > VC is far more flexible and Emacs-y than Magit in how it handles
>>   > buffers.  E.g. in Magit if you C-x x u a log buffer, because you want to
>>   > log something else at the same time, Magit will re-use the first log
>>   > buffer anyway.
>>
>> Isn't that a bug in Magit?  It could be fixed
>> to handle special buffers in the usual Emacs fashion that other packages use.
>
> It may well be a deliberate UI choice.

Ah, Jonas says downthread it might indeed be under review, apologies.

-- 
Sean Whitton

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

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> Yes.  But I think you could just do that here, too?  I.e., instead
>> of
>> copying and managing files, you can just write the `C-x v D' diff to a
>> file, apply reverted diff, apply user-edited patch, commit, reverse
>> edited patch, apply diff from file.
>
> If you like, I can take this route as well. It seems a bit more
> error-prone, though: some unknown bug in diff-apply-hunk might make it
> fail at the end, or even corrupt the file contents silently.
>
> 'cp' seems more bullet-proof in that regard.

You should be able to get back to the previous state reliably by
reverting to the repository state and then applying the saved patch,
though.

Which is basically what git does with stashes, anyway...

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

[Dmitry Gutov]

On 01.09.2022 13:16, Lars Ingebrigtsen wrote:
> You should be able to get back to the previous state reliably by
> reverting to the repository state and then applying the saved patch,
> though.

The step of "applying the saved patch" relies on our Lisp code being 
correct and being able to handle any diff. It's not an outrageous 
assumption, but an assumption nevertheless.

> Which is basically what git does with stashes, anyway...

Yup. Git is vastly better tested, though.

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

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> The step of "applying the saved patch" relies on our Lisp code being
> correct and being able to handle any diff. It's not an outrageous
> assumption, but an assumption nevertheless.

Oh, I was assuming that we were assuming that we had "patch" installed
for this to work.  I don't think that's unreasonable for new
functionality like this.

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

[Dmitry Gutov]

On 01.09.2022 14:45, Lars Ingebrigtsen wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>> The step of "applying the saved patch" relies on our Lisp code being
>> correct and being able to handle any diff. It's not an outrageous
>> assumption, but an assumption nevertheless.
> Oh, I was assuming that we were assuming that we had "patch" installed
> for this to work.  I don't think that's unreasonable for new
> functionality like this.

I suggested this upthread but didn't get any affirmative response.

This would be easier, yes.

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

[Eli Zaretskii]

> Date: Thu, 1 Sep 2022 14:48:32 +0300
> Cc: Juri Linkov <juri@linkov.net>, Po Lu <luangruo@yahoo.com>,
>  "Alfred M. Szmidt" <ams@gnu.org>, eliz@gnu.org, emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> On 01.09.2022 14:45, Lars Ingebrigtsen wrote:
> > Oh, I was assuming that we were assuming that we had "patch" installed
> > for this to work.  I don't think that's unreasonable for new
> > functionality like this.
> 
> I suggested this upthread but didn't get any affirmative response.
> 
> This would be easier, yes.

If it will be easier, by all means go for it.  I, too, don't think
it's unreasonable.  Even on MS-Windows, there's a port of GNU Patch,
and people who have Git for Windows installed get Patch as part of the
deal.

Thanks.

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

[Robert Pluim]

>>>>> On Thu, 01 Sep 2022 13:45:57 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Dmitry Gutov <dgutov@yandex.ru> writes:
    >> The step of "applying the saved patch" relies on our Lisp code being
    >> correct and being able to handle any diff. It's not an outrageous
    >> assumption, but an assumption nevertheless.

    Lars> Oh, I was assuming that we were assuming that we had "patch" installed
    Lars> for this to work.  I don't think that's unreasonable for new
    Lars> functionality like this.

'git apply' will work as a replacement for 'patch' as well.

Robert
-- 

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

[Dmitry Gutov]

On 01.09.2022 16:26, Robert Pluim wrote:
> 'git apply' will work as a replacement for 'patch' as well.

We're talking about a fallback implementation that will work across 
backends.

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

[Robert Pluim]

>>>>> On Thu, 1 Sep 2022 16:28:09 +0300, Dmitry Gutov <dgutov@yandex.ru> said:

    Dmitry> On 01.09.2022 16:26, Robert Pluim wrote:
    >> 'git apply' will work as a replacement for 'patch' as well.

    Dmitry> We're talking about a fallback implementation that will work across
    Dmitry> backends.

I was too succinct. If the implementation at some point calls for
"patch"-like functionality, but "patch" is not available but "git
apply" is, then we could fall back to "git apply" (and the
equivalent commands for other backends if they exist).

Robert
-- 

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

[Dmitry Gutov]

On 30.08.2022 15:03, Dmitry Gutov wrote:
> On 30.08.2022 10:20, Juri Linkov wrote:
>> I am unfamiliar with Mercurial, but looking at
>> https://www.mercurial-scm.org/wiki/GitConcepts#Git.27s_staging_area
>> where the table says that index/staging area is not necessary in 
>> Mercurial
>> and suggests to use shelve instead that corresponds to Git's stash,
>> it would be possible to do the same with Hg as we first tried to do
>> with Git using stash: put changes to stash/shelve, commit and restore
>> the previous state.
> 
> I suppose it might work, but then what happens when stashed/shelved 
> changes start to conflict with the ones just committed? The stash can 
> pop up with conflict markers.
> 
> Maybe that's okay, I don't know.

Oh BTW 'hg import -m <message> --bypass <filename>' might be the command 
we were looking for.

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

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > 'git apply' will work as a replacement for 'patch' as well.

Will it work with all the back ends that VC supports?
If it only works with git, it is not a very useful fallback.

Note that it would add the requirement for git to be installed.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

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

[Philip Kaludercic]

"Alfred M. Szmidt" <ams@gnu.org> writes:

>    > >     'C-x v v' on a diff buffer commits it as a patch (bug#52349)
>    > 
>    > Excellent!  This is going to save me a lot of work in the future.
>
>    Bother: This is only supported for Git, which is against the spirit of
>    VC, and definitely against the spirit of "C-x v v".
>
> This is a pitty indeed.  Specially seeing that it is easy enough to
> get this working for all VCSs.  Here is a hack I wrote ages ago, and
> this works for just about everything, it doesn't do exactly what is
> needed (e.g, this works on a single file basis) but it isn't too hard
> to get that working.

This feature looks very interesting to me.  Could it be added to vc or
as an package to GNU ELPA?

> ===File ~/diff-commit-hunk.el===============================
> ;;;; diff-commit-hunk.el --- commit parts of a hunk in `diff-mode'
>
> (require 'diff-mode)
>
> (defun current-buffer-file-name ()
>   (buffer-file-name (current-buffer)))
>
> (defun restore-source-file ()
>   (with-current-buffer (current-buffer)
>     (erase-buffer)
>     (insert-buffer "*diff-commit-hunk*")
>     (write-file (current-buffer-file-name)))
>   (remove-hook 'vc-checkin-hook 'restore-source-file))
>
> (defmacro with-original-file (&rest body)
>   "Reverts associated source file temporarily in a `diff-mode'
> buffer to the latest found in VCS, executes BODY and commits the
> changes back VCS."
>   `(progn
>      (save-excursion
>        (diff-goto-source)
>        (let ((buf (current-buffer)))
> 	 (with-current-buffer (get-buffer-create "*diff-commit-hunk*")
> 	   (erase-buffer)
> 	   (insert-buffer buf)))
>        (vc-revert-file (current-buffer-file-name)))
>      ,@body
>      (save-excursion
>        (diff-goto-source)
>        (write-file (current-buffer-file-name))
>        (add-hook 'vc-checkin-hook 'restore-source-file)
>        (vc-checkin (list (current-buffer-file-name))
> 		   (vc-backend (current-buffer-file-name))))))
>
> ;;;###autoload
> (defun diff-commit-hunk ()
>   "Revert associated source file to the latest version from VCS,
> and apply (and commit) current hunk."
>   (interactive)
>   (with-original-file
>    (let ((diff-advance-after-apply-hunk nil))
>      (diff-apply-hunk))))
>
> ;;;###autoload
> (defun diff-commit-all ()
>   "Like `diff-commit-hunk', but applies all hunks in the current
> diff buffer."
>   (interactive)
>   (with-original-file
>    (goto-char (point-min))
>    (diff-hunk-next)			;Skip header.
>    (while (not (eobp))
>      (diff-apply-hunk))))
>
> (define-key diff-mode-map (kbd "s-a") #'diff-commit-hunk)
> (define-key diff-mode-map (kbd "H-a") #'diff-commit-all)
>
> ;;;; diff-commit-hunk.el ends here.
> ============================================================

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

[Alfred M. Szmidt]

     > 'git apply' will work as a replacement for 'patch' as well.

   Will it work with all the back ends that VC supports?
   If it only works with git, it is not a very useful fallback.

It should; `git apply' doesn't require that directory and files it
works on is a git tree for its most basic functionality.  What might
be an issue is if .git/config is used, and any configuration values
are set that change the behaviour of `git apply'.

GNU patch has nicer features than `git apply' too, and is a standard
Unix program, it makes more sense to depend on patch than on `git
apply'.

   Note that it would add the requirement for git to be installed.

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

[Alfred M. Szmidt]

   >    > >     'C-x v v' on a diff buffer commits it as a patch (bug#52349)
   >    > 
   >    > Excellent!  This is going to save me a lot of work in the future.
   >
   >    Bother: This is only supported for Git, which is against the spirit of
   >    VC, and definitely against the spirit of "C-x v v".
   >
   > This is a pitty indeed.  Specially seeing that it is easy enough to
   > get this working for all VCSs.  Here is a hack I wrote ages ago, and
   > this works for just about everything, it doesn't do exactly what is
   > needed (e.g, this works on a single file basis) but it isn't too hard
   > to get that working.

   This feature looks very interesting to me.  Could it be added to vc or
   as an package to GNU ELPA?

Isn't it the same feature as being implemented (in a better manner)?

   > ===File ~/diff-commit-hunk.el===============================
   > ;;;; diff-commit-hunk.el --- commit parts of a hunk in `diff-mode'
   >
   > (require 'diff-mode)
   >
   > (defun current-buffer-file-name ()
   >   (buffer-file-name (current-buffer)))
   >
   > (defun restore-source-file ()
   >   (with-current-buffer (current-buffer)
   >     (erase-buffer)
   >     (insert-buffer "*diff-commit-hunk*")
   >     (write-file (current-buffer-file-name)))
   >   (remove-hook 'vc-checkin-hook 'restore-source-file))
   >
   > (defmacro with-original-file (&rest body)
   >   "Reverts associated source file temporarily in a `diff-mode'
   > buffer to the latest found in VCS, executes BODY and commits the
   > changes back VCS."
   >   `(progn
   >      (save-excursion
   >        (diff-goto-source)
   >        (let ((buf (current-buffer)))
   > 	 (with-current-buffer (get-buffer-create "*diff-commit-hunk*")
   > 	   (erase-buffer)
   > 	   (insert-buffer buf)))
   >        (vc-revert-file (current-buffer-file-name)))
   >      ,@body
   >      (save-excursion
   >        (diff-goto-source)
   >        (write-file (current-buffer-file-name))
   >        (add-hook 'vc-checkin-hook 'restore-source-file)
   >        (vc-checkin (list (current-buffer-file-name))
   > 		   (vc-backend (current-buffer-file-name))))))
   >
   > ;;;###autoload
   > (defun diff-commit-hunk ()
   >   "Revert associated source file to the latest version from VCS,
   > and apply (and commit) current hunk."
   >   (interactive)
   >   (with-original-file
   >    (let ((diff-advance-after-apply-hunk nil))
   >      (diff-apply-hunk))))
   >
   > ;;;###autoload
   > (defun diff-commit-all ()
   >   "Like `diff-commit-hunk', but applies all hunks in the current
   > diff buffer."
   >   (interactive)
   >   (with-original-file
   >    (goto-char (point-min))
   >    (diff-hunk-next)			;Skip header.
   >    (while (not (eobp))
   >      (diff-apply-hunk))))
   >
   > (define-key diff-mode-map (kbd "s-a") #'diff-commit-hunk)
   > (define-key diff-mode-map (kbd "H-a") #'diff-commit-all)
   >
   > ;;;; diff-commit-hunk.el ends here.
   > ============================================================

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

[Philip Kaludercic]

"Alfred M. Szmidt" <ams@gnu.org> writes:

>    >    > >     'C-x v v' on a diff buffer commits it as a patch (bug#52349)
>    >    > 
>    >    > Excellent!  This is going to save me a lot of work in the future.
>    >
>    >    Bother: This is only supported for Git, which is against the spirit of
>    >    VC, and definitely against the spirit of "C-x v v".
>    >
>    > This is a pitty indeed.  Specially seeing that it is easy enough to
>    > get this working for all VCSs.  Here is a hack I wrote ages ago, and
>    > this works for just about everything, it doesn't do exactly what is
>    > needed (e.g, this works on a single file basis) but it isn't too hard
>    > to get that working.
>
>    This feature looks very interesting to me.  Could it be added to vc or
>    as an package to GNU ELPA?
>
> Isn't it the same feature as being implemented (in a better manner)?

If it is then I missed the message, in which case my question should be
ignored.

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

[Dmitry Gutov]

On 31.08.2022 19:36, Eli Zaretskii wrote:
>> Date: Wed, 31 Aug 2022 19:21:12 +0300
>> Cc: Po Lu<luangruo@yahoo.com>, "Alfred M. Szmidt"<ams@gnu.org>,
>>   eliz@gnu.org,larsi@gnus.org,emacs-devel@gnu.org
>> From: Dmitry Gutov<dgutov@yandex.ru>
>>
>> On 31.08.2022 19:06, Juri Linkov wrote:
>>>> But a loop is necessary because the diff can have more than one hunk, isn't
>>>> it?
>>> A loop is necessary indeed.  The problem is that I tried it with Bzr,
>>> but it has such a bug that `bzr diff` adds an extra line at the end,
>>> so it's not (eobp) after the last diff hunk.  After removing the
>>> final newline in the*vc-diff*  buffer the loop can be finished.
>> Hmm, so how do we apply the full diff in a backend-agnostic fashion?
>>
>> Should we delegate to 'patch', then? And require it to be installed.
> Shouldn't the code which looks for hunks be resilient to such minor
> differences?  Why does it fail if there's an extra line at the end?
> Does that line look anything like another hunk?

I'm not sure how that line is supposed to look (I haven't been able to 
reproduce it with the one Bzr repo I had), but it should be possible to 
use a loop like

   (while (or (bobp) (looking-at diff-hunk-header-re)
     (diff-apply-hunk))

Anyway, I went ahead with using 'patch' for now. At least it will show 
simpler errors if user tries to commit patch with broken syntax.

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

[Dmitry Gutov]

On 17.10.2022 02:53, Dmitry Gutov wrote:
> I'm not sure how that line is supposed to look (I haven't been able to 
> reproduce it with the one Bzr repo I had)

Ah, I see it now. It's an extra newline.

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

[Dmitry Gutov]

On 01.09.2022 14:48, Dmitry Gutov wrote:
> On 01.09.2022 14:45, Lars Ingebrigtsen wrote:
>> Dmitry Gutov<dgutov@yandex.ru>  writes:
>>
>>> The step of "applying the saved patch" relies on our Lisp code being
>>> correct and being able to handle any diff. It's not an outrageous
>>> assumption, but an assumption nevertheless.
>> Oh, I was assuming that we were assuming that we had "patch" installed
>> for this to work.  I don't think that's unreasonable for new
>> functionality like this.
> 
> I suggested this upthread but didn't get any affirmative response.
> 
> This would be easier, yes.

So... I ended up using 'patch', but not for backing up existing 
uncommitted changes.

It's the same reason as why 'hg shelve' and 'git stash' are a poor fit: 
when we try to apply the patch at the end, we can get a conflict (and 
not every such conflict will be easy enough to fix by hand).

The file-copying approach matches Git's implementation well, and 
hopefully will work with various backends, including CVS. I only tested 
with Hg for now.

Feedback welcome.

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

[Juri Linkov]

> The file-copying approach matches Git's implementation well, and hopefully
> will work with various backends, including CVS. I only tested with Hg for
> now.
>
> Feedback welcome.

Thanks, confirmed to work with bzr.

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

[Dmitry Gutov]

On 30.08.2022 16:42, Po Lu wrote:
> I would, but currently I'm heavily occupied with some other code (which
> happens to use Subversion), and the feature would be quite convenient.

I'm curious whether you've had the chance to test it with SVN by now.