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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.