From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
Stefan Haller <lists@haller-berlin.de>,
Phillip Wood <phillip.wood123@gmail.com>,
Eric Sunshine <sunshine@sunshineco.com>,
Glen Choo <chooglen@google.com>,
Phillip Wood <phillip.wood@dunelm.org.uk>,
Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH v3 5/7] rebase: fix rewritten list for failed pick
Date: Wed, 23 Aug 2023 10:55:53 +0200 (CEST) [thread overview]
Message-ID: <304553b3-3668-0b98-c6dc-3c047fd9e34c@gmx.de> (raw)
In-Reply-To: <df4019458665eccf2b16cdf1d6c1061186a62711.1690903412.git.gitgitgadget@gmail.com>
Hi Phillip,
On Tue, 1 Aug 2023, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> git rebase keeps a list that maps the OID of each commit before it was
> rebased to the OID of the equivalent commit after the rebase. This list
> is used to drive the "post-rewrite" hook that is called at the end of a
> successful rebase. When a rebase stops for the user to resolve merge
> conflicts the OID of the commit being picked is written to
> ".git/rebase-merge/stopped-sha". Then when the rebase is continued that
> OID is added to the list of rewritten commits. Unfortunately if a commit
> cannot be picked because it would overwrite an untracked file we still
> write the "stopped-sha1" file. This means that when the rebase is
> continued the commit is added into the list of rewritten commits even
> though it has not been picked yet.
>
> Fix this by not calling error_with_patch() for failed commands. The pick
> has failed so there is nothing to commit and therefore we do not want to
> set up the state files for committing staged changes when the rebase
> continues. This change means we no-longer write a patch for the failed
> command or display the error message printed by error_with_patch(). As
> the command has failed the patch isn't really useful and in any case the
> user can inspect the commit associated with the failed command by
> inspecting REBASE_HEAD. Unless the user has disabled it we already print
> an advice message that is more helpful than the message from
> error_with_patch() which the user will still see. Even if the advice is
> disabled the user will see the messages from the merge machinery
> detailing the problem.
>
> To simplify writing REBASE_HEAD in this case pick_one_commit() is
> modified to avoid duplicating the code that adds the failed command
> back into the todo list.
This motivates the change well, and answered all but one of the questions
I had about it, being:
> diff --git a/sequencer.c b/sequencer.c
> index 62277e7bcc1..e25abfd2fb4 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> [...]
> @@ -4658,12 +4659,8 @@ static int pick_one_commit(struct repository *r,
> check_todo);
> if (is_rebase_i(opts) && res < 0) {
> /* Reschedule */
> - advise(_(rescheduled_advice),
> - get_item_line_length(todo_list, todo_list->current),
> - get_item_line(todo_list, todo_list->current));
> - todo_list->current--;
Why is it okay to remove this decrement?
Here is why: The code that calls `save_todo()` in the `if (reschedule)`
block of the loop of `pick_commits()` _duplicates_ the logic that is
removed here, including the advice and the decrementing of `current`.
I had to instrument the code before and after this patch to figure this
out, as I had missed the fact that the now-remaining code also decremented
the `current` attribute.
So: All is good with this patch. If you'd like to amend the commit message
accordingly, I would not be opposed, but I could now live equally as
easily without it.
> - if (save_todo(todo_list, opts))
> - return -1;
> + *reschedule = 1;
> + return -1;
> }
> if (item->command == TODO_EDIT) {
> struct commit *commit = item->commit;
I'd like to point out how delighted I am about this careful test case:
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index 96ae0edf1e1..4938ebb1c17 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -165,12 +165,12 @@ test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
> test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
> test_tick &&
> test_must_fail git rebase -ir HEAD &&
> + test_cmp_rev REBASE_HEAD H^0 &&
> grep "^merge -C .* G$" .git/rebase-merge/done &&
> grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
> - test_path_is_file .git/rebase-merge/patch &&
> + test_path_is_missing .git/rebase-merge/patch &&
>
> : fail because of merge conflict &&
> - rm G.t .git/rebase-merge/patch &&
> git reset --hard conflicting-G &&
> test_must_fail git rebase --continue &&
> ! grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
> diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
> index 5f3ff051ca2..ad7f8c6f002 100755
> --- a/t/t5407-post-rewrite-hook.sh
> +++ b/t/t5407-post-rewrite-hook.sh
> @@ -17,6 +17,12 @@ test_expect_success 'setup' '
> git checkout A^0 &&
> test_commit E bar E &&
> test_commit F foo F &&
> + git checkout B &&
> + git merge E &&
> + git tag merge-E &&
> + test_commit G G &&
> + test_commit H H &&
> + test_commit I I &&
> git checkout main &&
>
> test_hook --setup post-rewrite <<-EOF
> @@ -173,6 +179,48 @@ test_fail_interactive_rebase () {
> )
> }
>
> +test_expect_success 'git rebase with failed pick' '
> + clear_hook_input &&
> + cat >todo <<-\EOF &&
> + exec >bar
> + merge -C merge-E E
> + exec >G
> + pick G
> + exec >H 2>I
> + pick H
> + fixup I
> + EOF
> +
> + (
> + set_replace_editor todo &&
> + test_must_fail git rebase -i D D 2>err
> + ) &&
> + grep "would be overwritten" err &&
> + rm bar &&
> +
> + test_must_fail git rebase --continue 2>err &&
> + grep "would be overwritten" err &&
> + rm G &&
> +
> + test_must_fail git rebase --continue 2>err &&
> + grep "would be overwritten" err &&
> + rm H &&
> +
> + test_must_fail git rebase --continue 2>err &&
> + grep "would be overwritten" err &&
> + rm I &&
> +
> + git rebase --continue &&
> + echo rebase >expected.args &&
> + cat >expected.data <<-EOF &&
> + $(git rev-parse merge-E) $(git rev-parse HEAD~2)
> + $(git rev-parse G) $(git rev-parse HEAD~1)
> + $(git rev-parse H) $(git rev-parse HEAD)
> + $(git rev-parse I) $(git rev-parse HEAD)
> + EOF
> + verify_hook_input
> +'
> +
> test_expect_success 'git rebase -i (unchanged)' '
> git reset --hard D &&
> clear_hook_input &&
Here is my ACK.
Thank you,
Johannes
next prev parent reply other threads:[~2023-08-23 9:46 UTC|newest]
Thread overview: 80+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-19 14:48 [PATCH] rebase -i: do not update "done" when rescheduling command Phillip Wood via GitGitGadget
2023-03-20 7:29 ` Stefan Haller
2023-03-20 17:46 ` Junio C Hamano
2023-03-24 10:50 ` Phillip Wood
2023-03-24 15:49 ` Junio C Hamano
2023-03-24 16:22 ` Phillip Wood
2023-03-27 7:04 ` Johannes Schindelin
2023-08-03 12:56 ` Phillip Wood
2023-08-23 8:54 ` Johannes Schindelin
2023-04-21 14:57 ` [PATCH v2 0/6] rebase -i: impove handling of failed commands Phillip Wood via GitGitGadget
2023-04-21 14:57 ` [PATCH v2 1/6] rebase -i: move unlink() calls Phillip Wood via GitGitGadget
2023-04-21 17:22 ` Junio C Hamano
2023-04-27 10:15 ` Phillip Wood
2023-04-21 14:57 ` [PATCH v2 2/6] rebase -i: remove patch file after conflict resolution Phillip Wood via GitGitGadget
2023-04-21 19:01 ` Junio C Hamano
2023-04-27 10:17 ` Phillip Wood
2023-06-21 20:14 ` Glen Choo
2023-07-14 10:08 ` Phillip Wood
2023-07-14 16:51 ` Junio C Hamano
2023-07-17 15:39 ` Phillip Wood
2023-04-21 14:57 ` [PATCH v2 3/6] sequencer: factor out part of pick_commits() Phillip Wood via GitGitGadget
2023-04-21 19:12 ` Eric Sunshine
2023-04-21 19:31 ` Junio C Hamano
2023-04-21 20:00 ` Phillip Wood
2023-04-21 21:21 ` Junio C Hamano
2023-04-21 14:57 ` [PATCH v2 4/6] rebase --continue: refuse to commit after failed command Phillip Wood via GitGitGadget
2023-04-21 19:14 ` Eric Sunshine
2023-04-21 21:05 ` Junio C Hamano
2023-06-21 20:35 ` Glen Choo
2023-04-21 14:57 ` [PATCH v2 5/6] rebase: fix rewritten list for failed pick Phillip Wood via GitGitGadget
2023-06-21 20:49 ` Glen Choo
2023-07-25 15:42 ` Phillip Wood
2023-07-25 16:46 ` Glen Choo
2023-07-26 13:08 ` Phillip Wood
2023-07-26 17:48 ` Glen Choo
2023-07-28 13:19 ` Phillip Wood
2023-04-21 14:57 ` [PATCH v2 6/6] rebase -i: fix adding failed command to the todo list Phillip Wood via GitGitGadget
2023-06-21 20:59 ` Glen Choo
2023-04-21 16:56 ` [PATCH v2 0/6] rebase -i: impove handling of failed commands Junio C Hamano
2023-06-21 20:07 ` Glen Choo
2023-08-01 15:23 ` [PATCH v3 0/7] " Phillip Wood via GitGitGadget
2023-08-01 15:23 ` [PATCH v3 1/7] rebase -i: move unlink() calls Phillip Wood via GitGitGadget
2023-08-01 17:22 ` Junio C Hamano
2023-08-01 18:42 ` Phillip Wood
2023-08-01 19:31 ` Junio C Hamano
2023-08-01 15:23 ` [PATCH v3 2/7] rebase -i: remove patch file after conflict resolution Phillip Wood via GitGitGadget
2023-08-01 17:23 ` Junio C Hamano
2023-08-01 18:47 ` Phillip Wood
2023-08-01 15:23 ` [PATCH v3 3/7] sequencer: use rebase_path_message() Phillip Wood via GitGitGadget
2023-08-01 17:23 ` Junio C Hamano
2023-08-01 18:49 ` Phillip Wood
2023-08-02 22:02 ` Junio C Hamano
2023-08-01 15:23 ` [PATCH v3 4/7] sequencer: factor out part of pick_commits() Phillip Wood via GitGitGadget
2023-08-23 8:55 ` Johannes Schindelin
2023-08-01 15:23 ` [PATCH v3 5/7] rebase: fix rewritten list for failed pick Phillip Wood via GitGitGadget
2023-08-23 8:55 ` Johannes Schindelin [this message]
2023-09-04 14:31 ` Phillip Wood
2023-08-01 15:23 ` [PATCH v3 6/7] rebase --continue: refuse to commit after failed command Phillip Wood via GitGitGadget
2023-08-23 9:01 ` Johannes Schindelin
2023-09-04 14:37 ` Phillip Wood
2023-09-05 11:17 ` Johannes Schindelin
2023-09-05 14:57 ` Junio C Hamano
2023-09-05 15:25 ` Phillip Wood
2023-08-01 15:23 ` [PATCH v3 7/7] rebase -i: fix adding failed command to the todo list Phillip Wood via GitGitGadget
2023-08-02 22:10 ` [PATCH v3 0/7] rebase -i: impove handling of failed commands Junio C Hamano
2023-08-03 13:06 ` Phillip Wood
2023-08-09 13:08 ` Phillip Wood
2023-08-07 20:16 ` Glen Choo
2023-08-09 10:06 ` Phillip Wood
2023-09-06 15:22 ` [PATCH v4 " Phillip Wood via GitGitGadget
2023-09-06 15:22 ` [PATCH v4 1/7] rebase -i: move unlink() calls Phillip Wood via GitGitGadget
2023-09-06 15:22 ` [PATCH v4 2/7] rebase -i: remove patch file after conflict resolution Phillip Wood via GitGitGadget
2023-09-06 15:22 ` [PATCH v4 3/7] sequencer: use rebase_path_message() Phillip Wood via GitGitGadget
2023-09-06 15:22 ` [PATCH v4 4/7] sequencer: factor out part of pick_commits() Phillip Wood via GitGitGadget
2023-09-06 15:22 ` [PATCH v4 5/7] rebase: fix rewritten list for failed pick Phillip Wood via GitGitGadget
2023-09-06 15:22 ` [PATCH v4 6/7] rebase --continue: refuse to commit after failed command Phillip Wood via GitGitGadget
2023-09-06 15:22 ` [PATCH v4 7/7] rebase -i: fix adding failed command to the todo list Phillip Wood via GitGitGadget
2023-09-06 21:01 ` [PATCH v4 0/7] rebase -i: impove handling of failed commands Junio C Hamano
2023-09-07 9:56 ` Johannes Schindelin
2023-09-07 20:33 ` 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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=304553b3-3668-0b98-c6dc-3c047fd9e34c@gmx.de \
--to=johannes.schindelin@gmx.de \
--cc=chooglen@google.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=lists@haller-berlin.de \
--cc=phillip.wood123@gmail.com \
--cc=phillip.wood@dunelm.org.uk \
--cc=sunshine@sunshineco.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.
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 NNTP newsgroup(s).