GNU bug report logs - #60126
30.0.50; vc-git-checkin: Offer to unstage conflicting changes

Previous Next

Full log

Message #73 received at 60126-done <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 60126-done <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting
 changes
Date: Sat, 24 Dec 2022 16:50:55 +0200

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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.