'git stash push' isn't atomic when Ctrl-C is pressed

'git stash push' isn't atomic when Ctrl-C is pressed

[Yuri]

Ctrl-C was pressed in the middle. git creates the stash record and 
didn't update the files.


Expected behavior: Ctrl-C should cleanly roll back the operation.



Yuri

Re: 'git stash push' isn't atomic when Ctrl-C is pressed

[Ævar Arnfjörð Bjarmason]

On Tue, Jan 25 2022, Yuri wrote:

> Ctrl-C was pressed in the middle. git creates the stash record and
> didn't update the files.
>
>
> Expected behavior: Ctrl-C should cleanly roll back the operation.

Yes, you're right. It really should be fixed.

It's a known issue with builtin/stash.c being written in C, but really
only still being a faithful conversion of the code we had in a
git-stash.sh shellscript until relatively recently.

(No fault of those doing the conversion, that's always the logical first
step).

So it modifies various refs, reflogs etc., but does so mostly via
shelling out to other git commands, whereas it really should be moved to
using the ref transaction API.

Ig you or anyone else is interested in would be a most welcome project
to work on...

Re: 'git stash push' isn't atomic when Ctrl-C is pressed

[John Cai]

On 26 Jan 2022, at 8:41, Ævar Arnfjörð Bjarmason wrote:

> On Tue, Jan 25 2022, Yuri wrote:
>
>> Ctrl-C was pressed in the middle. git creates the stash record and
>> didn't update the files.
>>
>>
>> Expected behavior: Ctrl-C should cleanly roll back the operation.
>
> Yes, you're right. It really should be fixed.
>
> It's a known issue with builtin/stash.c being written in C, but really
> only still being a faithful conversion of the code we had in a
> git-stash.sh shellscript until relatively recently.
>
> (No fault of those doing the conversion, that's always the logical first
> step).
>
> So it modifies various refs, reflogs etc., but does so mostly via
> shelling out to other git commands, whereas it really should be moved to
> using the ref transaction API.
>
> Ig you or anyone else is interested in would be a most welcome project
> to work on…

I’d be happy to help with this!

Re: 'git stash push' isn't atomic when Ctrl-C is pressed

[Ævar Arnfjörð Bjarmason]

On Wed, Jan 26 2022, John Cai wrote:

> On 26 Jan 2022, at 8:41, Ævar Arnfjörð Bjarmason wrote:
>
>> On Tue, Jan 25 2022, Yuri wrote:
>>
>>> Ctrl-C was pressed in the middle. git creates the stash record and
>>> didn't update the files.
>>>
>>>
>>> Expected behavior: Ctrl-C should cleanly roll back the operation.
>>
>> Yes, you're right. It really should be fixed.
>>
>> It's a known issue with builtin/stash.c being written in C, but really
>> only still being a faithful conversion of the code we had in a
>> git-stash.sh shellscript until relatively recently.
>>
>> (No fault of those doing the conversion, that's always the logical first
>> step).
>>
>> So it modifies various refs, reflogs etc., but does so mostly via
>> shelling out to other git commands, whereas it really should be moved to
>> using the ref transaction API.
>>
>> Ig you or anyone else is interested in would be a most welcome project
>> to work on…
>
> I’d be happy to help with this!

Thanks. I think that would be a great thing to work on.

Part of it is summarized in my 212631ed50a (refs/files: remove unused
REF_DELETING in lock_ref_oid_basic(), 2021-07-20). I think there was a
better summary somewhere (maybe an exchange with Jeff King?) but I can't
find it now.

There's a further summary of the remaining callers in 52106430dc8
(refs/files: remove "name exist?" check in lock_ref_oid_basic(),
2021-10-16), which as we'll see is closely coupled with these remaining
transaction-less refs API callers.

Beginning to fix that is as basic as starting with some light move of
existing code into library form, e.g. when you "git stash drop" it will
invoke:

    18:29:02.800983 git.c:459               trace: built-in: git reflog delete --updateref --rewrite 'refs/stash@{0}'

