Skip to content

rebase -r: a bugfix and two status-related improvements #1897

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

rebase -r: a bugfix and two status-related improvements #1897

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c8f77c
rebase -r: do create merge commit after empty resolution
phil-blain Feb 23, 2025
e297b71
wt-status: also abbreviate 'merge' and 'fixup -C' lines during rebase
phil-blain Jan 16, 2025
db01acd
wt-status: suggest 'git rebase --continue' to conclude 'merge' instru…
phil-blain Feb 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion sequencer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4349,6 +4349,7 @@ static int do_merge(struct repository *r,
error(_("could not even attempt to merge '%.*s'"),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Fri, Mar 28, 2025 at 1:03 PM Philippe Blain via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> When a user runs 'git rebase --continue' to conclude a conflicted merge
> during a 'git rebase -r' invocation, we do not create a merge commit if
> the resolution was empty (i.e. if the index and HEAD are identical). We
> simply continue the rebase as if no 'merge' instruction had been given.
> This is confusing since all commits from the side branch are absent from
> the rebased history. What's more, if that 'merge' is the last
> instruction in the todo list, we fail to remove the merge state, such
> that running 'git status' shows we are still merging after the rebase
> has concluded.
> [...]
> Make sure to also remove MERGE_HEAD when a merge command fails to start.
> We already remove MERGE_MSG since e032abd5a0 (rebase: fix rewritten list
> for failed pick, 2023-09-06). Removing MERGE_HEAD ensures that in this
> situation, upon 'git rebase --continue' we still exit early in
> 'commit_staged_changes', without calling 'run_git_commit'. This is
> already covered by t5407.11, which fails without this change because we
> enter 'run_git_commit' and then fail to find 'rebase_path_message'.
>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> +test_expect_success '--continue creates merge commit after empty resolution' '
> +       git reset --hard main &&
> +       git checkout -b rebase_i_merge &&
> +       test_commit unrelated &&
> +       git checkout -b rebase_i_merge_side &&
> +       test_commit side2 main.txt &&
> +       git checkout rebase_i_merge &&
> +       test_commit side1 main.txt &&
> +       PICK=$(git rev-parse --short rebase_i_merge) &&
> +       test_must_fail git merge rebase_i_merge_side &&
> +       echo side1 >main.txt &&
> +       git add main.txt &&
> +       test_tick &&
> +       git commit --no-edit &&
> +       FAKE_LINES="1 2 3 5 6 7 8 9 10 11" &&
> +       export FAKE_LINES &&
> +       test_must_fail git rebase -ir main &&

I don't think you want to be setting FAKE_LINES like this since doing
so will pollute the environment for all tests following this one. You
can find existing precedent in this script which demonstrates the
correct way to handle this case. Specifically, you'd want:

    test_must_fail env FAKE_LINES="1 2 3 5 6 7 8 9 10 11" \
        git rebase -ir main &&

> +       echo side1 >main.txt &&
> +       git add main.txt &&
> +       git rebase --continue &&
> +       git log --merges >out &&
> +       test_grep "Merge branch '\''rebase_i_merge_side'\''" out

You could take advantage of the SQ variable defined by t/test-lib.sh
to make this a bit easier to digest:

    test_grep "Merge branch ${SQ}rebase_i_merge_side${SQ}" out

Or, even simpler, you'll find that some test scripts just use regex
wildcard "." to make the needle even more readable:

    test_grep "Merge branch .rebase_i_merge_side." out

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Fri, Mar 28, 2025 at 1:14 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Mar 28, 2025 at 1:03 PM Philippe Blain via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> > +test_expect_success '--continue creates merge commit after empty resolution' '
> > +       [...]
> > +       git commit --no-edit &&
> > +       FAKE_LINES="1 2 3 5 6 7 8 9 10 11" &&
> > +       export FAKE_LINES &&
> > +       test_must_fail git rebase -ir main &&
>
> I don't think you want to be setting FAKE_LINES like this since doing
> so will pollute the environment for all tests following this one. You
> can find existing precedent in this script which demonstrates the
> correct way to handle this case. Specifically, you'd want:
>
>     test_must_fail env FAKE_LINES="1 2 3 5 6 7 8 9 10 11" \
>         git rebase -ir main &&

To clarify, by "pollute", I mean that it can impact subsequent tests
which don't take care to override FAKE_LINES as necessary. There
certainly are test scripts which use the:

    FAKE_LINES=... &&
    export FAKE_LINES &&

form successfully, but such scripts are careful to override/set
FAKE_LINES in every test. This particular script (t3418), on the other
hand, does not otherwise employ the form in which the variable is
exported, so introducing it in a test which is inserted into the
middle of the script feels dangerous.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Eric,

On Fri, 28 Mar 2025, Eric Sunshine wrote:

> On Fri, Mar 28, 2025 at 1:14 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Fri, Mar 28, 2025 at 1:03 PM Philippe Blain via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > > diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> > > +test_expect_success '--continue creates merge commit after empty resolution' '
> > > +       [...]
> > > +       git commit --no-edit &&
> > > +       FAKE_LINES="1 2 3 5 6 7 8 9 10 11" &&
> > > +       export FAKE_LINES &&
> > > +       test_must_fail git rebase -ir main &&
> >
> > I don't think you want to be setting FAKE_LINES like this since doing
> > so will pollute the environment for all tests following this one. You
> > can find existing precedent in this script which demonstrates the
> > correct way to handle this case. Specifically, you'd want:
> >
> >     test_must_fail env FAKE_LINES="1 2 3 5 6 7 8 9 10 11" \
> >         git rebase -ir main &&
>
> To clarify, by "pollute", I mean that it can impact subsequent tests
> which don't take care to override FAKE_LINES as necessary. There
> certainly are test scripts which use the:
>
>     FAKE_LINES=... &&
>     export FAKE_LINES &&
>
> form successfully, but such scripts are careful to override/set
> FAKE_LINES in every test. This particular script (t3418), on the other
> hand, does not otherwise employ the form in which the variable is
> exported, so introducing it in a test which is inserted into the
> middle of the script feels dangerous.

The entire `FAKE_LINES` paradigm is broken, and since I suspect that it
was me who introduced it, I apologize.

A much better way to have done this would have been to write the string to
a certain file, say, $(git rev-parse --git-path sequencer.pick-lines), and
in the `fake-editor.sh`:

- test for the existence of that file, and if it exists
  - use its contents
  - delete that file

Ciao,
Johannes

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Phillip Wood wrote (reply to this):

Hi Philippe

On 28/03/2025 17:03, Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
> > When a user runs 'git rebase --continue' to conclude a conflicted merge
> during a 'git rebase -r' invocation, we do not create a merge commit if
> the resolution was empty (i.e. if the index and HEAD are identical). We
> simply continue the rebase as if no 'merge' instruction had been given.
> This is confusing since all commits from the side branch are absent from
> the rebased history. What's more, if that 'merge' is the last
> instruction in the todo list, we fail to remove the merge state, such
> that running 'git status' shows we are still merging after the rebase
> has concluded.
> > This happens because in 'sequencer.c::commit_staged_changes', we exit
> early before calling 'run_git_commit' if 'is_clean' is true, i.e. if
> nothing is staged. Fix this by also checking for the presence of
> MERGE_HEAD before exiting early, such that we do call 'run_git_commit'
> when MERGE_HEAD is present. This also ensures that we unlink
> git_path_merge_head later in 'commit_staged_changes' to clear the merge
> state.
> > Make sure to also remove MERGE_HEAD when a merge command fails to start.
> We already remove MERGE_MSG since e032abd5a0 (rebase: fix rewritten list
> for failed pick, 2023-09-06). Removing MERGE_HEAD ensures that in this
> situation, upon 'git rebase --continue' we still exit early in
> 'commit_staged_changes', without calling 'run_git_commit'. This is
> already covered by t5407.11, which fails without this change because we
> enter 'run_git_commit' and then fail to find 'rebase_path_message'.

Thanks for fixing this.

> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>   sequencer.c                |  3 ++-
>   t/t3418-rebase-continue.sh | 24 ++++++++++++++++++++++++
>   2 files changed, 26 insertions(+), 1 deletion(-)
> > diff --git a/sequencer.c b/sequencer.c
> index ad0ab75c8d4..2baaf716a3c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4349,6 +4349,7 @@ static int do_merge(struct repository *r,
>   		error(_("could not even attempt to merge '%.*s'"),
>   		      merge_arg_len, arg);
>   		unlink(git_path_merge_msg(r));
> +		unlink(git_path_merge_head(r));


I think we want to clean up git_path_merge_mode() as well. Perhaps we should call remove_merge_branch_state() instead of deleting the individual files ourselves here.

> +test_expect_success '--continue creates merge commit after empty resolution' '
> +	git reset --hard main &&
> +	git checkout -b rebase_i_merge &&
> +	test_commit unrelated &&
> +	git checkout -b rebase_i_merge_side &&
> +	test_commit side2 main.txt &&
> +	git checkout rebase_i_merge &&
> +	test_commit side1 main.txt &&
> +	PICK=$(git rev-parse --short rebase_i_merge) &&
> +	test_must_fail git merge rebase_i_merge_side &&
> +	echo side1 >main.txt &&
> +	git add main.txt &&
> +	test_tick &&
> +	git commit --no-edit &&
> +	FAKE_LINES="1 2 3 5 6 7 8 9 10 11" &&
> +	export FAKE_LINES &&
> +	test_must_fail git rebase -ir main &&
> +	echo side1 >main.txt &&
> +	git add main.txt &&
> +	git rebase --continue &&
> +	git log --merges >out &&
> +	test_grep "Merge branch '\''rebase_i_merge_side'\''" out
> +'

I wonder if t3430 would be a better home for this as it already has the setup necessary to create a failing merge. It would be good to add a test to check that "git rebase --skip" does not create an empty merge as well.

Thanks

Phillip

>   test_expect_success '--skip after failed fixup cleans commit message' '
>   	test_when_finished "test_might_fail git rebase --abort" &&
>   	git checkout -b with-conflicting-fixup &&

merge_arg_len, arg);
unlink(git_path_merge_msg(r));
unlink(git_path_merge_head(r));
goto leave_merge;
}
/*
Expand Down Expand Up @@ -5364,7 +5365,7 @@ static int commit_staged_changes(struct repository *r,
flags |= AMEND_MSG;
}

if (is_clean) {
if (is_clean && !file_exists(git_path_merge_head(r))) {
if (refs_ref_exists(get_main_ref_store(r),
"CHERRY_PICK_HEAD") &&
refs_delete_ref(get_main_ref_store(r), "",
Expand Down
24 changes: 24 additions & 0 deletions t/t3418-rebase-continue.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,30 @@ test_expect_success 'rebase -r passes merge strategy options correctly' '
git rebase --continue
'

test_expect_success '--continue creates merge commit after empty resolution' '
git reset --hard main &&
git checkout -b rebase_i_merge &&
test_commit unrelated &&
git checkout -b rebase_i_merge_side &&
test_commit side2 main.txt &&
git checkout rebase_i_merge &&
test_commit side1 main.txt &&
PICK=$(git rev-parse --short rebase_i_merge) &&
test_must_fail git merge rebase_i_merge_side &&
echo side1 >main.txt &&
git add main.txt &&
test_tick &&
git commit --no-edit &&
FAKE_LINES="1 2 3 5 6 7 8 9 10 11" &&
export FAKE_LINES &&
test_must_fail git rebase -ir main &&
echo side1 >main.txt &&
git add main.txt &&
git rebase --continue &&
git log --merges >out &&
test_grep "Merge branch '\''rebase_i_merge_side'\''" out
'

test_expect_success '--skip after failed fixup cleans commit message' '
test_when_finished "test_might_fail git rebase --abort" &&
git checkout -b with-conflicting-fixup &&
Expand Down
31 changes: 22 additions & 9 deletions wt-status.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1342,9 +1342,11 @@ static int split_commit_in_progress(struct wt_status *s)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Phillip Wood wrote (reply to this):

Hi Philippe

On 28/03/2025 17:03, Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
> > When "git status" is invoked during a rebase, we print the last commands
> done and the next commands to do, and abbreviate commit hashes found in
> those lines. However, we only abbreviate hashes in 'pick', 'squash' and
> plain 'fixup' lines, not those in 'merge -C' and 'fixup -C' lines, as
> the parsing done in wt-status.c::abbrev_oid_in_line is not prepared for
> such lines.
> > Improve the parsing done by this function by special casing 'fixup' and
> 'merge' such that the hash to abbreviate is the string found in the
> third field of 'split', instead of the second one for other commands.
> Introduce a 'hash' strbuf pointer to point to the correct field in all
> cases.

Sounds good. It is a shame that the parsing here is not better integrated with the sequencer. I think that would be a much bigger task though. The patch looks good and is definitely an improvement on the status quo for the user.

I was going to ask about a test but it looks like one of the tests added in the next patch checks that we abbreviate "merge -C <oid>". It would be worth mentioning that in the commit message.

Thanks

Phillip

> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>   wt-status.c | 31 ++++++++++++++++++++++---------
>   1 file changed, 22 insertions(+), 9 deletions(-)
> > diff --git a/wt-status.c b/wt-status.c
> index 1da5732f57b..d11d9f9f142 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1342,9 +1342,11 @@ static int split_commit_in_progress(struct wt_status *s)
>   >   /*
>    * Turn
> - * "pick d6a2f0303e897ec257dd0e0a39a5ccb709bc2047 some message"
> + * "pick d6a2f0303e897ec257dd0e0a39a5ccb709bc2047 some message" and
> + * "merge -C d6a2f0303e897ec257dd0e0a39a5ccb709bc2047 some-branch"
>    * into
> - * "pick d6a2f03 some message"
> + * "pick d6a2f03 some message" and
> + * "merge -C d6a2f03 some-branch"
>    *
>    * The function assumes that the line does not contain useless spaces
>    * before or after the command.
> @@ -1360,20 +1362,31 @@ static void abbrev_oid_in_line(struct strbuf *line)
>   	    starts_with(line->buf, "l "))
>   		return;
>   > -	split = strbuf_split_max(line, ' ', 3);
> +	split = strbuf_split_max(line, ' ', 4);
>   	if (split[0] && split[1]) {
>   		struct object_id oid;
> -
> +		struct strbuf *hash;
> +
> +		if ((!strcmp(split[0]->buf, "merge ") ||
> +		     !strcmp(split[0]->buf, "m "    ) ||
> +		     !strcmp(split[0]->buf, "fixup ") ||
> +		     !strcmp(split[0]->buf, "f "    )) &&
> +		    (!strcmp(split[1]->buf, "-C ") ||
> +		     !strcmp(split[1]->buf, "-c "))) {
> +			hash = split[2];
> +		} else {
> +			hash = split[1];
> +		}
>   		/*
>   		 * strbuf_split_max left a space. Trim it and re-add
>   		 * it after abbreviation.
>   		 */
> -		strbuf_trim(split[1]);
> -		if (!repo_get_oid(the_repository, split[1]->buf, &oid)) {
> -			strbuf_reset(split[1]);
> -			strbuf_add_unique_abbrev(split[1], &oid,
> +		strbuf_trim(hash);
> +		if (!repo_get_oid(the_repository, hash->buf, &oid)) {
> +			strbuf_reset(hash);
> +			strbuf_add_unique_abbrev(hash, &oid,
>   						 DEFAULT_ABBREV);
> -			strbuf_addch(split[1], ' ');
> +			strbuf_addch(hash, ' ');
>   			strbuf_reset(line);
>   			for (i = 0; split[i]; i++)
>   				strbuf_addbuf(line, split[i]);

/*
* Turn
* "pick d6a2f0303e897ec257dd0e0a39a5ccb709bc2047 some message"
* "pick d6a2f0303e897ec257dd0e0a39a5ccb709bc2047 some message" and
* "merge -C d6a2f0303e897ec257dd0e0a39a5ccb709bc2047 some-branch"
* into
* "pick d6a2f03 some message"
* "pick d6a2f03 some message" and
* "merge -C d6a2f03 some-branch"
*
* The function assumes that the line does not contain useless spaces
* before or after the command.
Expand All @@ -1360,20 +1362,31 @@ static void abbrev_oid_in_line(struct strbuf *line)
starts_with(line->buf, "l "))
return;

split = strbuf_split_max(line, ' ', 3);
split = strbuf_split_max(line, ' ', 4);
if (split[0] && split[1]) {
struct object_id oid;

struct strbuf *hash;

if ((!strcmp(split[0]->buf, "merge ") ||
!strcmp(split[0]->buf, "m " ) ||
!strcmp(split[0]->buf, "fixup ") ||
!strcmp(split[0]->buf, "f " )) &&
(!strcmp(split[1]->buf, "-C ") ||
!strcmp(split[1]->buf, "-c "))) {
hash = split[2];
} else {
hash = split[1];
}
/*
* strbuf_split_max left a space. Trim it and re-add
* it after abbreviation.
*/
strbuf_trim(split[1]);
if (!repo_get_oid(the_repository, split[1]->buf, &oid)) {
strbuf_reset(split[1]);
strbuf_add_unique_abbrev(split[1], &oid,
strbuf_trim(hash);
if (!repo_get_oid(the_repository, hash->buf, &oid)) {
strbuf_reset(hash);
strbuf_add_unique_abbrev(hash, &oid,
DEFAULT_ABBREV);
strbuf_addch(split[1], ' ');
strbuf_addch(hash, ' ');
strbuf_reset(line);
for (i = 0; split[i]; i++)
strbuf_addbuf(line, split[i]);
Expand Down