| Message ID | 20230903151132.739166-1-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sequencer: update abort safety file more sparingly | expand |
Hi Oswald On 03/09/2023 16:11, Oswald Buddenhagen wrote: > The only situation where the file's content matters is --continue'ing > (after a multi-cherry-pick merge conflict). I don't think "cherry-pick --continue" consults the abort safety file, it only matters for "cherry-pick --skip" and "cherry-pick --abort". > This means that it is > sufficient to write it in a single place, when we are prematurely > exiting the main workhorse. I think this introduces a regression because the safety file will not get updated when "cherry-pick --continue" stops for the user to resolve conflicts. > This is much easier to reason about than the > three dispersed calls originally introduced in 1e41229d ("sequencer: > make sequencer abort safer", 2016-12-07). We now can also remove the > inefficient file-based check whether the file needs writing, which > wasn't even reliable: a single pick executed during an interrupted > sequence would bypass the safety. An alternate view is that the abort safety file exists to prevent the user losing commits that have not been cherry-picked and it is desirable to be able to abort after cherry-picking a single pick in the middle of a sequence of cherry-picks. Best Wishes Phillip > Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> > > --- > Cc: Stephan Beyer <s-beyer@gmx.net> > Cc: Johannes Schindelin <johannes.schindelin@gmx.de> > Cc: Phillip Wood <phillip.wood123@gmail.com> > --- > sequencer.c | 9 ++------- > t/t3510-cherry-pick-sequence.sh | 9 +++++++++ > 2 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index a66dcf8ab2..716384cc7b 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -575,10 +575,6 @@ static void update_abort_safety_file(void) > { > struct object_id head; > > - /* Do nothing on a single-pick */ > - if (!file_exists(git_path_seq_dir())) > - return; > - > if (!repo_get_oid(the_repository, "HEAD", &head)) > write_file(git_path_abort_safety_file(), "%s", oid_to_hex(&head)); > else > @@ -618,7 +614,6 @@ static int fast_forward_to(struct repository *r, > strbuf_release(&sb); > strbuf_release(&err); > ref_transaction_free(transaction); > - update_abort_safety_file(); > return 0; > } > > @@ -2435,7 +2430,6 @@ static int do_pick_commit(struct repository *r, > free_message(commit, &msg); > free(author); > strbuf_release(&msgbuf); > - update_abort_safety_file(); > > return res; > } > @@ -5269,8 +5263,9 @@ int sequencer_pick_revisions(struct repository *r, > return -1; > if (save_opts(opts)) > return -1; > - update_abort_safety_file(); > res = pick_commits(r, &todo_list, opts); > + if (todo_list.current < todo_list.nr) > + update_abort_safety_file(); > todo_list_release(&todo_list); > return res; > } > diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh > index 3b0fa66c33..170b664c33 100755 > --- a/t/t3510-cherry-pick-sequence.sh > +++ b/t/t3510-cherry-pick-sequence.sh > @@ -318,6 +318,15 @@ test_expect_success '--abort does not unsafely change HEAD' ' > test_cmp_rev base HEAD > ' > > +test_expect_success '--abort after single pick does not unsafely change HEAD' ' > + pristine_detach initial && > + test_must_fail git cherry-pick picked anotherpick && > + git reset --hard && > + git cherry-pick unrelatedpick && > + git cherry-pick --abort 2>actual && > + test_i18ngrep "You seem to have moved HEAD" actual > +' > + > test_expect_success 'cherry-pick --abort to cancel multiple revert' ' > pristine_detach anotherpick && > test_expect_code 1 git revert base..picked &&
On Sun, Sep 03, 2023 at 07:40:00PM +0100, Phillip Wood wrote: >On 03/09/2023 16:11, Oswald Buddenhagen wrote: >> The only situation where the file's content matters is --continue'ing >> (after a multi-cherry-pick merge conflict). > >I don't think "cherry-pick --continue" consults the abort safety file, > duh, obvious blunder. >it only matters for "cherry-pick --skip" > that doesn't seem right. a --skip is just a --continue with a prior reset, more or less. >and "cherry-pick --abort". > that one, of course. >> This means that it is >> sufficient to write it in a single place, when we are prematurely >> exiting the main workhorse. > >I think this introduces a regression because the safety file will not >get updated when "cherry-pick --continue" stops for the user to resolve >conflicts. > true, there is indeed this second entry point. i'll try to find a better "choke point". >> which wasn't even reliable: a single pick executed during an >> interrupted sequence would bypass the safety. > >An alternate view is that the abort safety file exists to prevent the >user losing commits that have not been cherry-picked and it is >desirable to be able to abort after cherry-picking a single pick in the >middle of a sequence of cherry-picks. > if you did a fresh commit before or after the single pick, you'd lose it. also, the feature doesn't actually prevent aborting, only the automatic reset. regards
On 03/09/2023 20:25, Oswald Buddenhagen wrote: > On Sun, Sep 03, 2023 at 07:40:00PM +0100, Phillip Wood wrote: >> On 03/09/2023 16:11, Oswald Buddenhagen wrote: >>> The only situation where the file's content matters is --continue'ing >>> (after a multi-cherry-pick merge conflict). >> >> I don't think "cherry-pick --continue" consults the abort safety file, > duh, obvious blunder. > >> it only matters for "cherry-pick --skip" >> > that doesn't seem right. a --skip is just a --continue with a prior > reset, more or less. sequencer_skip() calls rollback_is_safe() which checks the abort safety file. >> and "cherry-pick --abort". >> > that one, of course. > >>> This means that it is >>> sufficient to write it in a single place, when we are prematurely >>> exiting the main workhorse. >> >> I think this introduces a regression because the safety file will not >> get updated when "cherry-pick --continue" stops for the user to >> resolve conflicts. >> > true, there is indeed this second entry point. > i'll try to find a better "choke point". I think that is probably tricky, I'm not really clear what the aim/purpose of this refactoring is. >>> which wasn't even reliable: a single pick executed during an >>> interrupted sequence would bypass the safety. >> >> An alternate view is that the abort safety file exists to prevent the >> user losing commits that have not been cherry-picked and it is >> desirable to be able to abort after cherry-picking a single pick in >> the middle of a sequence of cherry-picks. >> > if you did a fresh commit before or after the single pick, you'd lose it. > also, Oh, I can see that you'd lose a commit made before a single pick but I don't see how you'd lose a commit made after it. I'm still not convinced it is a particularly helpful change. > the feature doesn't actually prevent aborting, only the automatic > reset. Oh right, it removes the state directory but leaves HEAD untouched if it does not match the commit recorded in the abort safety file. Best Wishes Phillip > regards
On Sun, Sep 03, 2023 at 08:48:14PM +0100, Phillip Wood wrote: >On 03/09/2023 20:25, Oswald Buddenhagen wrote: >> On Sun, Sep 03, 2023 at 07:40:00PM +0100, Phillip Wood wrote: >>> it only matters for "cherry-pick --skip" >>> >> that doesn't seem right. a --skip is just a --continue with a prior >> reset, more or less. > >sequencer_skip() calls rollback_is_safe() which checks the abort safety >file. > that's weird. can you think of a good reason for doing that? >> i'll try to find a better "choke point". > >I think that is probably tricky, > yeah >I'm not really clear what the aim/purpose of this refactoring is. > to make my head not explode. more specifically, to get it out of the way of the rebase path, which is what i'm actually concerned with. generally, i think this whole ad-hoc state management is a nightmare, and i'd be surprised if there weren't some more loose ends. i think i'd aim for an object-oriented-ish design with an encapsulated state, lazy loading getters, lazy setters, and a commit entry point (or maybe several partial ones). no idea how that would play out. >> if you did a fresh commit before or after the single pick, you'd lose >> it. > >Oh, I can see that you'd lose a commit made before a single pick but I >don't see how you'd lose a commit made after it. > right. thinko. it's a bit late here. ^^ regards
On 03/09/2023 21:18, Oswald Buddenhagen wrote: > On Sun, Sep 03, 2023 at 08:48:14PM +0100, Phillip Wood wrote: >> On 03/09/2023 20:25, Oswald Buddenhagen wrote: >>> On Sun, Sep 03, 2023 at 07:40:00PM +0100, Phillip Wood wrote: >>>> it only matters for "cherry-pick --skip" >>>> >>> that doesn't seem right. a --skip is just a --continue with a prior >>> reset, more or less. >> >> sequencer_skip() calls rollback_is_safe() which checks the abort >> safety file. >> > that's weird. can you think of a good reason for doing that? I think it is clear from the code - so it does not reset changes after the user has committed the conflict resolution. >>> i'll try to find a better "choke point". >> >> I think that is probably tricky, >> > yeah > >> I'm not really clear what the aim/purpose of this refactoring is. >> > to make my head not explode. > more specifically, to get it out of the way of the rebase path, which is > what i'm actually concerned with. rebase and cherry-pick share the same code path most of the time. In particular "cherry-pick --continue" and "rebase --continue" both use sequencer_continue() as their entry point so I think the best you can do is guard the calls to update_abort_safety_file() with "if (!is_rebase_i(opts))" or add "if (is_rebase_i(opts)) return" to the start of update_abort_safety_file(). > generally, i think this whole ad-hoc state management is a nightmare, > and i'd be surprised if there weren't some more loose ends. > i think i'd aim for an object-oriented-ish design with an encapsulated > state, lazy loading getters, lazy setters, and a commit entry point (or > maybe several partial ones). no idea how that would play out. I've been working on something similar to only write the state to disc when the sequencer stops for user interaction. I'm hoping to have the first set of patches ready to submit in the next development cycle. You can see the branch at [1]. It is very much a work in progress at the moment, the code is mostly OK (I'm running it in my git build) but some commits are empty, others need splitting and the commit messages need a lot of work. The basic idea is to add a private struct that holds the state and write that to disc when pick_commits() returns. Best Wishes Phillip [1] https://github.com/phillipwood/git/commits/wip/sequencer-context >>> if you did a fresh commit before or after the single pick, you'd lose >>> it. >> >> Oh, I can see that you'd lose a commit made before a single pick but I >> don't see how you'd lose a commit made after it. >> > right. thinko. it's a bit late here. ^^ > > regards
On Mon, Sep 04, 2023 at 11:05:22AM +0100, Phillip Wood wrote: >On 03/09/2023 21:18, Oswald Buddenhagen wrote: >> On Sun, Sep 03, 2023 at 08:48:14PM +0100, Phillip Wood wrote: >>> On 03/09/2023 20:25, Oswald Buddenhagen wrote: >>>> On Sun, Sep 03, 2023 at 07:40:00PM +0100, Phillip Wood wrote: >>>>> it only matters for "cherry-pick --skip" >>>>> >>>> that doesn't seem right. a --skip is just a --continue with a prior >>>> reset, more or less. >>> >>> sequencer_skip() calls rollback_is_safe() which checks the abort >>> safety file. >>> >> that's weird. can you think of a good reason for doing that? > >I think it is clear from the code - so it does not reset changes after >the user has committed the conflict resolution. > yeah, i've researched that meanwhile. the background is that the machinery that was originally introduced for abort safety was later (ab-)used for checking whether a --skip makes sense (de81ca3f36, "cherry-pick/revert: add --skip option", 2019-07-02). this made the state file a misnomer (not that "abort-safety" was a particularly good name to start with - i'd have used "latest-head" or some such). but the implementation made no sense to me, so i read the mailing list archive. the result is the attached patch. however, even so, it seems kinda wrong to me: going by HEAD means that dropping commits would also trigger it, which would make the given advice misleading. in fact, the situation this code path is covering is fundamentally different from the normal merge conflict: rather than letting the user resolve it and us finishing the commit, we are rescheduling the pick. but that means that --skip needs to skip whatever the next command is? that doesn't sound right. also, i just tried --continue after a path conflict, and it apparently did the same as --skip, so something is really wrong. also, when we have no _HEAD, actually attempting to `reset --merge` is pointless, no? oh, and i just noticed that the git-prompt is buggy: it doesn't tell me about the interrupted multi-pick nested into an interrupted rebase. ugh, and rebase lets me continue despite still being in the multi-pick. and the path conflict check is made ineffective by the file in question being in .gitignore?! (i force-added config.mak.autogen for testing, and cherry-picking over it goes through just fine.) >> i think i'd aim for an object-oriented-ish design with an >> encapsulated state, lazy loading getters, lazy setters, and a commit >> entry point (or maybe several partial ones). no idea how that would >> play out. > >I've been working on something similar > awesome! (well, except for the rebase nightmare in my own series i expect because of this.) >to only write the state to disc when the sequencer stops for user >interaction. > note that this must cover ctrl-c as well, because the sequencer state must be consistent with HEAD. of course one could also delay updating HEAD, but that hinges on no relevant hooks being present, i think? git-replay has a huge advantage here ... regards
diff --git a/sequencer.c b/sequencer.c index a66dcf8ab2..716384cc7b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -575,10 +575,6 @@ static void update_abort_safety_file(void) { struct object_id head; - /* Do nothing on a single-pick */ - if (!file_exists(git_path_seq_dir())) - return; - if (!repo_get_oid(the_repository, "HEAD", &head)) write_file(git_path_abort_safety_file(), "%s", oid_to_hex(&head)); else @@ -618,7 +614,6 @@ static int fast_forward_to(struct repository *r, strbuf_release(&sb); strbuf_release(&err); ref_transaction_free(transaction); - update_abort_safety_file(); return 0; } @@ -2435,7 +2430,6 @@ static int do_pick_commit(struct repository *r, free_message(commit, &msg); free(author); strbuf_release(&msgbuf); - update_abort_safety_file(); return res; } @@ -5269,8 +5263,9 @@ int sequencer_pick_revisions(struct repository *r, return -1; if (save_opts(opts)) return -1; - update_abort_safety_file(); res = pick_commits(r, &todo_list, opts); + if (todo_list.current < todo_list.nr) + update_abort_safety_file(); todo_list_release(&todo_list); return res; } diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index 3b0fa66c33..170b664c33 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -318,6 +318,15 @@ test_expect_success '--abort does not unsafely change HEAD' ' test_cmp_rev base HEAD ' +test_expect_success '--abort after single pick does not unsafely change HEAD' ' + pristine_detach initial && + test_must_fail git cherry-pick picked anotherpick && + git reset --hard && + git cherry-pick unrelatedpick && + git cherry-pick --abort 2>actual && + test_i18ngrep "You seem to have moved HEAD" actual +' + test_expect_success 'cherry-pick --abort to cancel multiple revert' ' pristine_detach anotherpick && test_expect_code 1 git revert base..picked &&
The only situation where the file's content matters is --continue'ing (after a multi-cherry-pick merge conflict). This means that it is sufficient to write it in a single place, when we are prematurely exiting the main workhorse. This is much easier to reason about than the three dispersed calls originally introduced in 1e41229d ("sequencer: make sequencer abort safer", 2016-12-07). We now can also remove the inefficient file-based check whether the file needs writing, which wasn't even reliable: a single pick executed during an interrupted sequence would bypass the safety. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- Cc: Stephan Beyer <s-beyer@gmx.net> Cc: Johannes Schindelin <johannes.schindelin@gmx.de> Cc: Phillip Wood <phillip.wood123@gmail.com> --- sequencer.c | 9 ++------- t/t3510-cherry-pick-sequence.sh | 9 +++++++++ 2 files changed, 11 insertions(+), 7 deletions(-)