Which will lock a ref with:
    
    (gdb) bt
    #0  BUG_fl (file=0x7965ef "refs/files-backend.c", line=1011, fmt=0x796f30 "hi") at usage.c:324
    #1  0x0000000000675488 in lock_ref_oid_basic (refs=0x855470, refname=0x855140 "refs/stash", err=0x7fffffffd9b0) at refs/files-backend.c:1011
    #2  0x00000000006722bb in files_reflog_expire (ref_store=0x855470, refname=0x855140 "refs/stash", expire_flags=6, prepare_fn=0x4c9390 <reflog_expiry_prepare>,
        should_prune_fn=0x4c8c40 <should_expire_reflog_ent>, cleanup_fn=0x4c9510 <reflog_expiry_cleanup>, policy_cb_data=0x7fffffffdb30) at refs/files-backend.c:3159
    #3  0x000000000066d832 in refs_reflog_expire (refs=0x855470, refname=0x855140 "refs/stash", flags=6, prepare_fn=0x4c9390 <reflog_expiry_prepare>,
        should_prune_fn=0x4c8c40 <should_expire_reflog_ent>, cleanup_fn=0x4c9510 <reflog_expiry_cleanup>, policy_cb_data=0x7fffffffdb30) at refs.c:2411
    #4  0x000000000066d88f in reflog_expire (refname=0x855140 "refs/stash", flags=6, prepare_fn=0x4c9390 <reflog_expiry_prepare>, should_prune_fn=0x4c8c40 <should_expire_reflog_ent>,
        cleanup_fn=0x4c9510 <reflog_expiry_cleanup>, policy_cb_data=0x7fffffffdb30) at refs.c:2423
    #5  0x00000000004c8ad7 in cmd_reflog_delete (argc=1, argv=0x7fffffffe118, prefix=0x0) at builtin/reflog.c:780
    #6  0x00000000004c7abb in cmd_reflog (argc=5, argv=0x7fffffffe110, prefix=0x0) at builtin/reflog.c:839
    #7  0x0000000000407c8b in run_builtin (p=0x81e0e0 <commands+2304>, argc=5, argv=0x7fffffffe110) at git.c:465
    #8  0x0000000000406742 in handle_builtin (argc=5, argv=0x7fffffffe110) at git.c:719
    #9  0x0000000000407676 in run_argv (argcp=0x7fffffffdfcc, argv=0x7fffffffdfc0) at git.c:786
    #10 0x0000000000406501 in cmd_main (argc=5, argv=0x7fffffffe110) at git.c:917
    #11 0x0000000000510fba in main (argc=6, argv=0x7fffffffe108) at common-main.c:56

Which, if you chase that down to builtin/reflog.c you can see is just a
case where we could avoid the sub-process invocation by calling
reflog_expire() ourselves in builtin/stash.c, perhps after lib-ifying
some of what it's doing into a new top-level reflog.c, and having the
command-line specific stuff in builtin/reflog.c call that.

You've hacked on that code recently, so that should be easy to start
with :)

Then once we do all the ref updates in-process it should be easy (or
easy enough...) to move it over to the ref transaction API. So if we
fail we'll roll everything back properly.

The eventual approximate goal should be to get rid of
lock_ref_oid_basic() entirely, it's legacy API in the refs files backend
that makes a lot of things over there more complex. Some other users of
it can be found with:

    git grep -w -e copy_ref -e rename_ref

Or, by just removing the function recursively during testing, and
following the compilation errors.

Re: 'git stash push' isn't atomic when Ctrl-C is pressed

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Tue, Jan 25 2022, Yuri wrote:
>
>> Ctrl-C was pressed in the middle. git creates the stash record and
>> didn't update the files.
>>
>>
>> Expected behavior: Ctrl-C should cleanly roll back the operation.
>
> Yes, you're right. It really should be fixed.
>
> It's a known issue with builtin/stash.c being written in C, but really
> only still being a faithful conversion of the code we had in a
> git-stash.sh shellscript until relatively recently.
>
> (No fault of those doing the conversion, that's always the logical first
> step).
>
> So it modifies various refs, reflogs etc., but does so mostly via
> shelling out to other git commands, whereas it really should be moved to
> using the ref transaction API.
>
> Ig you or anyone else is interested in would be a most welcome project
> to work on...

