| Message ID | 20230323162235.995574-6-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sequencer refactoring | expand |
Hi Oswald On 23/03/2023 16:22, Oswald Buddenhagen wrote: > Creating a suitable todo file is a potentially labor-intensive process, > so be less cavalier about discarding it when something goes wrong (e.g., > the user messed with the repo while editing the todo). I was thinking about this problem the other day in the context of rescheduling commands when they cannot be executed because they would overwrite an untracked file. My thought was that we should prepend a "reset" command to the todo list so that the checkout happened when the user continued the rebase. How does this patch ensure the checkout happens when the user continues the rebase? Best Wishes Phillip > Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> > --- > builtin/rebase.c | 1 + > sequencer.c | 4 ++++ > sequencer.h | 1 + > t/t3404-rebase-interactive.sh | 3 ++- > 4 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index a309addd50..728c869db4 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -153,6 +153,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts) > replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP); > replay.quiet = !(opts->flags & REBASE_NO_QUIET); > replay.verbose = opts->flags & REBASE_VERBOSE; > + replay.precious_todo = opts->flags & REBASE_INTERACTIVE_EXPLICIT; > replay.reschedule_failed_exec = opts->reschedule_failed_exec; > replay.committer_date_is_author_date = > opts->committer_date_is_author_date; > diff --git a/sequencer.c b/sequencer.c > index b1c29c8802..f8a7f4e721 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -4570,6 +4570,10 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts, > .default_reflog_action = sequencer_reflog_action(opts) > }; > if (reset_head(r, &ropts)) { > + // Editing the todo may have been costly; don't just discard it. > + if (opts->precious_todo) > + exit(1); // Error was already printed > + > apply_autostash(rebase_path_autostash()); > sequencer_remove_state(opts); > return error(_("could not detach HEAD")); > diff --git a/sequencer.h b/sequencer.h > index 1a3e616af2..a1b8ca6eb1 100644 > --- a/sequencer.h > +++ b/sequencer.h > @@ -47,6 +47,7 @@ struct replay_opts { > int keep_redundant_commits; > int verbose; > int quiet; > + int precious_todo; > int reschedule_failed_exec; > int committer_date_is_author_date; > int ignore_date; > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index ff0afad63e..c625aad10a 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -288,13 +288,14 @@ test_expect_success 'abort' ' > ' > > test_expect_success 'abort with error when new base cannot be checked out' ' > + test_when_finished "git rebase --abort ||:" && > git rm --cached file1 && > git commit -m "remove file in base" && > test_must_fail git rebase -i primary > output 2>&1 && > test_i18ngrep "The following untracked working tree files would be overwritten by checkout:" \ > output && > test_i18ngrep "file1" output && > - test_path_is_missing .git/rebase-merge && > + test_path_is_dir .git/rebase-merge && > rm file1 && > git reset --hard HEAD^ > '
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > Creating a suitable todo file is a potentially labor-intensive process, > so be less cavalier about discarding it when something goes wrong (e.g., > the user messed with the repo while editing the todo). Is there a reason why we do not always keep it? Why is the file sometimes precious but not precious at all in other times? Tying the previous bit to "-i was explicitly given" feels a bit unintuitive---when the sequencer machinery was implicitly chosen, and gives the control back to the user, should a user be forbidden to muck with the todo list? > diff --git a/builtin/rebase.c b/builtin/rebase.c > index a309addd50..728c869db4 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -153,6 +153,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts) > replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP); > replay.quiet = !(opts->flags & REBASE_NO_QUIET); > replay.verbose = opts->flags & REBASE_VERBOSE; > + replay.precious_todo = opts->flags & REBASE_INTERACTIVE_EXPLICIT; > replay.reschedule_failed_exec = opts->reschedule_failed_exec; > replay.committer_date_is_author_date = > opts->committer_date_is_author_date; > diff --git a/sequencer.c b/sequencer.c > index b1c29c8802..f8a7f4e721 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -4570,6 +4570,10 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts, > .default_reflog_action = sequencer_reflog_action(opts) > }; > if (reset_head(r, &ropts)) { > + // Editing the todo may have been costly; don't just discard it. > + if (opts->precious_todo) > + exit(1); // Error was already printed No // comments, please. > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index ff0afad63e..c625aad10a 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -288,13 +288,14 @@ test_expect_success 'abort' ' > ' > > test_expect_success 'abort with error when new base cannot be checked out' ' > + test_when_finished "git rebase --abort ||:" && > git rm --cached file1 && > git commit -m "remove file in base" && > test_must_fail git rebase -i primary > output 2>&1 && > test_i18ngrep "The following untracked working tree files would be overwritten by checkout:" \ > output && > test_i18ngrep "file1" output && > - test_path_is_missing .git/rebase-merge && > + test_path_is_dir .git/rebase-merge && > rm file1 && > git reset --hard HEAD^ > ' Are we happy to just see that the directory still exists? I thought the original motivation explained in the proposed log message was to keep the todo list file, so shouldn't you be checking if the file is there (and if you can reliably ensure that the file has contents that are expected, that would be even better)? Also, as the keeping of the todo list is now conditional, we should have another test that checks that the file is gone when that condition ("INTERACTIVE_EXPLICIT"?) does not trigger, I think. Other than that, nicely written. Thanks.
On Thu, Mar 23, 2023 at 07:31:04PM +0000, Phillip Wood wrote: >On 23/03/2023 16:22, Oswald Buddenhagen wrote: >> Creating a suitable todo file is a potentially labor-intensive process, >> so be less cavalier about discarding it when something goes wrong (e.g., >> the user messed with the repo while editing the todo). > >I was thinking about this problem the other day in the context of >rescheduling commands when they cannot be executed because they would >overwrite an untracked file. My thought was that we should prepend a >"reset" command to the todo list so that the checkout happened when the >user continued the rebase. > so you basically want to convert the magic `onto` into an explicit todo command? i'm not sure what the advantage would be, and i certainly can think of disadvantages re. usability and backwards compat. >How does this patch ensure the checkout happens when the user continues >the rebase? > the idea was never that the user --continue's. we're talking about a fatal error, and the patch's purpose is only to allow the user to salvage their work manually. it's an interesting question, though, esp. in light of patch 8/8 of this series.
On Thu, Mar 23, 2023 at 01:16:47PM -0700, Junio C Hamano wrote: >Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > >> Creating a suitable todo file is a potentially labor-intensive process, >> so be less cavalier about discarding it when something goes wrong (e.g., >> the user messed with the repo while editing the todo). > >Is there a reason why we do not always keep it? Why is the file >sometimes precious but not precious at all in other times? > the unedited initial todo just isn't precious. that implies that in a non-interactive rebase, it is always worthless at the time of the initial reset. >Tying the previous bit to "-i was explicitly given" feels a bit >unintuitive---when the sequencer machinery was implicitly chosen, >and gives the control back to the user, should a user be forbidden >to muck with the todo list? > that would be an --edit-todo and --continue during a mid-rebase stop. rather different case. >No // comments, please. > (apparently with a special exception for examples in the apidocs, presumably because escaping nested comments would be just too ugly.) >> - test_path_is_missing .git/rebase-merge && >> + test_path_is_dir .git/rebase-merge && > >Are we happy to just see that the directory still exists? I thought >the original motivation explained in the proposed log message was to >keep the todo list file, so shouldn't you be checking if the file is >there > fair point. >(and if you can reliably ensure that the file has contents >that are expected, that would be even better)? > i could grep for a shortened sha1 i would obtain from the branch. but given that the error scenario of a present but somehow corrupted todo seems implausible given the circumstances, that seems like overkill. >Also, as the keeping of the todo list is now conditional, we should >have another test that checks that the file is gone when that >condition ("INTERACTIVE_EXPLICIT"?) does not trigger, I think. > that would be for t3400-rebase.sh. i suppose we could extend 'Show verbose error when HEAD could not be detached'.
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > On Thu, Mar 23, 2023 at 01:16:47PM -0700, Junio C Hamano wrote: >>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: >> >>> Creating a suitable todo file is a potentially labor-intensive process, >>> so be less cavalier about discarding it when something goes wrong (e.g., >>> the user messed with the repo while editing the todo). >> >>Is there a reason why we do not always keep it? Why is the file >>sometimes precious but not precious at all in other times? >> > the unedited initial todo just isn't precious. that implies that in a > non-interactive rebase, it is always worthless at the time of the > initial reset. I see. Thanks for clarifying. Just FYI, the primary purpose reviewers ask questions on the proposed change is to help submitters polish their patch (both the proposed log message text and the code) to clarify points they found hard to understand and/or they suspect would be hard to understand for other readers. So please do not be happy by just receiving "I see, thanks" and stop there. Instead, please update the patch so that future readers would not have to ask similar question again. >>(and if you can reliably ensure that the file has contents >>that are expected, that would be even better)? >> > i could grep for a shortened sha1 i would obtain from the branch. but > given that the error scenario of a present but somehow corrupted todo > seems implausible given the circumstances, that seems like overkill. It is OK. If it were easy to prepare the "todo should look like this" golden copy, then doing test_cmp the actual file with it would have been a simple way to ensure both existence of and sane contents in the file at the same time, but if it isn't cheap to prepare such an expected output, I agree with you that it is not worth the extra effort. Thanks.
Hi Oswald On 23/03/2023 22:38, Oswald Buddenhagen wrote: > On Thu, Mar 23, 2023 at 07:31:04PM +0000, Phillip Wood wrote: >> On 23/03/2023 16:22, Oswald Buddenhagen wrote: >>> Creating a suitable todo file is a potentially labor-intensive process, >>> so be less cavalier about discarding it when something goes wrong (e.g., >>> the user messed with the repo while editing the todo). >> >> I was thinking about this problem the other day in the context of >> rescheduling commands when they cannot be executed because they would >> overwrite an untracked file. My thought was that we should prepend a >> "reset" command to the todo list so that the checkout happened when >> the user continued the rebase. >> > so you basically want to convert the magic `onto` into an explicit todo > command? i'm not sure what the advantage would be, and i certainly can > think of disadvantages re. usability and backwards compat. If the initial checkout of "onto" fails I want the rebase to stop so the user can try and fix the problem (usually remove an untracked file) and then run "git rebase --continue" to continue the rebase including the initial checkout. Adding a "reset" command to the beginning of the todo list when the initial checkout fails is one way of achieving that. >> How does this patch ensure the checkout happens when the user >> continues the rebase? >> > the idea was never that the user --continue's. we're talking about a > fatal error, If it is a fatal error what is stopping the user from running "rebase --continue" and wreaking havoc? You seem to be expecting the user to know that they need to (1) run "git rebase --edit-todo" to save the todo list somewhere safe (2) run "git rebase --abort" to abort the rebase and restore any autostash. (Have you checked that --abort is safe to run when HEAD is not detached?) (3) fix whatever prevented the checkout from working (4) re-run "git rebase" and restore the saved todo list when prompted to edit it It would be much more user friendly to simply allow them to fix the problem with the checkout and run "git rebase --continue" Best Wishes Phillip > and the patch's purpose is only to allow the user to > salvage their work manually. > it's an interesting question, though, esp. in light of patch 8/8 of this > series.
On Fri, Mar 24, 2023 at 02:15:47PM +0000, Phillip Wood wrote: >On 23/03/2023 22:38, Oswald Buddenhagen wrote: >> so you basically want to convert the magic `onto` into an explicit >> todo command? i'm not sure what the advantage would be, and i >> certainly can think of disadvantages re. usability and backwards >> compat. > >If the initial checkout of "onto" fails I want the rebase to stop so the >user can try and fix the problem (usually remove an untracked file) and >then run "git rebase --continue" to continue the rebase including the >initial checkout. Adding a "reset" command to the beginning of the todo >list when the initial checkout fails is one way of achieving that. > i suppose, but patch 8/8 does pretty much the same, only with fewer side effects. >>> How does this patch ensure the checkout happens when the user >>> continues the rebase? >>> >> the idea was never that the user --continue's. we're talking about a >> fatal error, > >If it is a fatal error what is stopping the user from running "rebase >--continue" and wreaking havoc? [...] > >It would be much more user friendly to simply allow them to fix the >problem with the checkout and run "git rebase --continue" > i'll reorder the patch to the end of the series, as after the currently last patch we'll be in a much better position to deal with the fallout in a sane way. thanks!
diff --git a/builtin/rebase.c b/builtin/rebase.c index a309addd50..728c869db4 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -153,6 +153,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts) replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP); replay.quiet = !(opts->flags & REBASE_NO_QUIET); replay.verbose = opts->flags & REBASE_VERBOSE; + replay.precious_todo = opts->flags & REBASE_INTERACTIVE_EXPLICIT; replay.reschedule_failed_exec = opts->reschedule_failed_exec; replay.committer_date_is_author_date = opts->committer_date_is_author_date; diff --git a/sequencer.c b/sequencer.c index b1c29c8802..f8a7f4e721 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4570,6 +4570,10 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts, .default_reflog_action = sequencer_reflog_action(opts) }; if (reset_head(r, &ropts)) { + // Editing the todo may have been costly; don't just discard it. + if (opts->precious_todo) + exit(1); // Error was already printed + apply_autostash(rebase_path_autostash()); sequencer_remove_state(opts); return error(_("could not detach HEAD")); diff --git a/sequencer.h b/sequencer.h index 1a3e616af2..a1b8ca6eb1 100644 --- a/sequencer.h +++ b/sequencer.h @@ -47,6 +47,7 @@ struct replay_opts { int keep_redundant_commits; int verbose; int quiet; + int precious_todo; int reschedule_failed_exec; int committer_date_is_author_date; int ignore_date; diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index ff0afad63e..c625aad10a 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -288,13 +288,14 @@ test_expect_success 'abort' ' ' test_expect_success 'abort with error when new base cannot be checked out' ' + test_when_finished "git rebase --abort ||:" && git rm --cached file1 && git commit -m "remove file in base" && test_must_fail git rebase -i primary > output 2>&1 && test_i18ngrep "The following untracked working tree files would be overwritten by checkout:" \ output && test_i18ngrep "file1" output && - test_path_is_missing .git/rebase-merge && + test_path_is_dir .git/rebase-merge && rm file1 && git reset --hard HEAD^ '
Creating a suitable todo file is a potentially labor-intensive process, so be less cavalier about discarding it when something goes wrong (e.g., the user messed with the repo while editing the todo). Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- builtin/rebase.c | 1 + sequencer.c | 4 ++++ sequencer.h | 1 + t/t3404-rebase-interactive.sh | 3 ++- 4 files changed, 8 insertions(+), 1 deletion(-)