| Message ID | 20230809171531.2564829-1-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] sequencer: rectify empty hint in call of require_clean_work_tree() | expand |
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > The canonical way to represent "no error hint" is making it NULL, which > shortcuts the error() call altogether. This fixes the output by removing > the line which said just "error:", which would appear when starting a > rebase whose initial checkout worked fine despite a dirty worktree. This Thanks for adding this info. "git rebase" does not seem to start (i.e. does not perform "initial checkout") from a dirty working tree, with error: cannot rebase: You have unstaged changes. error: Please commit or stash them. at least from my quick attempt. I am not sure if this is actually triggerble? In any case, I've replaced v2 that I had with this version, as the description is much better. The change to the code does look correct but now it does not seem to trigger, it is unclear to me if the fix is merely theoretical (a command-by-command transcript would have helped here). Thanks.
On Wed, Aug 09, 2023 at 02:44:25PM -0700, Junio C Hamano wrote: >"git rebase" does not seem to start (i.e. does not perform "initial >checkout") from a dirty working tree, with > > error: cannot rebase: You have unstaged changes. > error: Please commit or stash them. > >at least from my quick attempt. I am not sure if this is actually >triggerble? > hmm, unless i'm missing something, it is apparently not ... currently. it becomes actually relevant only after my "rebase: improve resumption from incorrect initial todo list" patch, which still needs reworking. so there are several possibilities how to proceed with this: - you merge it as a merely theoretical fix (adjust the commit message yourself if saving the round-trip is more convenient for you) - we postpone the change until it becomes not theoretical - we make sure that the code really is not triggerable and delete it. this would obviously introduce some churn, as i'll need to re-introduce it. but then, i remember that something was wrong with it anyway (i.e., it didn't actually trigger in some cases i expected it to, but i haven't figured out yet why), so the new code would be slightly different anyway. regards
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > it becomes actually relevant only after my "rebase: improve resumption > from incorrect initial todo list" patch, which still needs reworking. We are already past -rc1 by the time you see this response, so this fix is not of immediate urgency. It has been with us for more than several cycles, it does not seem to be readily triggerable, and its effect is merely a single extra empty error message when other things are mentioned there. Even if it may not be triggerable, consistent use of the API is something that is worth aiming for. If I were working on a separate change (i.e. your "resume from bad initial todo") that will make this triggerable, I would explain this patch as a pure clean-up patch to use the API function consistently, and tell readers that there is currently no way to trigger it (assuming that it is the case---I only poked a bit yesterday and would not claim that I did a thorough investigation), and that it will start mattering with a later step in the series. And make it an early part of the series that will contain the "resume from bad initial todo" patch. Thanks.
diff --git a/sequencer.c b/sequencer.c index cc9821ece2..d15a7409d8 100644 --- a/sequencer.c +++ b/sequencer.c @@ -6182,7 +6182,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla if (checkout_onto(r, opts, onto_name, &oid, orig_head)) goto cleanup; - if (require_clean_work_tree(r, "rebase", "", 1, 1)) + if (require_clean_work_tree(r, "rebase", NULL, 1, 1)) goto cleanup; todo_list_write_total_nr(&new_todo); diff --git a/wt-status.c b/wt-status.c index 8a1a4fb1f0..c8c1780566 100644 --- a/wt-status.c +++ b/wt-status.c @@ -2654,8 +2654,12 @@ int require_clean_work_tree(struct repository *r, } if (err) { - if (hint) + if (hint) { + if (!*hint) + BUG("empty hint passed to require_clean_work_tree();" + " use NULL instead"); error("%s", hint); + } if (!gently) exit(128); }
The canonical way to represent "no error hint" is making it NULL, which shortcuts the error() call altogether. This fixes the output by removing the line which said just "error:", which would appear when starting a rebase whose initial checkout worked fine despite a dirty worktree. This was introduced by 97e1873 (rebase -i: rewrite complete_action() in C, 2018-08-28), which did a somewhat inaccurate conversion from shell. To avoid that such bugs re-appear, test for the condition in require_clean_work_tree(). Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- v3: - added BUG() - rewrote commit message again v2: - expanded commit message Cc: Junio C Hamano <gitster@pobox.com> --- sequencer.c | 2 +- wt-status.c | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-)