I must be missing something.

If I understand the problem description correctly, the user does

	git stash push

which

 * bundles the local changes by recording a commit (with trees and
   blobs) that represents the new stash entry

 * removes the local modification from the working tree files.

And if the user kills the process while the second step is running,
there will be files that are restored to HEAD and other files that
are left unrestored, because the process was killed before it had a
chance to do so.  At that point, we probably do not even know which
ones have been restored to be "rolled back"---that knowledge is lost
when the process got killed.

My take on it is that it is not something that we can call "_should_
be fixed".  It is in the same category as "yes, it would be nice if
the world worked that way, and it would be nice if we had moon,
too".

And it has nothing to do with the command being written in C or
shell, and it does not have much to do with the existence of ref
transaction API.  If you want atomic working tree update, you'd need
to snapshot the working tree state, record the fact that you are
about to muck with the working tree in a secure place and make sure
that hits the disk platter, perform the "stash push" operation
including the working tree update, and then remove the record.  The
record will help you discover that your earlier attempt for doing so
failed for whatever reason (e.g. ^C, kill -9, power failure).  Then
you'd need to arrange that the state gets restored, and possibly
redo what you were doing.

Which theoretically can be done.  But it would be not practically
cheap enough to use in a day-to-day operation.  It certainly would
be too much to expect a new person to be able to "work on".

And the "theoretically" part is important, in that it draws the line
between what is realistic and unrealistic.  The thing written in C
or shell would not make much of a dent and the existence of ref
transaction API would not have much effect on partial working tree
updates not being restored.  They are red-herrings.

I suspect that the untold thinking behind your statement was that we
should try not to discourage new users from asking, and I agree with
the sentiment to a certain degree.  But at the same time, I think it
is simply irresponsible to do so without distinguising between
asking for something realistic and unrealistic.

Re: 'git stash push' isn't atomic when Ctrl-C is pressed

[Junio C Hamano]

Yuri <yuri@rawbw.com> writes:

> Ctrl-C was pressed in the middle. git creates the stash record and
> didn't update the files.

If you kill a program in the middle, bad things can happen ;-)

In this case, however, recovery is very easy.

