Re: [PATCH v4 0/7] rebase -i: impove handling of failed commands

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Phillip,

On Wed, 6 Sep 2023, Phillip Wood via GitGitGadget wrote:

> Range-diff vs v3:
>
>  1:  1ab1ad2ef07 ! 1:  ae4f873b3d0 rebase -i: move unlink() calls
>      @@ Metadata
>        ## Commit message ##
>           rebase -i: move unlink() calls
>
>      -    At the start of each iteration the loop that picks commits removes
>      -    state files from the previous pick. However some of these are only
>      -    written if there are conflicts and so we break out of the loop after
>      -    writing them. Therefore they only need to be removed when the rebase
>      -    continues, not in each iteration.
>      +    At the start of each iteration the loop that picks commits removes the
>      +    state files from the previous pick. However some of these files are only
>      +    written if there are conflicts in which case we exit the loop before the
>      +    end of the loop body. Therefore they only need to be removed when the
>      +    rebase continues, not at the start of each iteration.
>
>           Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
>  2:  e2a758eb4a5 ! 2:  f540ed1d607 rebase -i: remove patch file after conflict resolution
>      @@ Commit message
>           now used in two different places rebase_path_patch() is added and used
>           to obtain the path for the patch.
>
>      +    To construct the path write_patch() previously used get_dir() which
>      +    returns different paths depending on whether we're rebasing or
>      +    cherry-picking/reverting. As this function is only called when
>      +    rebasing it is safe to use a hard coded string for the directory
>      +    instead. An assertion is added to make sure we don't starting calling
>      +    this function when cherry-picking in the future.
>      +
>           Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
>        ## sequencer.c ##
>      @@ sequencer.c: static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
>         * For the post-rewrite hook, we make a list of rewritten commits and
>         * their new sha1s.  The rewritten-pending list keeps the sha1s of
>       @@ sequencer.c: static int make_patch(struct repository *r,
>      + 	char hex[GIT_MAX_HEXSZ + 1];
>      + 	int res = 0;
>      +
>      ++	if (!is_rebase_i(opts))
>      ++		BUG("make_patch should only be called when rebasing");
>      ++
>      + 	oid_to_hex_r(hex, &commit->object.oid);
>      + 	if (write_message(hex, strlen(hex), rebase_path_stopped_sha(), 1) < 0)
>        		return -1;
>        	res |= write_rebase_head(&commit->object.oid);
>
>  3:  8f6c0e40567 ! 3:  818bdaf772d sequencer: use rebase_path_message()
>      @@ Commit message
>           made function to get the path name instead. This was the last
>           remaining use of the strbuf so remove it as well.
>
>      +    As with the previous patch we now use a hard coded string rather than
>      +    git_dir() when constructing the path. This is safe for the same
>      +    reason (make_patch() is only called when rebasing) and is protected by
>      +    the assertion added in the previous patch.
>      +
>           Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
>        ## sequencer.c ##
>  4:  a1fad70f4b9 = 4:  bd67765a864 sequencer: factor out part of pick_commits()
>  5:  df401945866 ! 5:  f6f330f7063 rebase: fix rewritten list for failed pick
>      @@ Commit message
>           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.
>      +    The code to add a failed command back into the todo list is duplicated
>      +    between pick_one_commit() and the loop in pick_commits(). Both sites
>      +    print advice about the command being rescheduled, decrement the current
>      +    item and save the todo list. To avoid duplicating this code
>      +    pick_one_commit() is modified to set a flag to indicate that the command
>      +    should be rescheduled in the main loop. This simplifies things as only
>      +    the remaining copy of the code needs to be modified to set REBASE_HEAD
>      +    rather than calling error_with_patch().
>
>           Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
>  6:  2ed7cbe5fff = 6:  0ca5fccca17 rebase --continue: refuse to commit after failed command
>  7:  bbe0afde512 = 7:  8d5f6d51e19 rebase -i: fix adding failed command to the todo list

Thank you for indulging me. This iteration looks good to me!

Ciao,
Johannes