GNU bug report logs -
#60126
30.0.50; vc-git-checkin: Offer to unstage conflicting changes
Previous Next
Reported by: Sean Whitton <spwhitton <at> spwhitton.name>
Date: Fri, 16 Dec 2022 18:34:01 UTC
Severity: normal
Found in version 30.0.50
Done: Sean Whitton <spwhitton <at> spwhitton.name>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 60126 in the body.
You can then email your comments to 60126 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
juri <at> linkov.net, dgutov <at> yandex.ru, bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Fri, 16 Dec 2022 18:34:01 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Sean Whitton <spwhitton <at> spwhitton.name>:
New bug report received and forwarded. Copy sent to
juri <at> linkov.net, dgutov <at> yandex.ru, bug-gnu-emacs <at> gnu.org.
(Fri, 16 Dec 2022 18:34:01 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
X-debbugs-cc: juri <at> linkov.net, dgutov <at> yandex.ru
I seem often to end up with a nonempty index when trying to commit patches
using C-x v v from diff-mode, so I came up with this patch.
--
Sean Whitton
[0001-vc-git-checkin-Offer-to-unstage-conflicting-changes.patch (text/x-diff, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Sat, 17 Dec 2022 17:28:04 GMT)
Full text and
rfc822 format available.
Message #8 received at 60126 <at> debbugs.gnu.org (full text, mbox):
> I seem often to end up with a nonempty index when trying to commit patches
> using C-x v v from diff-mode, so I came up with this patch.
From the version 30.0.50 in the subject do I correctly infer
that you propose this patch for master? In master this
could be installed immediately, but for the release branch
I'd prefer first to test it extensively.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Sun, 18 Dec 2022 00:21:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 60126 <at> debbugs.gnu.org (full text, mbox):
Hello,
On Sat 17 Dec 2022 at 07:06PM +02, Juri Linkov wrote:
>> I seem often to end up with a nonempty index when trying to commit patches
>> using C-x v v from diff-mode, so I came up with this patch.
>
> From the version 30.0.50 in the subject do I correctly infer
> that you propose this patch for master? In master this
> could be installed immediately, but for the release branch
> I'd prefer first to test it extensively.
Yes. It's a new feature, so I don't see how it could go on the release
branch?
--
Sean Whitton
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Sun, 18 Dec 2022 01:09:03 GMT)
Full text and
rfc822 format available.
Message #14 received at 60126 <at> debbugs.gnu.org (full text, mbox):
On 16/12/2022 20:32, Sean Whitton wrote:
> I seem often to end up with a nonempty index when trying to commit patches
> using C-x v v from diff-mode, so I came up with this patch.
Thanks!
Perhaps we should offer to stash the changes, though?
Starting with Git 2.13, 'git stash' now has the 'push' subcommand which
accepts individual file names:
https://stackoverflow.com/a/5506483/615245
Or make it a three-way choice with read-multiple-choice.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Mon, 19 Dec 2022 22:31:01 GMT)
Full text and
rfc822 format available.
Message #17 received at 60126 <at> debbugs.gnu.org (full text, mbox):
Hello,
On Sun 18 Dec 2022 at 03:08AM +02, Dmitry Gutov wrote:
> Perhaps we should offer to stash the changes, though?
>
> Starting with Git 2.13, 'git stash' now has the 'push' subcommand which
> accepts individual file names:
>
> https://stackoverflow.com/a/5506483/615245
>
> Or make it a three-way choice with read-multiple-choice.
This is a nice suggestion. A step further would be to unconditionally
stash and unstash. Given how committing patches with C-x v v works, I
don't believe it can ever be the case that the stash is not applicable
afterwards? If that's wrong, I'll implement what you suggest.
--
Sean Whitton
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Tue, 20 Dec 2022 00:54:02 GMT)
Full text and
rfc822 format available.
Message #20 received at 60126 <at> debbugs.gnu.org (full text, mbox):
On 20/12/2022 00:30, Sean Whitton wrote:
> This is a nice suggestion. A step further would be to unconditionally
> stash and unstash. Given how committing patches with C-x v v works, I
> don't believe it can ever be the case that the stash is not applicable
> afterwards?
I'm not sure that's 100% true, given that we'll want to stage the
contents of the staging area (which are supposedly represented as diffs
against the last committed state), and our command, while keeping the
contents of files on disk intact, moves the last commit to a new state.
> If that's wrong, I'll implement what you suggest.
...but we might as well try and experiment. Worst case: the stash won't
apply cleanly and the user will have to do it by hand. That would mean
no big loss of information, at least.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Tue, 20 Dec 2022 06:44:02 GMT)
Full text and
rfc822 format available.
Message #23 received at 60126 <at> debbugs.gnu.org (full text, mbox):
Hello,
On Tue 20 Dec 2022 at 02:53AM +02, Dmitry Gutov wrote:
> On 20/12/2022 00:30, Sean Whitton wrote:
>> This is a nice suggestion. A step further would be to unconditionally
>> stash and unstash. Given how committing patches with C-x v v works, I
>> don't believe it can ever be the case that the stash is not applicable
>> afterwards?
>
> I'm not sure that's 100% true, given that we'll want to stage the contents of
> the staging area (which are supposedly represented as diffs against the last
> committed state), and our command, while keeping the contents of files on disk
> intact, moves the last commit to a new state.
Good point about the representation of the index.
In addition, you can't stash just the content of the staging area, and
not also the working tree, without hacks -- see `magit-stash-save' as
called by `magit-stash-index'. I don't want to reproduce those hacks,
so I was thinking we would be stashing both the worktree and the index
for any and all files that have any changes staged. However ...
>> If that's wrong, I'll implement what you suggest.
>
> ...but we might as well try and experiment. Worst case: the stash won't apply
> cleanly and the user will have to do it by hand. That would mean no big loss
> of information, at least.
... for files that are modified by the patch to be committed, it's very
easy to generate situations where git can't or won't apply the stash.
It's true that there isn't a loss of information, but it's a frustrating
context switch for the user, who might not have even realised a stash
would be created, and now has to untangle things.
So, I'm now thinking:
- automatically stash index+worktree for any files with changes staged
that are *not* modified by the patch to be committed
- offer to unstage any files with changes staged that *are* modified by
the patch to be committed.
This way, we have to prompt the user only when files involved in the
patch have staged changes, and we shouldn't ever force the user into
dealing with a stash that won't apply.
--
Sean Whitton
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Tue, 20 Dec 2022 13:48:01 GMT)
Full text and
rfc822 format available.
Message #26 received at 60126 <at> debbugs.gnu.org (full text, mbox):
On 20/12/2022 08:43, Sean Whitton wrote:
> In addition, you can't stash just the content of the staging area, and
> not also the working tree, without hacks
I think
git stash push --staged -- file1 file2 ...
does that.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Tue, 20 Dec 2022 15:14:01 GMT)
Full text and
rfc822 format available.
Message #29 received at 60126 <at> debbugs.gnu.org (full text, mbox):
On 20/12/2022 08:43, Sean Whitton wrote:
> So, I'm now thinking:
>
> - automatically stash index+worktree for any files with changes staged
> that are*not* modified by the patch to be committed
I think it's possible to just skip those when checking the index area.
And then, when committing, specify individual files to commit from the
index.
E.g. this way:
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index b5959d535c0..dee102d8586 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1052,7 +1052,7 @@ vc-git-checkin
(lambda (value) (when (equal value "yes") (list
argument)))))
;; When operating on the whole tree, better pass "-a" than ".",
since "."
;; fails when we're committing a merge.
- (apply #'vc-git-command nil 0 (if (and only (not
vc-git-patch-string)) files)
+ (apply #'vc-git-command nil 0 (if only files)
(nconc (if msg-file (list "commit" "-F"
(file-local-name msg-file))
(list "commit" "-m"))
(I'm not sure if the list of files is passed to this function correctly
when committing a patch; if not, fixing that would also be needed.)
> - offer to unstage any files with changes staged that*are* modified by
> the patch to be committed.
Or we could just abort, like we do now. Up to you (do you encounter this
particular situation often?).
These could be two separate changes anyway.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Tue, 20 Dec 2022 16:48:02 GMT)
Full text and
rfc822 format available.
Message #32 received at 60126 <at> debbugs.gnu.org (full text, mbox):
Hello,
On Tue 20 Dec 2022 at 03:47PM +02, Dmitry Gutov wrote:
> On 20/12/2022 08:43, Sean Whitton wrote:
>> In addition, you can't stash just the content of the staging area, and
>> not also the working tree, without hacks
>
> I think
>
> git stash push --staged -- file1 file2 ...
>
> does that.
Ah nice, though, my git doesn't have that option (Debian stable).
--
Sean Whitton
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Tue, 20 Dec 2022 17:09:02 GMT)
Full text and
rfc822 format available.
Message #35 received at 60126 <at> debbugs.gnu.org (full text, mbox):
Hello,
On Tue 20 Dec 2022 at 05:13PM +02, Dmitry Gutov wrote:
> On 20/12/2022 08:43, Sean Whitton wrote:
>> So, I'm now thinking:
>> - automatically stash index+worktree for any files with changes staged
>> that are*not* modified by the patch to be committed
>
> I think it's possible to just skip those when checking the index area. And
> then, when committing, specify individual files to commit from the index.
Ah, so it is!
> E.g. this way:
>
> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index b5959d535c0..dee102d8586 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -1052,7 +1052,7 @@ vc-git-checkin
> (lambda (value) (when (equal value "yes") (list argument)))))
> ;; When operating on the whole tree, better pass "-a" than ".", since
> "."
> ;; fails when we're committing a merge.
> - (apply #'vc-git-command nil 0 (if (and only (not vc-git-patch-string))
> files)
> + (apply #'vc-git-command nil 0 (if only files)
> (nconc (if msg-file (list "commit" "-F"
> (file-local-name msg-file))
> (list "commit" "-m"))
>
> (I'm not sure if the list of files is passed to this function correctly when
> committing a patch; if not, fixing that would also be needed.)
It's not usually passed, but it could be. So I think I need to clear
out files and populate it again during the loop through
vc-git-patch-string. We'll want this always to happen, so in the commit
command we'll want (and (or only vc-git-patch-string) files).
>> - offer to unstage any files with changes staged that*are* modified by
>> the patch to be committed.
>
> Or we could just abort, like we do now. Up to you (do you encounter this
> particular situation often?).
I feel like I do, yes, though I'm not completely sure which of the two
situations I keep finding myself in. It does no harm to have the
feature, at least.
Thank you for the input.
--
Sean Whitton
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Tue, 20 Dec 2022 17:17:01 GMT)
Full text and
rfc822 format available.
Message #38 received at 60126 <at> debbugs.gnu.org (full text, mbox):
> (I'm not sure if the list of files is passed to this function correctly
> when committing a patch; if not, fixing that would also be needed.)
A list of files should be correct, but needs more testing to confirm.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Tue, 20 Dec 2022 23:11:02 GMT)
Full text and
rfc822 format available.
Message #41 received at 60126 <at> debbugs.gnu.org (full text, mbox):
Hello,
On Tue 20 Dec 2022 at 10:04AM -07, Sean Whitton wrote:
> On Tue 20 Dec 2022 at 05:13PM +02, Dmitry Gutov wrote:
>
>> On 20/12/2022 08:43, Sean Whitton wrote:
>>> So, I'm now thinking:
>>> - automatically stash index+worktree for any files with changes staged
>>> that are*not* modified by the patch to be committed
>>
>> I think it's possible to just skip those when checking the index area. And
>> then, when committing, specify individual files to commit from the index.
>
> Ah, so it is!
It turns out this doesn't work: if you specify individual files to
commit from the index, then git also includes any changes to those files
in the worktree too, and you can't ask it not to, because --only is
implied whenever files are supplied on the command line.
In other words, when vc-git-patch-string is non-nil, we mustn't pass a
list of files to git. So if there are files not involved in the commit
with staged changes, we need to stash those.
I tried implementing that, which is not hard, but then we pop that
stash, the staged changes aren't restored to the index. The result is
that if the user has a mixture of staged and unstaged changes to a file
which is not part of the commit, then afterwards the unstaged changes
will have been unstaged, mixed in with the staged changes again. In
some circumstances this could constitute a loss of work.
There are a few ways to overcome this. We can use the --staged option,
but that's only available in very recent versions of git. We could
perform a complex double-stash:
- git stash push -k -- foo
- git reset -- foo
- git stash push -k -- foo
- [commit]
- git stash pop
- git add -- foo
- git stash pop
Or we could do something like what Magit does to stash only the index.
Any thoughts on what would be best?
In the meantime, I've pushed a version of my previous patch, as I think
it makes sense to implement the stashing, if we do, as a second change.
--
Sean Whitton
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Tue, 20 Dec 2022 23:42:02 GMT)
Full text and
rfc822 format available.
Message #44 received at 60126 <at> debbugs.gnu.org (full text, mbox):
Hello,
On Tue 20 Dec 2022 at 04:10PM -07, Sean Whitton wrote:
> ... afterwards the unstaged changes
> will have been unstaged, mixed in with the staged changes again.
afterwards the staged changes will have been unstaged, mixed in with the
unstaged changes again*
--
Sean Whitton
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Tue, 20 Dec 2022 23:46:01 GMT)
Full text and
rfc822 format available.
Message #47 received at 60126 <at> debbugs.gnu.org (full text, mbox):
On 21/12/2022 01:10, Sean Whitton wrote:
> I tried implementing that, which is not hard, but then we pop that
> stash, the staged changes aren't restored to the index. The result is
> that if the user has a mixture of staged and unstaged changes to a file
> which is not part of the commit, then afterwards the unstaged changes
> will have been unstaged, mixed in with the staged changes again. In
> some circumstances this could constitute a loss of work.
>
> There are a few ways to overcome this. We can use the --staged option,
> but that's only available in very recent versions of git.
IIUC the --staged option is indeed limited to the very new Git, but that
option is used when creating a stash (when we want to stash the staging
area only).
When restoring a stash, to reinstate the stashed index, you would use
the option --index. It's older than --staged (e.g. it's available in Git
2.22.0, and that's as far back as the docs at git-scm.com/docs go). Not
sure if it's in Debian Stable or not.
Regarding the alternatives -- double stashing, or the Magit way, it's
hard to form a strong opinion before examining them in detail (I trust
you can make a good choice).
For completeness, though, here's a way to implement 'git push --staged'
with Git plumbing manually: https://stackoverflow.com/a/72582276/615245
And as for a 'git pop --index' substitute, if the stash contains only
the index area stuff, it might be as easy as
git diff stash@{0}^..stash@{0} > patch.diff
git apply --cached patch.diff
git stash drop
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Fri, 23 Dec 2022 00:13:01 GMT)
Full text and
rfc822 format available.
Message #50 received at 60126 <at> debbugs.gnu.org (full text, mbox):
Hello,
On Wed 21 Dec 2022 at 01:45AM +02, Dmitry Gutov wrote:
> IIUC the --staged option is indeed limited to the very new Git, but that
> option is used when creating a stash (when we want to stash the staging area
> only).
>
> When restoring a stash, to reinstate the stashed index, you would use the
> option --index. It's older than --staged (e.g. it's available in Git 2.22.0,
> and that's as far back as the docs at git-scm.com/docs go). Not sure if it's
> in Debian Stable or not.
Ah, thanks for reminding me, I was getting mixed up.
Unfortunately it's probably not much use, because 'git stash push -- x'
stashes all staged changes, it turns out, not just those in x.
> Regarding the alternatives -- double stashing, or the Magit way, it's
> hard to form a strong opinion before examining them in detail (I trust
> you can make a good choice).
>
> For completeness, though, here's a way to implement 'git push --staged' with
> Git plumbing manually: https://stackoverflow.com/a/72582276/615245
>
> And as for a 'git pop --index' substitute, if the stash contains only the
> index area stuff, it might be as easy as
>
> git diff stash@{0}^..stash@{0} > patch.diff
> git apply --cached patch.diff
> git stash drop
These references are helpful. I'll investigate further.
--
Sean Whitton
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Fri, 23 Dec 2022 04:01:02 GMT)
Full text and
rfc822 format available.
Message #53 received at 60126 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hello,
On Thu 22 Dec 2022 at 05:12PM -07, Sean Whitton wrote:
>> For completeness, though, here's a way to implement 'git push --staged' with
>> Git plumbing manually: https://stackoverflow.com/a/72582276/615245
>>
>> And as for a 'git pop --index' substitute, if the stash contains only the
>> index area stuff, it might be as easy as
>>
>> git diff stash@{0}^..stash@{0} > patch.diff
>> git apply --cached patch.diff
>> git stash drop
>
> These references are helpful. I'll investigate further.
Here is my patch.
It works, except that sometimes the let-binding of process-environment
fails, such that the commands affect the normal index rather than the
temporary index. Can you see what I'm doing wrong there?
Thanks.
--
Sean Whitton
[0001-vc-git-checkin-Stash-other-staged-changes.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Fri, 23 Dec 2022 08:18:01 GMT)
Full text and
rfc822 format available.
Message #56 received at 60126 <at> debbugs.gnu.org (full text, mbox):
> Cc: 60126 <at> debbugs.gnu.org, juri <at> linkov.net
> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Date: Thu, 22 Dec 2022 20:59:53 -0700
>
> + ;; Prepare stash commit object, which has a special structure.
> + (let* ((tree-commit (git-string "commit-tree" "-m" message
Can 'message' include newlines? if so, we need to either convert the
newlines to spaces, or invoke the command with -F instead, writing the
message to a temporary file. Because otherwise this will fail on
Windows.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Fri, 23 Dec 2022 22:56:02 GMT)
Full text and
rfc822 format available.
Message #59 received at 60126 <at> debbugs.gnu.org (full text, mbox):
On 23/12/2022 02:12, Sean Whitton wrote:
> Unfortunately it's probably not much use, because 'git stash push -- x'
> stashes all staged changes, it turns out, not just those in x.
Seems to work fine over here.
And the manual says:
<pathspec>...
This option is only valid for push command.
The new stash entry records the modified states only for the
files that match the pathspec. The index entries and working tree files
are then rolled back to the state in HEAD only for
these files, too, leaving files that do not match the pathspec
intact.
For more details, see the pathspec entry in gitglossary(7).
Check out 'man git stash', perhaps your version of Git just doesn't have
that feature yet. Though I thought it came with the introduction of 'git
stash push', as opposed to 'git stash save'.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Fri, 23 Dec 2022 23:19:02 GMT)
Full text and
rfc822 format available.
Message #62 received at 60126 <at> debbugs.gnu.org (full text, mbox):
On 23/12/2022 05:59, Sean Whitton wrote:
> It works, except that sometimes the let-binding of process-environment
> fails, such that the commands affect the normal index rather than the
> temporary index. Can you see what I'm doing wrong there?
Could you describe be "sometimes" occurrences? Does that happen through
repeating a similar action several times? Or slightly different actions?
The process-environment setup seems fine. We did corrupt it in 1-2
places in the past using 'setenv', but I don't see anything like that in
the current codebase. And the effect would probably be different anyway.
Reply sent
to
Sean Whitton <spwhitton <at> spwhitton.name>:
You have taken responsibility.
(Sat, 24 Dec 2022 02:03:01 GMT)
Full text and
rfc822 format available.
Notification sent
to
Sean Whitton <spwhitton <at> spwhitton.name>:
bug acknowledged by developer.
(Sat, 24 Dec 2022 02:03:02 GMT)
Full text and
rfc822 format available.
Message #67 received at 60126-done <at> debbugs.gnu.org (full text, mbox):
Hello,
On Sat 24 Dec 2022 at 01:18AM +02, Dmitry Gutov wrote:
> On 23/12/2022 05:59, Sean Whitton wrote:
>> It works, except that sometimes the let-binding of process-environment
>> fails, such that the commands affect the normal index rather than the
>> temporary index. Can you see what I'm doing wrong there?
>
> Could you describe be "sometimes" occurrences? Does that happen through
> repeating a similar action several times? Or slightly different actions?
>
> The process-environment setup seems fine. We did corrupt it in 1-2 places in
> the past using 'setenv', but I don't see anything like that in the current
> codebase. And the effect would probably be different anyway.
Thank you for looking. Slightly embarassingly, I can't reproduce the
problem today. So I've gone ahead and pushed.
I am pretty happy with this approach, in the end. Compared with other
possible uses of 'git stash', it's quite clean:
- it doesn't touch the worktree copies of the files not involved in the
commit
- it stashes a single diff, rather than two diffs (one for the worktree
and one for the index), which is less for the user to deal with if
manual recovery becomes required.
Thanks again for the helpful discussion on this one.
--
Sean Whitton
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Sat, 24 Dec 2022 02:04:01 GMT)
Full text and
rfc822 format available.
Message #70 received at 60126 <at> debbugs.gnu.org (full text, mbox):
Hello,
On Fri 23 Dec 2022 at 10:16AM +02, Eli Zaretskii wrote:
>> Cc: 60126 <at> debbugs.gnu.org, juri <at> linkov.net
>> From: Sean Whitton <spwhitton <at> spwhitton.name>
>> Date: Thu, 22 Dec 2022 20:59:53 -0700
>>
>> + ;; Prepare stash commit object, which has a special structure.
>> + (let* ((tree-commit (git-string "commit-tree" "-m" message
>
> Can 'message' include newlines? if so, we need to either convert the
> newlines to spaces, or invoke the command with -F instead, writing the
> message to a temporary file. Because otherwise this will fail on
> Windows.
Ah, thank you.
These "messages" are more like labels, or names, than commit messages.
I've gone ahead and hard coded it for the time being.
--
Sean Whitton
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Sat, 24 Dec 2022 14:52:02 GMT)
Full text and
rfc822 format available.
Message #73 received at 60126-done <at> debbugs.gnu.org (full text, mbox):
On 24/12/2022 04:02, Sean Whitton wrote:
> Hello,
>
> On Sat 24 Dec 2022 at 01:18AM +02, Dmitry Gutov wrote:
>
>> On 23/12/2022 05:59, Sean Whitton wrote:
>>> It works, except that sometimes the let-binding of process-environment
>>> fails, such that the commands affect the normal index rather than the
>>> temporary index. Can you see what I'm doing wrong there?
>> Could you describe be "sometimes" occurrences? Does that happen through
>> repeating a similar action several times? Or slightly different actions?
>>
>> The process-environment setup seems fine. We did corrupt it in 1-2 places in
>> the past using 'setenv', but I don't see anything like that in the current
>> codebase. And the effect would probably be different anyway.
> Thank you for looking. Slightly embarassingly, I can't reproduce the
> problem today. So I've gone ahead and pushed.
Thanks!
> I am pretty happy with this approach, in the end. Compared with other
> possible uses of 'git stash', it's quite clean:
>
> - it doesn't touch the worktree copies of the files not involved in the
> commit
>
> - it stashes a single diff, rather than two diffs (one for the worktree
> and one for the index), which is less for the user to deal with if
> manual recovery becomes required.
It does indeed feel like we ended up in a good place. The code was
pretty complex, though, and got more so.
We don't have tests covering vc-git-checkin-patch at all. Any chance
you'll fancy working on adding those? Even if you only cover the
scenarios where the user doesn't get prompted at all (either there's
nothing staged, or the staged contents match the committed patch).
Writing (and debugging) a test could also help sort out fiddly
behaviors, like the one you may have seen yesterday.
We have a default implementation for checkin-patch, so adding generic
test for all major backends could work (in vc-tests.el). One or two
extra tests could be also predicated on (eq backend 'Git), so that we
don't yet need to copy the repository setup/teardown code to vc-git.el.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Sat, 24 Dec 2022 18:23:02 GMT)
Full text and
rfc822 format available.
Message #76 received at 60126-done <at> debbugs.gnu.org (full text, mbox):
Hello,
On Sat 24 Dec 2022 at 04:50PM +02, Dmitry Gutov wrote:
> It does indeed feel like we ended up in a good place. The code was pretty
> complex, though, and got more so.
>
> We don't have tests covering vc-git-checkin-patch at all. Any chance you'll
> fancy working on adding those? Even if you only cover the scenarios where the
> user doesn't get prompted at all (either there's nothing staged, or the staged
> contents match the committed patch).
>
> Writing (and debugging) a test could also help sort out fiddly behaviors, like
> the one you may have seen yesterday.
>
> We have a default implementation for checkin-patch, so adding generic test for
> all major backends could work (in vc-tests.el). One or two extra tests could
> be also predicated on (eq backend 'Git), so that we don't yet need to copy the
> repository setup/teardown code to vc-git.el.
Now that we understand clearly what we want it to do, I bet the code in
vc-checkin-git could be simplified (vc-git--stash-staged-changes is
fine). So I'll see about doing that, with some tests, as you suggest.
Let me ask you about the parsing of the 'diff --git' lines. I wasn't
happy with my regexp approach to extracting the filename. I'm not sure
it can actually fail, but the current codes assumes it can, and that
adds complexity.
The --src-prefix, --dst-prefix and --no-prefix options to git-diff(1)
might be relevant, but then we couldn't use a simple string-match to
find hunks in vc-git-patch-string.
Do you have any better ideas of how to extract the filename from the git
--diff line, or perhaps a proof that my approach can't fail? :)
--
Sean Whitton
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Sat, 24 Dec 2022 18:24:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Sat, 24 Dec 2022 19:27:02 GMT)
Full text and
rfc822 format available.
Message #82 received at 60126-done <at> debbugs.gnu.org (full text, mbox):
On 24/12/2022 20:22, Sean Whitton wrote:
> Now that we understand clearly what we want it to do, I bet the code in
> vc-checkin-git could be simplified (vc-git--stash-staged-changes is
> fine). So I'll see about doing that, with some tests, as you suggest.
>
> Let me ask you about the parsing of the 'diff --git' lines.
...
> Do you have any better ideas of how to extract the filename from the git
> --diff line, or perhaps a proof that my approach can't fail? :)
I don't know. You could try
(and (looking-at diff-file-header-re) (match-string 1))
instead, but it matches a different line (one that starts with "---").
> I wasn't
> happy with my regexp approach to extracting the filename. I'm not sure
> it can actually fail, but the current codes assumes it can, and that
> adds complexity.
Not sure which failure you are referring to, but the process of removing
already-staged hunks from vc-git-patch-string can indeed fail, because
the hunks might be staged, or might be not. The idea was to support both
situations.
> The --src-prefix, --dst-prefix and --no-prefix options to git-diff(1)
> might be relevant, but then we couldn't use a simple string-match to
> find hunks in vc-git-patch-string.
Right.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#60126; Package
emacs.
(Sat, 24 Dec 2022 20:11:02 GMT)
Full text and
rfc822 format available.
Message #85 received at 60126-done <at> debbugs.gnu.org (full text, mbox):
Hello,
On Sat 24 Dec 2022 at 09:26PM +02, Dmitry Gutov wrote:
> On 24/12/2022 20:22, Sean Whitton wrote:
>
>> Now that we understand clearly what we want it to do, I bet the code in
>> vc-checkin-git could be simplified (vc-git--stash-staged-changes is
>> fine). So I'll see about doing that, with some tests, as you suggest.
>> Let me ask you about the parsing of the 'diff --git' lines.
> ...
>> Do you have any better ideas of how to extract the filename from the git
>> --diff line, or perhaps a proof that my approach can't fail? :)
>
> I don't know. You could try
>
> (and (looking-at diff-file-header-re) (match-string 1))
>
> instead, but it matches a different line (one that starts with "---").
That seems better, thank you.
>> I wasn't happy with my regexp approach to extracting the filename.
>> I'm not sure it can actually fail, but the current codes assumes it
>> can, and that adds complexity.
>
> Not sure which failure you are referring to, but the process of removing
> already-staged hunks from vc-git-patch-string can indeed fail, because the
> hunks might be staged, or might be not. The idea was to support both
> situations.
I meant that my regexp might fail to correctly parse the 'diff --git'
line, and then the code goes straight to "Index not empty."
>> The --src-prefix, --dst-prefix and --no-prefix options to git-diff(1)
>> might be relevant, but then we couldn't use a simple string-match to
>> find hunks in vc-git-patch-string.
>
> Right.
>
--
Sean Whitton
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Sun, 22 Jan 2023 12:24:05 GMT)
Full text and
rfc822 format available.
This bug report was last modified 2 years and 149 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.