Because you know that a stash entry was created that records the
local changes, if you want to finish "git stash push" (in other
words, if you wish you didn't kill "git stash push" in the middle),
you can "git reset --hard" yourself, because by definition, after
"git stash push" records all the local changes, it would have
cleared any and all local changes.  Or if you truly regret you
started "git stash push" in the first place, then you can still "git
reset --hard" and then "git stash pop".  The first step will let the
"git stash push" to "finish", and then popping the local changes
recorded in there will restore the state before you started running
"git stash push".

Re: 'git stash push' isn't atomic when Ctrl-C is pressed

[Ævar Arnfjörð Bjarmason]

On Wed, Jan 26 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Tue, Jan 25 2022, Yuri wrote:
>>
>>> Ctrl-C was pressed in the middle. git creates the stash record and
>>> didn't update the files.
>>>
>>>
>>> Expected behavior: Ctrl-C should cleanly roll back the operation.
>>
>> Yes, you're right. It really should be fixed.
>>
>> It's a known issue with builtin/stash.c being written in C, but really
>> only still being a faithful conversion of the code we had in a
>> git-stash.sh shellscript until relatively recently.
>>
>> (No fault of those doing the conversion, that's always the logical first
>> step).
>>
>> So it modifies various refs, reflogs etc., but does so mostly via
>> shelling out to other git commands, whereas it really should be moved to
>> using the ref transaction API.
>>
>> Ig you or anyone else is interested in would be a most welcome project
>> to work on...
>
> I must be missing something.
>
> If I understand the problem description correctly, the user does
>
> 	git stash push
>
> which
>
>  * bundles the local changes by recording a commit (with trees and
>    blobs) that represents the new stash entry
>
>  * removes the local modification from the working tree files.
>
> And if the user kills the process while the second step is running,
> there will be files that are restored to HEAD and other files that
> are left unrestored, because the process was killed before it had a
> chance to do so.  At that point, we probably do not even know which
> ones have been restored to be "rolled back"---that knowledge is lost
> when the process got killed.
>
> My take on it is that it is not something that we can call "_should_
> be fixed".  It is in the same category as "yes, it would be nice if
> the world worked that way, and it would be nice if we had moon,
> too".
>
> And it has nothing to do with the command being written in C or
> shell, and it does not have much to do with the existence of ref
> transaction API.  If you want atomic working tree update, you'd need
> to snapshot the working tree state, record the fact that you are
> about to muck with the working tree in a secure place and make sure
> that hits the disk platter, perform the "stash push" operation
> including the working tree update, and then remove the record.  The
> record will help you discover that your earlier attempt for doing so
> failed for whatever reason (e.g. ^C, kill -9, power failure).  Then
> you'd need to arrange that the state gets restored, and possibly
> redo what you were doing.
>
> Which theoretically can be done.  But it would be not practically
> cheap enough to use in a day-to-day operation.  It certainly would
> be too much to expect a new person to be able to "work on".
>
> And the "theoretically" part is important, in that it draws the line
> between what is realistic and unrealistic.  The thing written in C
> or shell would not make much of a dent and the existence of ref
> transaction API would not have much effect on partial working tree
> updates not being restored.  They are red-herrings.

I understood this problem as being one where we do the ref work first,
which we could start a transaction for, the user then ctrl+c's after the
ref work is done, but before the working tree is updated.

Which, if we use the ref transaction API we could intercept with an
atexit() hook that would inspect our state to see if we're done, and if
not roll back the transaction. It wouldn't survive a kill -9, but would
handle being interrupted.

We'd then leave the working tree modifications in place, or not, and
roll back our ref updates, or not, depending on where we were when we
were ctrl+c'd.

The reason it being a command that's recently converted to C (in terms
of its API use) matters is because we don't have any sort of
cross-command ref transaction mechanism, if the ref-modifying command
we're invoking fails we don't know why exactly based on its exit code.

So we're not prepared to roll it back, although that could theoretically
be addressed it's easier just to move it all to in-process API use, and
it also addresses the general TODO of reducing how much we shell out to
ourselves over using internal APIs.

> I suspect that the untold thinking behind your statement was that we
> should try not to discourage new users from asking, and I agree with
> the sentiment to a certain degree.  But at the same time, I think it
> is simply irresponsible to do so without distinguising between
> asking for something realistic and unrealistic.

I must admit I'm not deeply familiar with builtin/stash.c in particular,
but I've looked at e.g. the API use in builtin/branch.c that I
referenced, and I think that (and converting to the transaction API in
general) is a very suitable task for someone who's interested in
contributing, but not deeply familiar with the codebase.

Re: 'git stash push' isn't atomic when Ctrl-C is pressed

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I understood this problem as being one where we do the ref work first,
> which we could start a transaction for, the user then ctrl+c's after the
> ref work is done, but before the working tree is updated.

If the process is killed while the working tree is half updated,
nothing you do with ref transaction will help.

>> I suspect that the untold thinking behind your statement was that we
>> should try not to discourage new users from asking, and I agree with
>> the sentiment to a certain degree.  But at the same time, I think it
>> is simply irresponsible to do so without distinguising between
>> asking for something realistic and unrealistic.
>
> I must admit I'm not deeply familiar with builtin/stash.c in particular,

Then you can try to be on the conservative side, perhaps, to avoid
misleading less experienced folks next time?  Thanks.

