git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Yuri <yuri@rawbw.com>, Git Mailing List <git@vger.kernel.org>
Subject: Re: 'git stash push' isn't atomic when Ctrl-C is pressed
Date: Wed, 26 Jan 2022 11:58:25 -0800	[thread overview]
Message-ID: <xmqqlez2qnfi.fsf@gitster.g> (raw)
In-Reply-To: <220126.86bkzyfw3q.gmgdl@evledraar.gmail.com> ("Ævar Arnfjörð Bjarmason"'s message of "Wed, 26 Jan 2022 14:41:50 +0100")

Æ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.

  parent reply	other threads:[~2022-01-26 19:58 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-25 17:13 'git stash push' isn't atomic when Ctrl-C is pressed Yuri
2022-01-26 13:41 ` Ævar Arnfjörð Bjarmason
2022-01-26 16:09   ` John Cai
2022-01-26 17:30     ` Ævar Arnfjörð Bjarmason
2022-01-26 19:58   ` Junio C Hamano [this message]
2022-01-26 20:47     ` Ævar Arnfjörð Bjarmason
2022-01-26 23:15       ` Junio C Hamano
2022-01-27  2:36         ` Ævar Arnfjörð Bjarmason
2022-01-27 18:05           ` Junio C Hamano
2022-01-27  1:53       ` John Cai
2022-01-26 20:23 ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqlez2qnfi.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=yuri@rawbw.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).