{"thread":{"id":"57307","subject":"'git stash push' isn't atomic when Ctrl-C is pressed","startedAt":"2022-01-25T17:31:45Z","lastAt":"2022-01-27T18:06:00Z","messageCount":11,"participants":["Yuri","Ævar Arnfjörð Bjarmason","John Cai","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"446856","messageId":"4493bcea-c7dd-da0a-e811-83044b3a1cac@tsoft.com","threadId":"57307","inReplyTo":null,"subject":"'git stash push' isn't atomic when Ctrl-C is pressed","fromName":"Yuri","fromEmail":"yuri@rawbw.com","sentAt":"2022-01-25T17:13:33Z","receivedAt":"2022-01-25T17:31:45Z","isPatch":false,"sender":{"key":"yuri@rawbw.com","avatar":null},"body":"Ctrl-C was pressed in the middle. git creates the stash record and \ndidn't update the files.\n\n\nExpected behavior: Ctrl-C should cleanly roll back the operation.\n\n\n\nYuri\n\n\n"},{"id":"446959","messageId":"220126.86bkzyfw3q.gmgdl@evledraar.gmail.com","threadId":"57307","inReplyTo":"4493bcea-c7dd-da0a-e811-83044b3a1cac@tsoft.com","subject":"Re: 'git stash push' isn't atomic when Ctrl-C is pressed","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-26T13:41:50Z","receivedAt":"2022-01-26T13:46:38Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jan 25 2022, Yuri wrote:\n\n> Ctrl-C was pressed in the middle. git creates the stash record and\n> didn't update the files.\n>\n>\n> Expected behavior: Ctrl-C should cleanly roll back the operation.\n\nYes, you're right. It really should be fixed.\n\nIt's a known issue with builtin/stash.c being written in C, but really\nonly still being a faithful conversion of the code we had in a\ngit-stash.sh shellscript until relatively recently.\n\n(No fault of those doing the conversion, that's always the logical first\nstep).\n\nSo it modifies various refs, reflogs etc., but does so mostly via\nshelling out to other git commands, whereas it really should be moved to\nusing the ref transaction API.\n\nIg you or anyone else is interested in would be a most welcome project\nto work on...\n"},{"id":"446972","messageId":"B6F534FC-FCEE-4A90-9576-233103865B3E@gitlab.com","threadId":"57307","inReplyTo":"220126.86bkzyfw3q.gmgdl@evledraar.gmail.com","subject":"Re: 'git stash push' isn't atomic when Ctrl-C is pressed","fromName":"John Cai","fromEmail":"jcai@gitlab.com","sentAt":"2022-01-26T16:09:15Z","receivedAt":"2022-01-26T16:09:20Z","isPatch":false,"sender":{"key":"jcai@gitlab.com","avatar":"https://gravatar.com/avatar/4ba958d432b21b53dd76009b44d31b70bd387a31d2957cee1b257ced4bf812f8?d=mp&s=160"},"body":"\n\nOn 26 Jan 2022, at 8:41, Ævar Arnfjörð Bjarmason wrote:\n\n> On Tue, Jan 25 2022, Yuri wrote:\n>\n>> Ctrl-C was pressed in the middle. git creates the stash record and\n>> didn't update the files.\n>>\n>>\n>> Expected behavior: Ctrl-C should cleanly roll back the operation.\n>\n> Yes, you're right. It really should be fixed.\n>\n> It's a known issue with builtin/stash.c being written in C, but really\n> only still being a faithful conversion of the code we had in a\n> git-stash.sh shellscript until relatively recently.\n>\n> (No fault of those doing the conversion, that's always the logical first\n> step).\n>\n> So it modifies various refs, reflogs etc., but does so mostly via\n> shelling out to other git commands, whereas it really should be moved to\n> using the ref transaction API.\n>\n> Ig you or anyone else is interested in would be a most welcome project\n> to work on…\n\nI’d be happy to help with this!\n"},{"id":"446978","messageId":"220126.86h79qe692.gmgdl@evledraar.gmail.com","threadId":"57307","inReplyTo":"B6F534FC-FCEE-4A90-9576-233103865B3E@gitlab.com","subject":"Re: 'git stash push' isn't atomic when Ctrl-C is pressed","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-26T17:30:43Z","receivedAt":"2022-01-26T17:50:22Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 26 2022, John Cai wrote:\n\n> On 26 Jan 2022, at 8:41, Ævar Arnfjörð Bjarmason wrote:\n>\n>> On Tue, Jan 25 2022, Yuri wrote:\n>>\n>>> Ctrl-C was pressed in the middle. git creates the stash record and\n>>> didn't update the files.\n>>>\n>>>\n>>> Expected behavior: Ctrl-C should cleanly roll back the operation.\n>>\n>> Yes, you're right. It really should be fixed.\n>>\n>> It's a known issue with builtin/stash.c being written in C, but really\n>> only still being a faithful conversion of the code we had in a\n>> git-stash.sh shellscript until relatively recently.\n>>\n>> (No fault of those doing the conversion, that's always the logical first\n>> step).\n>>\n>> So it modifies various refs, reflogs etc., but does so mostly via\n>> shelling out to other git commands, whereas it really should be moved to\n>> using the ref transaction API.\n>>\n>> Ig you or anyone else is interested in would be a most welcome project\n>> to work on…\n>\n> I’d be happy to help with this!\n\nThanks. I think that would be a great thing to work on.\n\nPart of it is summarized in my 212631ed50a (refs/files: remove unused\nREF_DELETING in lock_ref_oid_basic(), 2021-07-20). I think there was a\nbetter summary somewhere (maybe an exchange with Jeff King?) but I can't\nfind it now.\n\nThere's a further summary of the remaining callers in 52106430dc8\n(refs/files: remove \"name exist?\" check in lock_ref_oid_basic(),\n2021-10-16), which as we'll see is closely coupled with these remaining\ntransaction-less refs API callers.\n\nBeginning to fix that is as basic as starting with some light move of\nexisting code into library form, e.g. when you \"git stash drop\" it will\ninvoke:\n\n 18:29:02.800983 git.c:459 trace: built-in: git reflog delete --updateref --rewrite 'refs/stash@{0}'\n\nWhich will lock a ref with:\n \n (gdb) bt\n #0 BUG_fl (file=0x7965ef \"refs/files-backend.c\", line=1011, fmt=0x796f30 \"hi\") at usage.c:324\n #1 0x0000000000675488 in lock_ref_oid_basic (refs=0x855470, refname=0x855140 \"refs/stash\", err=0x7fffffffd9b0) at refs/files-backend.c:1011\n #2 0x00000000006722bb in files_reflog_expire (ref_store=0x855470, refname=0x855140 \"refs/stash\", expire_flags=6, prepare_fn=0x4c9390 ,\n should_prune_fn=0x4c8c40 , cleanup_fn=0x4c9510 , policy_cb_data=0x7fffffffdb30) at refs/files-backend.c:3159\n #3 0x000000000066d832 in refs_reflog_expire (refs=0x855470, refname=0x855140 \"refs/stash\", flags=6, prepare_fn=0x4c9390 ,\n should_prune_fn=0x4c8c40 , cleanup_fn=0x4c9510 , policy_cb_data=0x7fffffffdb30) at refs.c:2411\n #4 0x000000000066d88f in reflog_expire (refname=0x855140 \"refs/stash\", flags=6, prepare_fn=0x4c9390 , should_prune_fn=0x4c8c40 ,\n cleanup_fn=0x4c9510 , policy_cb_data=0x7fffffffdb30) at refs.c:2423\n #5 0x00000000004c8ad7 in cmd_reflog_delete (argc=1, argv=0x7fffffffe118, prefix=0x0) at builtin/reflog.c:780\n #6 0x00000000004c7abb in cmd_reflog (argc=5, argv=0x7fffffffe110, prefix=0x0) at builtin/reflog.c:839\n #7 0x0000000000407c8b in run_builtin (p=0x81e0e0 , argc=5, argv=0x7fffffffe110) at git.c:465\n #8 0x0000000000406742 in handle_builtin (argc=5, argv=0x7fffffffe110) at git.c:719\n #9 0x0000000000407676 in run_argv (argcp=0x7fffffffdfcc, argv=0x7fffffffdfc0) at git.c:786\n #10 0x0000000000406501 in cmd_main (argc=5, argv=0x7fffffffe110) at git.c:917\n #11 0x0000000000510fba in main (argc=6, argv=0x7fffffffe108) at common-main.c:56\n\nWhich, if you chase that down to builtin/reflog.c you can see is just a\ncase where we could avoid the sub-process invocation by calling\nreflog_expire() ourselves in builtin/stash.c, perhps after lib-ifying\nsome of what it's doing into a new top-level reflog.c, and having the\ncommand-line specific stuff in builtin/reflog.c call that.\n\nYou've hacked on that code recently, so that should be easy to start\nwith :)\n\nThen once we do all the ref updates in-process it should be easy (or\neasy enough...) to move it over to the ref transaction API. So if we\nfail we'll roll everything back properly.\n\nThe eventual approximate goal should be to get rid of\nlock_ref_oid_basic() entirely, it's legacy API in the refs files backend\nthat makes a lot of things over there more complex. Some other users of\nit can be found with:\n\n git grep -w -e copy_ref -e rename_ref\n\nOr, by just removing the function recursively during testing, and\nfollowing the compilation errors.\n"},{"id":"446987","messageId":"xmqqlez2qnfi.fsf@gitster.g","threadId":"57307","inReplyTo":"220126.86bkzyfw3q.gmgdl@evledraar.gmail.com","subject":"Re: 'git stash push' isn't atomic when Ctrl-C is pressed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-26T19:58:25Z","receivedAt":"2022-01-26T19:58:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Tue, Jan 25 2022, Yuri wrote:\n>\n>> Ctrl-C was pressed in the middle. git creates the stash record and\n>> didn't update the files.\n>>\n>>\n>> Expected behavior: Ctrl-C should cleanly roll back the operation.\n>\n> Yes, you're right. It really should be fixed.\n>\n> It's a known issue with builtin/stash.c being written in C, but really\n> only still being a faithful conversion of the code we had in a\n> git-stash.sh shellscript until relatively recently.\n>\n> (No fault of those doing the conversion, that's always the logical first\n> step).\n>\n> So it modifies various refs, reflogs etc., but does so mostly via\n> shelling out to other git commands, whereas it really should be moved to\n> using the ref transaction API.\n>\n> Ig you or anyone else is interested in would be a most welcome project\n> to work on...\n\nI must be missing something.\n\nIf I understand the problem description correctly, the user does\n\n\tgit stash push\n\nwhich\n\n * bundles the local changes by recording a commit (with trees and\n blobs) that represents the new stash entry\n\n * removes the local modification from the working tree files.\n\nAnd if the user kills the process while the second step is running,\nthere will be files that are restored to HEAD and other files that\nare left unrestored, because the process was killed before it had a\nchance to do so. At that point, we probably do not even know which\nones have been restored to be \"rolled back\"---that knowledge is lost\nwhen the process got killed.\n\nMy take on it is that it is not something that we can call \"_should_\nbe fixed\". It is in the same category as \"yes, it would be nice if\nthe world worked that way, and it would be nice if we had moon,\ntoo\".\n\nAnd it has nothing to do with the command being written in C or\nshell, and it does not have much to do with the existence of ref\ntransaction API. If you want atomic working tree update, you'd need\nto snapshot the working tree state, record the fact that you are\nabout to muck with the working tree in a secure place and make sure\nthat hits the disk platter, perform the \"stash push\" operation\nincluding the working tree update, and then remove the record. The\nrecord will help you discover that your earlier attempt for doing so\nfailed for whatever reason (e.g. ^C, kill -9, power failure). Then\nyou'd need to arrange that the state gets restored, and possibly\nredo what you were doing.\n\nWhich theoretically can be done. But it would be not practically\ncheap enough to use in a day-to-day operation. It certainly would\nbe too much to expect a new person to be able to \"work on\".\n\nAnd the \"theoretically\" part is important, in that it draws the line\nbetween what is realistic and unrealistic. The thing written in C\nor shell would not make much of a dent and the existence of ref\ntransaction API would not have much effect on partial working tree\nupdates not being restored. They are red-herrings.\n\nI suspect that the untold thinking behind your statement was that we\nshould try not to discourage new users from asking, and I agree with\nthe sentiment to a certain degree. But at the same time, I think it\nis simply irresponsible to do so without distinguising between\nasking for something realistic and unrealistic.\n"},{"id":"446994","messageId":"xmqq4k5qqma2.fsf@gitster.g","threadId":"57307","inReplyTo":"4493bcea-c7dd-da0a-e811-83044b3a1cac@tsoft.com","subject":"Re: 'git stash push' isn't atomic when Ctrl-C is pressed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-26T20:23:17Z","receivedAt":"2022-01-26T20:23:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yuri writes:\n\n> Ctrl-C was pressed in the middle. git creates the stash record and\n> didn't update the files.\n\nIf you kill a program in the middle, bad things can happen ;-)\n\nIn this case, however, recovery is very easy.\n\nBecause you know that a stash entry was created that records the\nlocal changes, if you want to finish \"git stash push\" (in other\nwords, if you wish you didn't kill \"git stash push\" in the middle),\nyou can \"git reset --hard\" yourself, because by definition, after\n\"git stash push\" records all the local changes, it would have\ncleared any and all local changes. Or if you truly regret you\nstarted \"git stash push\" in the first place, then you can still \"git\nreset --hard\" and then \"git stash pop\". The first step will let the\n\"git stash push\" to \"finish\", and then popping the local changes\nrecorded in there will restore the state before you started running\n\"git stash push\".\n\n"},{"id":"446998","messageId":"220126.864k5qdx66.gmgdl@evledraar.gmail.com","threadId":"57307","inReplyTo":"xmqqlez2qnfi.fsf@gitster.g","subject":"Re: 'git stash push' isn't atomic when Ctrl-C is pressed","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-26T20:47:10Z","receivedAt":"2022-01-26T21:06:31Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 26 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> On Tue, Jan 25 2022, Yuri wrote:\n>>\n>>> Ctrl-C was pressed in the middle. git creates the stash record and\n>>> didn't update the files.\n>>>\n>>>\n>>> Expected behavior: Ctrl-C should cleanly roll back the operation.\n>>\n>> Yes, you're right. It really should be fixed.\n>>\n>> It's a known issue with builtin/stash.c being written in C, but really\n>> only still being a faithful conversion of the code we had in a\n>> git-stash.sh shellscript until relatively recently.\n>>\n>> (No fault of those doing the conversion, that's always the logical first\n>> step).\n>>\n>> So it modifies various refs, reflogs etc., but does so mostly via\n>> shelling out to other git commands, whereas it really should be moved to\n>> using the ref transaction API.\n>>\n>> Ig you or anyone else is interested in would be a most welcome project\n>> to work on...\n>\n> I must be missing something.\n>\n> If I understand the problem description correctly, the user does\n>\n> \tgit stash push\n>\n> which\n>\n> * bundles the local changes by recording a commit (with trees and\n> blobs) that represents the new stash entry\n>\n> * removes the local modification from the working tree files.\n>\n> And if the user kills the process while the second step is running,\n> there will be files that are restored to HEAD and other files that\n> are left unrestored, because the process was killed before it had a\n> chance to do so. At that point, we probably do not even know which\n> ones have been restored to be \"rolled back\"---that knowledge is lost\n> when the process got killed.\n>\n> My take on it is that it is not something that we can call \"_should_\n> be fixed\". It is in the same category as \"yes, it would be nice if\n> the world worked that way, and it would be nice if we had moon,\n> too\".\n>\n> And it has nothing to do with the command being written in C or\n> shell, and it does not have much to do with the existence of ref\n> transaction API. If you want atomic working tree update, you'd need\n> to snapshot the working tree state, record the fact that you are\n> about to muck with the working tree in a secure place and make sure\n> that hits the disk platter, perform the \"stash push\" operation\n> including the working tree update, and then remove the record. The\n> record will help you discover that your earlier attempt for doing so\n> failed for whatever reason (e.g. ^C, kill -9, power failure). Then\n> you'd need to arrange that the state gets restored, and possibly\n> redo what you were doing.\n>\n> Which theoretically can be done. But it would be not practically\n> cheap enough to use in a day-to-day operation. It certainly would\n> be too much to expect a new person to be able to \"work on\".\n>\n> And the \"theoretically\" part is important, in that it draws the line\n> between what is realistic and unrealistic. The thing written in C\n> or shell would not make much of a dent and the existence of ref\n> transaction API would not have much effect on partial working tree\n> updates not being restored. They are red-herrings.\n\nI understood this problem as being one where we do the ref work first,\nwhich we could start a transaction for, the user then ctrl+c's after the\nref work is done, but before the working tree is updated.\n\nWhich, if we use the ref transaction API we could intercept with an\natexit() hook that would inspect our state to see if we're done, and if\nnot roll back the transaction. It wouldn't survive a kill -9, but would\nhandle being interrupted.\n\nWe'd then leave the working tree modifications in place, or not, and\nroll back our ref updates, or not, depending on where we were when we\nwere ctrl+c'd.\n\nThe reason it being a command that's recently converted to C (in terms\nof its API use) matters is because we don't have any sort of\ncross-command ref transaction mechanism, if the ref-modifying command\nwe're invoking fails we don't know why exactly based on its exit code.\n\nSo we're not prepared to roll it back, although that could theoretically\nbe addressed it's easier just to move it all to in-process API use, and\nit also addresses the general TODO of reducing how much we shell out to\nourselves over using internal APIs.\n\n> I suspect that the untold thinking behind your statement was that we\n> should try not to discourage new users from asking, and I agree with\n> the sentiment to a certain degree. But at the same time, I think it\n> is simply irresponsible to do so without distinguising between\n> asking for something realistic and unrealistic.\n\nI must admit I'm not deeply familiar with builtin/stash.c in particular,\nbut I've looked at e.g. the API use in builtin/branch.c that I\nreferenced, and I think that (and converting to the transaction API in\ngeneral) is a very suitable task for someone who's interested in\ncontributing, but not deeply familiar with the codebase.\n\n"},{"id":"447014","messageId":"xmqqczkeozpx.fsf@gitster.g","threadId":"57307","inReplyTo":"220126.864k5qdx66.gmgdl@evledraar.gmail.com","subject":"Re: 'git stash push' isn't atomic when Ctrl-C is pressed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-26T23:15:54Z","receivedAt":"2022-01-26T23:15:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I understood this problem as being one where we do the ref work first,\n> which we could start a transaction for, the user then ctrl+c's after the\n> ref work is done, but before the working tree is updated.\n\nIf the process is killed while the working tree is half updated,\nnothing you do with ref transaction will help.\n\n>> I suspect that the untold thinking behind your statement was that we\n>> should try not to discourage new users from asking, and I agree with\n>> the sentiment to a certain degree. But at the same time, I think it\n>> is simply irresponsible to do so without distinguising between\n>> asking for something realistic and unrealistic.\n>\n> I must admit I'm not deeply familiar with builtin/stash.c in particular,\n\nThen you can try to be on the conservative side, perhaps, to avoid\nmisleading less experienced folks next time? Thanks.\n\n"},{"id":"447028","messageId":"E6946EAC-4F4D-41CF-9D36-5D1E64D0FF09@gitlab.com","threadId":"57307","inReplyTo":"220126.864k5qdx66.gmgdl@evledraar.gmail.com","subject":"Re: 'git stash push' isn't atomic when Ctrl-C is pressed","fromName":"John Cai","fromEmail":"jcai@gitlab.com","sentAt":"2022-01-27T01:53:06Z","receivedAt":"2022-01-27T01:53:10Z","isPatch":false,"sender":{"key":"jcai@gitlab.com","avatar":"https://gravatar.com/avatar/4ba958d432b21b53dd76009b44d31b70bd387a31d2957cee1b257ced4bf812f8?d=mp&s=160"},"body":"\n\nOn 26 Jan 2022, at 15:47, Ævar Arnfjörð Bjarmason wrote:\n\n> On Wed, Jan 26 2022, Junio C Hamano wrote:\n>\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>>> On Tue, Jan 25 2022, Yuri wrote:\n>>>\n>>>> Ctrl-C was pressed in the middle. git creates the stash record and\n>>>> didn't update the files.\n>>>>\n>>>>\n>>>> Expected behavior: Ctrl-C should cleanly roll back the operation.\n>>>\n>>> Yes, you're right. It really should be fixed.\n>>>\n>>> It's a known issue with builtin/stash.c being written in C, but really\n>>> only still being a faithful conversion of the code we had in a\n>>> git-stash.sh shellscript until relatively recently.\n>>>\n>>> (No fault of those doing the conversion, that's always the logical first\n>>> step).\n>>>\n>>> So it modifies various refs, reflogs etc., but does so mostly via\n>>> shelling out to other git commands, whereas it really should be moved to\n>>> using the ref transaction API.\n>>>\n>>> Ig you or anyone else is interested in would be a most welcome project\n>>> to work on...\n>>\n>> I must be missing something.\n>>\n>> If I understand the problem description correctly, the user does\n>>\n>> \tgit stash push\n>>\n>> which\n>>\n>> * bundles the local changes by recording a commit (with trees and\n>> blobs) that represents the new stash entry\n>>\n>> * removes the local modification from the working tree files.\n>>\n>> And if the user kills the process while the second step is running,\n>> there will be files that are restored to HEAD and other files that\n>> are left unrestored, because the process was killed before it had a\n>> chance to do so. At that point, we probably do not even know which\n>> ones have been restored to be \"rolled back\"---that knowledge is lost\n>> when the process got killed.\n>>\n>> My take on it is that it is not something that we can call \"_should_\n>> be fixed\". It is in the same category as \"yes, it would be nice if\n>> the world worked that way, and it would be nice if we had moon,\n>> too\".\n>>\n>> And it has nothing to do with the command being written in C or\n>> shell, and it does not have much to do with the existence of ref\n>> transaction API. If you want atomic working tree update, you'd need\n>> to snapshot the working tree state, record the fact that you are\n>> about to muck with the working tree in a secure place and make sure\n>> that hits the disk platter, perform the \"stash push\" operation\n>> including the working tree update, and then remove the record. The\n>> record will help you discover that your earlier attempt for doing so\n>> failed for whatever reason (e.g. ^C, kill -9, power failure). Then\n>> you'd need to arrange that the state gets restored, and possibly\n>> redo what you were doing.\n>>\n>> Which theoretically can be done. But it would be not practically\n>> cheap enough to use in a day-to-day operation. It certainly would\n>> be too much to expect a new person to be able to \"work on\".\n>>\n>> And the \"theoretically\" part is important, in that it draws the line\n>> between what is realistic and unrealistic. The thing written in C\n>> or shell would not make much of a dent and the existence of ref\n>> transaction API would not have much effect on partial working tree\n>> updates not being restored. They are red-herrings.\n>\n> I understood this problem as being one where we do the ref work first,\n> which we could start a transaction for, the user then ctrl+c's after the\n> ref work is done, but before the working tree is updated.\n>\n> Which, if we use the ref transaction API we could intercept with an\n> atexit() hook that would inspect our state to see if we're done, and if\n> not roll back the transaction. It wouldn't survive a kill -9, but would\n> handle being interrupted.\n>\n> We'd then leave the working tree modifications in place, or not, and\n> roll back our ref updates, or not, depending on where we were when we\n> were ctrl+c'd.\n>\n> The reason it being a command that's recently converted to C (in terms\n> of its API use) matters is because we don't have any sort of\n> cross-command ref transaction mechanism, if the ref-modifying command\n> we're invoking fails we don't know why exactly based on its exit code.\n>\n> So we're not prepared to roll it back, although that could theoretically\n> be addressed it's easier just to move it all to in-process API use, and\n> it also addresses the general TODO of reducing how much we shell out to\n> ourselves over using internal APIs.\n>\n>> I suspect that the untold thinking behind your statement was that we\n>> should try not to discourage new users from asking, and I agree with\n>> the sentiment to a certain degree. But at the same time, I think it\n>> is simply irresponsible to do so without distinguising between\n>> asking for something realistic and unrealistic.\n>\n> I must admit I'm not deeply familiar with builtin/stash.c in particular,\n> but I've looked at e.g. the API use in builtin/branch.c that I\n> referenced, and I think that (and converting to the transaction API in\n> general) is a very suitable task for someone who's interested in\n> contributing, but not deeply familiar with the codebase.\n\nIf folks think this would still be a useful change, then it sounds like a good\nproject to work on since it involves no changes in functionality and would help\nto reduce the number of shell outs in the code. So though it doesn’t really address\nthe issue brought up, I’d be glad to make some improvements nevertheless :)\n"},{"id":"447031","messageId":"220127.86zgnhdhft.gmgdl@evledraar.gmail.com","threadId":"57307","inReplyTo":"xmqqczkeozpx.fsf@gitster.g","subject":"Re: 'git stash push' isn't atomic when Ctrl-C is pressed","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-27T02:36:21Z","receivedAt":"2022-01-27T02:46:21Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 26 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> I understood this problem as being one where we do the ref work first,\n>> which we could start a transaction for, the user then ctrl+c's after the\n>> ref work is done, but before the working tree is updated.\n>\n> If the process is killed while the working tree is half updated,\n> nothing you do with ref transaction will help.\n\nThis thread is about Ctrl+C, i.e. SIGINT, but presumably we'd use\nsigchain_push_common() which covers that and various other signals.\n\nOf course if you're talking about SIGKILL all bets are off.\n\n>>> I suspect that the untold thinking behind your statement was that we\n>>> should try not to discourage new users from asking, and I agree with\n>>> the sentiment to a certain degree. But at the same time, I think it\n>>> is simply irresponsible to do so without distinguising between\n>>> asking for something realistic and unrealistic.\n>>\n>> I must admit I'm not deeply familiar with builtin/stash.c in particular,\n>\n> Then you can try to be on the conservative side, perhaps, to avoid\n> misleading less experienced folks next time? Thanks.\n\nI meant I'm not too familiar with details of how \"git stash\" interacts\nwith the working tree etc., which is part of what you brought up in your\nreply.\n\nBut I was mainly commenting on what I think is a fairly straightforward\nway to address the original report of wanting references rolled back on\nSIGINT, i.e. to move it to the reference transaction API, and to have it\nroll back changes in certain scenarios.\n\nOf course it's possible that I'm just missing something, do you see a\nreason for why having a signal handler be responsible for rolling back a\nreference transaction wouldn't work?\n"},{"id":"447104","messageId":"xmqq4k5p3vgd.fsf@gitster.g","threadId":"57307","inReplyTo":"220127.86zgnhdhft.gmgdl@evledraar.gmail.com","subject":"Re: 'git stash push' isn't atomic when Ctrl-C is pressed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-27T18:05:54Z","receivedAt":"2022-01-27T18:06:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Of course it's possible that I'm just missing something, do you see a\n> reason for why having a signal handler be responsible for rolling back a\n> reference transaction wouldn't work?\n\nIt is not an issue between would and would not work. It is what is\npractical and reasonable to support, and end-user expectation\nmanagement.\n\nBesides, if you did\n\n\topen the reference transaction\n\tcreate a new commit to represent a stash entry\n\trevert local modifications from working tree files\n\tupdate the stash ref with the new commit\n\tcommit the reference transaction\n\nwith the auto-rollback of the ref transaction as an atexit handler,\nit would help rolling back the reference update (i.e. update to\nrefs/stash to append a reflog entry), but it would not help at all\nrolling back updates to working tree files.\n\nIn fact, if you instead did them in this order instead:\n\n\topen the reference transaction\n\tcreate a new commit to represent a stash entry\n\tupdate the stash ref with the new commit\n\tcommit the reference transaction\n\trevert local modifications from working tree files\n\nit will safely record the local modifications in the stash *and* in\nthe refstore _before_ it starts to modify the working tree files, so\n\n (1) killing the process before the ref change is committed will\n truly be a no-op at the end-user level. There may have been\n unreferenced objects added to the object store, but that won't\n hurt anything.\n\n (2) killing the process after the ref change is committed, before\n we completely revert all local modifications from the working\n tree files, will still leave crufts in the working tree. But\n\n (2-a) you can \"git reset --hard\" to go to a known good state,\n i.e. the state you would have been in if \"git stash push\"\n were allowed to finish without interruption.\n\n (2-b) you can do (2-a) followed by \"git stash pop\" to go to\n another known good state, namely, the state before you\n ran \"git stash push\".\n\nIf you do not commit the ref transaction before starting to muck\nwith working tree files, such a recovery, which is transparent and\neasy to understand what is going on, would not be possible. You'd\nlose the local changes and be left with a working tree with local\nchanges partially reverted.\n\n\n\n"}]}