Re: 'git stash push' isn't atomic when Ctrl-C is pressed

[John Cai]

On 26 Jan 2022, at 15:47, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Jan 26 2022, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> On Tue, Jan 25 2022, Yuri wrote:
>>>
>>>> Ctrl-C was pressed in the middle. git creates the stash record and
>>>> didn't update the files.
>>>>
>>>>
>>>> Expected behavior: Ctrl-C should cleanly roll back the operation.
>>>
>>> Yes, you're right. It really should be fixed.
>>>
>>> It's a known issue with builtin/stash.c being written in C, but really
>>> only still being a faithful conversion of the code we had in a
>>> git-stash.sh shellscript until relatively recently.
>>>
>>> (No fault of those doing the conversion, that's always the logical first
>>> step).
>>>
>>> So it modifies various refs, reflogs etc., but does so mostly via
>>> shelling out to other git commands, whereas it really should be moved to
>>> using the ref transaction API.
>>>
>>> Ig you or anyone else is interested in would be a most welcome project
>>> to work on...
>>
>> I must be missing something.
>>
>> If I understand the problem description correctly, the user does
>>
>> 	git stash push
>>
>> which
>>
>>  * bundles the local changes by recording a commit (with trees and
>>    blobs) that represents the new stash entry
>>
>>  * removes the local modification from the working tree files.
>>
>> And if the user kills the process while the second step is running,
>> there will be files that are restored to HEAD and other files that
>> are left unrestored, because the process was killed before it had a
>> chance to do so.  At that point, we probably do not even know which
>> ones have been restored to be "rolled back"---that knowledge is lost
>> when the process got killed.
>>
>> My take on it is that it is not something that we can call "_should_
>> be fixed".  It is in the same category as "yes, it would be nice if
>> the world worked that way, and it would be nice if we had moon,
>> too".
>>
>> And it has nothing to do with the command being written in C or
>> shell, and it does not have much to do with the existence of ref
>> transaction API.  If you want atomic working tree update, you'd need
>> to snapshot the working tree state, record the fact that you are
>> about to muck with the working tree in a secure place and make sure
>> that hits the disk platter, perform the "stash push" operation
>> including the working tree update, and then remove the record.  The
>> record will help you discover that your earlier attempt for doing so
>> failed for whatever reason (e.g. ^C, kill -9, power failure).  Then
>> you'd need to arrange that the state gets restored, and possibly
>> redo what you were doing.
>>
>> Which theoretically can be done.  But it would be not practically
>> cheap enough to use in a day-to-day operation.  It certainly would
>> be too much to expect a new person to be able to "work on".
>>
>> And the "theoretically" part is important, in that it draws the line
>> between what is realistic and unrealistic.  The thing written in C
>> or shell would not make much of a dent and the existence of ref
>> transaction API would not have much effect on partial working tree
>> updates not being restored.  They are red-herrings.
>
> I understood this problem as being one where we do the ref work first,
> which we could start a transaction for, the user then ctrl+c's after the
> ref work is done, but before the working tree is updated.
>
> Which, if we use the ref transaction API we could intercept with an
> atexit() hook that would inspect our state to see if we're done, and if
> not roll back the transaction. It wouldn't survive a kill -9, but would
> handle being interrupted.
>
> We'd then leave the working tree modifications in place, or not, and
> roll back our ref updates, or not, depending on where we were when we
> were ctrl+c'd.
>
> The reason it being a command that's recently converted to C (in terms
> of its API use) matters is because we don't have any sort of
> cross-command ref transaction mechanism, if the ref-modifying command
> we're invoking fails we don't know why exactly based on its exit code.
>
> So we're not prepared to roll it back, although that could theoretically
> be addressed it's easier just to move it all to in-process API use, and
> it also addresses the general TODO of reducing how much we shell out to
> ourselves over using internal APIs.
>
>> I suspect that the untold thinking behind your statement was that we
>> should try not to discourage new users from asking, and I agree with
>> the sentiment to a certain degree.  But at the same time, I think it
>> is simply irresponsible to do so without distinguising between
>> asking for something realistic and unrealistic.
>
> I must admit I'm not deeply familiar with builtin/stash.c in particular,
> but I've looked at e.g. the API use in builtin/branch.c that I
> referenced, and I think that (and converting to the transaction API in
> general) is a very suitable task for someone who's interested in
> contributing, but not deeply familiar with the codebase.

If folks think this would still be a useful change, then it sounds like a good
project to work on since it involves no changes in functionality and would help
to reduce the number of shell outs in the code. So though it doesn’t really address
the issue brought up, I’d be glad to make some improvements nevertheless :)

Re: 'git stash push' isn't atomic when Ctrl-C is pressed

[Ævar Arnfjörð Bjarmason]

On Wed, Jan 26 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> I understood this problem as being one where we do the ref work first,
>> which we could start a transaction for, the user then ctrl+c's after the
>> ref work is done, but before the working tree is updated.
>
> If the process is killed while the working tree is half updated,
> nothing you do with ref transaction will help.

This thread is about Ctrl+C, i.e. SIGINT, but presumably we'd use
sigchain_push_common() which covers that and various other signals.

Of course if you're talking about SIGKILL all bets are off.

>>> I suspect that the untold thinking behind your statement was that we
>>> should try not to discourage new users from asking, and I agree with
>>> the sentiment to a certain degree.  But at the same time, I think it
>>> is simply irresponsible to do so without distinguising between
>>> asking for something realistic and unrealistic.
>>
>> I must admit I'm not deeply familiar with builtin/stash.c in particular,
>
> Then you can try to be on the conservative side, perhaps, to avoid
> misleading less experienced folks next time?  Thanks.

I meant I'm not too familiar with details of how "git stash" interacts
with the working tree etc., which is part of what you brought up in your
reply.

But I was mainly commenting on what I think is a fairly straightforward
way to address the original report of wanting references rolled back on
SIGINT, i.e. to move it to the reference transaction API, and to have it
roll back changes in certain scenarios.

Of course it's possible that I'm just missing something, do you see a
reason for why having a signal handler be responsible for rolling back a
reference transaction wouldn't work?

Re: 'git stash push' isn't atomic when Ctrl-C is pressed

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Of course it's possible that I'm just missing something, do you see a
> reason for why having a signal handler be responsible for rolling back a
> reference transaction wouldn't work?

It is not an issue between would and would not work.  It is what is
practical and reasonable to support, and end-user expectation
management.

Besides, if you did

	open the reference transaction
	create a new commit to represent a stash entry
	revert local modifications from working tree files
	update the stash ref with the new commit
	commit the reference transaction

with the auto-rollback of the ref transaction as an atexit handler,
it would help rolling back the reference update (i.e. update to
refs/stash to append a reflog entry), but it would not help at all
rolling back updates to working tree files.

In fact, if you instead did them in this order instead:

	open the reference transaction
	create a new commit to represent a stash entry
	update the stash ref with the new commit
	commit the reference transaction
	revert local modifications from working tree files

it will safely record the local modifications in the stash *and* in
the refstore _before_ it starts to modify the working tree files, so

 (1) killing the process before the ref change is committed will
     truly be a no-op at the end-user level.  There may have been
     unreferenced objects added to the object store, but that won't
     hurt anything.

 (2) killing the process after the ref change is committed, before
     we completely revert all local modifications from the working
     tree files, will still leave crufts in the working tree.  But

     (2-a) you can "git reset --hard" to go to a known good state,
           i.e. the state you would have been in if "git stash push"
           were allowed to finish without interruption.

     (2-b) you can do (2-a) followed by "git stash pop" to go to
           another known good state, namely, the state before you
           ran "git stash push".

If you do not commit the ref transaction before starting to muck
with working tree files, such a recovery, which is transparent and
easy to understand what is going on, would not be possible.  You'd
lose the local changes and be left with a working tree with local
changes partially reverted.