Skip to content

[RFC] merge --autostash: apply autostash in more cases #1002

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

Closed
phil-blain wants to merge 4 commits into from
Closed

[RFC] merge --autostash: apply autostash in more cases #1002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
85016e6
merge: add missing word "strategy" to a message
phil-blain Jul 22, 2021
5d3d95c
Documentation: define 'MERGE_AUTOSTASH'
phil-blain Jul 22, 2021
86becf7
merge: apply autostash if fast-forward fails
phil-blain Jul 21, 2021
e02b945
merge: apply autostash if merge strategy fails
phil-blain Jul 21, 2021
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 Documentation/merge-options.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,8 @@ endif::git-pull[]
--autostash::

@gitgitgadget gitgitgadget Bot Jul 23, 2021

Copy link
Copy Markdown

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, Felipe Contreras wrote (reply to this):

Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <levraiphilippeblain@gmail.com>

> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -154,7 +154,8 @@ endif::git-pull[]
>  --autostash::
>  --no-autostash::
>  	Automatically create a temporary stash entry before the operation
> -	begins, and apply it after the operation ends.  This means
> +	begins, record it in the special ref `MERGE_AUTOSTASH`
> +	and apply it after the operation ends.  This means
>  	that you can run the operation on a dirty worktree.  However, use
>  	with care: the final stash application after a successful
>  	merge might result in non-trivial conflicts.

Although normally I'm against mentioning low-level technical details in
high-level documentation, there doesn't seem to be a better location to
explain MERGE_AUTOSTASH, and it's just a simple sentence.

Fine by me.

-- 
Felipe Contreras

--no-autostash::
Automatically create a temporary stash entry before the operation
begins, and apply it after the operation ends. This means
begins, record it in the special ref `MERGE_AUTOSTASH`
and apply it after the operation ends. This means
that you can run the operation on a dirty worktree. However, use
with care: the final stash application after a successful
merge might result in non-trivial conflicts.
Expand Down
4 changes: 3 additions & 1 deletion builtin/merge.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1560,6 +1560,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
&head_commit->object.oid,

@gitgitgadget gitgitgadget Bot Jul 23, 2021

Copy link
Copy Markdown

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, Felipe Contreras wrote (reply to this):

Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <levraiphilippeblain@gmail.com>

> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1560,6 +1560,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  					  &head_commit->object.oid,
>  					  &commit->object.oid,
>  					  overwrite_ignore)) {
> +			apply_autostash(git_path_merge_autostash(the_repository));
>  			ret = 1;
>  			goto done;
>  		}

I can verify that this fixes my simple test.

> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 1cbc9715a81..216113d3537 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -122,6 +122,8 @@ test_expect_success 'setup' '
>  	c0=$(git rev-parse HEAD) &&
>  	cp file.1 file &&
>  	git add file &&
> +	cp file.1 other &&
> +	git add other &&
>  	test_tick &&
>  	git commit -m "commit 1" &&
>  	git tag c1 &&
> @@ -711,6 +713,15 @@ test_expect_success 'fast-forward merge with --autostash' '
>  	test_cmp result.1-5 file
>  '
>  
> +test_expect_success 'failed fast-forward merge with --autostash' '
> +	git reset --hard c0 &&
> +	git merge-file file file.orig file.5 &&
> +	cp file.5 other &&
> +	test_must_fail git merge --autostash c1 2>err &&
> +	test_i18ngrep "Applied autostash." err &&

I've heard others test we are moving away from test_i18ngrep in favor of
grep.

> +	test_cmp file.5 file
> +'
> +
>  test_expect_success 'octopus merge with --autostash' '
>  	git reset --hard c1 &&
>  	git merge-file file file.orig file.3 &&

-- 
Felipe Contreras

@gitgitgadget gitgitgadget Bot Jul 23, 2021

Copy link
Copy Markdown

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, Junio C Hamano wrote (reply to this):

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> Since 'git merge' learned '--autostash' in a03b55530a (merge: teach
> --autostash option, 2020-04-07), 'cmd_merge', in the fast-forward case,
> calls 'create_autostash' before calling 'checkout_fast_forward' if
> '--autostash' is given.
>
> However, if 'checkout_fast_forward' fails, the autostash is not applied
> to the working tree, nor saved in the stash list, since the code simply
> calls 'goto done'.
>
> Be more helpful to the user by applying the autostash in that case.
>
> An easy way to test a failing fast-forward is when we are merging a
> branch that has a tracked file that conflicts with an untracked file in
> the working tree.

I think this one makes sense, as the case that is tested I know
fast-forward (aka two-tree switching) would be atomic. If your
working tree is already broken (e.g. triggers I/O errors in the
middle, some directories having wrong permissions to make them
unwritable by you, etc.), you would also see an error from
fast-forward and you probably cannot tell these two cases apart,
and trying to unstash the local changes may make things even worse,
but I suspect that there isn't much you can do about it.

Thanks.

>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>  builtin/merge.c  |  1 +
>  t/t7600-merge.sh | 11 +++++++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 74797b6c7a6..788a6b0cd55 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1560,6 +1560,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  					  &head_commit->object.oid,
>  					  &commit->object.oid,
>  					  overwrite_ignore)) {
> +			apply_autostash(git_path_merge_autostash(the_repository));
>  			ret = 1;
>  			goto done;
>  		}
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 1cbc9715a81..216113d3537 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -122,6 +122,8 @@ test_expect_success 'setup' '
>  	c0=$(git rev-parse HEAD) &&
>  	cp file.1 file &&
>  	git add file &&
> +	cp file.1 other &&
> +	git add other &&
>  	test_tick &&
>  	git commit -m "commit 1" &&
>  	git tag c1 &&
> @@ -711,6 +713,15 @@ test_expect_success 'fast-forward merge with --autostash' '
>  	test_cmp result.1-5 file
>  '
>  
> +test_expect_success 'failed fast-forward merge with --autostash' '
> +	git reset --hard c0 &&
> +	git merge-file file file.orig file.5 &&
> +	cp file.5 other &&
> +	test_must_fail git merge --autostash c1 2>err &&
> +	test_i18ngrep "Applied autostash." err &&
> +	test_cmp file.5 file
> +'
> +
>  test_expect_success 'octopus merge with --autostash' '
>  	git reset --hard c1 &&
>  	git merge-file file file.orig file.3 &&

@gitgitgadget gitgitgadget Bot Jul 23, 2021

Copy link
Copy Markdown

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, Junio C Hamano wrote (reply to this):

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

> As explained in a03b55530a, and made more abvious by the tests added in
> that commit, the autostash is then applied if the merge succeeds, either
> directly or by committing (after conflict resolution or if '--no-commit'
> was given), or if the merge is aborted with 'git merge --abort'. In some
> other cases, like the user calling 'git reset --merge' or 'git merge
> --quit', the autostash is not applied, but saved in the stash list.

OK.

> However, there exists a scenario that creates an autostash but does not
> apply nor save it to the stash list: if the chosen merge strategy
> completely fails to handle the merge, i.e. 'try_merge_strategy' returns
> 2.
>
> Apply the autostash in that case also. An easy way to test that is to
> try to merge more than two commits but explicitely ask for the 'recursive'
> merge strategy.

We know that the recursive that barfs when asked to merge more than
one "other" histories will fail before touching the index or the
working tree, but I do not think the merge-strategy protocol makes
any guarantee as to what half-modified state the strategy would
leave the index and the working tree in.

So this change at the first glance looks risky, but at least we have
a call to restore_state() before you added an apply_autostash().
The restore_state() call is the same as the one is used between
application of a strategy that has failed the same way in the loop,
so we should be able to trust it to bring the state back to the
pristine (i.e. after autostash removed the local changes).

I think this change makes sense, too.

(I didn't look at 1/4 and 2/4)

Thanks.



> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>  builtin/merge.c  | 1 +
>  t/t7600-merge.sh | 8 ++++++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 788a6b0cd55..d44c14a21a3 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1709,6 +1709,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		else
>  			fprintf(stderr, _("Merge with strategy %s failed.\n"),
>  				use_strategies[0]->name);
> +		apply_autostash(git_path_merge_autostash(the_repository));
>  		ret = 2;
>  		goto done;
>  	} else if (best_strategy == wt_strategy)
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 216113d3537..2ef39d3088e 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -732,6 +732,14 @@ test_expect_success 'octopus merge with --autostash' '
>  	test_cmp result.1-3-5-9 file
>  '
>  
> +test_expect_success 'failed merge (exit 2) with --autostash' '
> +	git reset --hard c1 &&
> +	git merge-file file file.orig file.5 &&
> +	test_must_fail git merge -s recursive --autostash c2 c3 2>err &&
> +	test_i18ngrep "Applied autostash." err &&
> +	test_cmp result.1-5 file
> +'
> +
>  test_expect_success 'conflicted merge with --autostash, --abort restores stash' '
>  	git reset --hard c3 &&
>  	cp file.1 file &&

&commit->object.oid,
overwrite_ignore)) {
apply_autostash(git_path_merge_autostash(the_repository));
ret = 1;
goto done;
}
Expand Down Expand Up @@ -1708,14 +1709,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
else
fprintf(stderr, _("Merge with strategy %s failed.\n"),
use_strategies[0]->name);
apply_autostash(git_path_merge_autostash(the_repository));
ret = 2;
goto done;
} else if (best_strategy == wt_strategy)
; /* We already have its result in the working tree. */
else {

@gitgitgadget gitgitgadget Bot Jul 23, 2021

Copy link
Copy Markdown

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, Felipe Contreras wrote (reply to this):

Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1715,7 +1715,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  	else {
>  		printf(_("Rewinding the tree to pristine...\n"));
>  		restore_state(&head_commit->object.oid, &stash);
> -		printf(_("Using the %s to prepare resolving by hand.\n"),
> +		printf(_("Using the %s strategy to prepare resolving by hand.\n"),

Obviously correct.

-- 
Felipe Contreras

printf(_("Rewinding the tree to pristine...\n"));
restore_state(&head_commit->object.oid, &stash);
printf(_("Using the %s to prepare resolving by hand.\n"),
printf(_("Using the %s strategy to prepare resolving by hand.\n"),
best_strategy);
try_merge_strategy(best_strategy, common, remoteheads,
head_commit);
Expand Down
19 changes: 19 additions & 0 deletions t/t7600-merge.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ test_expect_success 'setup' '
c0=$(git rev-parse HEAD) &&
cp file.1 file &&
git add file &&
cp file.1 other &&
git add other &&
test_tick &&
git commit -m "commit 1" &&
git tag c1 &&
Expand Down Expand Up @@ -711,6 +713,15 @@ test_expect_success 'fast-forward merge with --autostash' '
test_cmp result.1-5 file
'

test_expect_success 'failed fast-forward merge with --autostash' '
git reset --hard c0 &&
git merge-file file file.orig file.5 &&
cp file.5 other &&
test_must_fail git merge --autostash c1 2>err &&
test_i18ngrep "Applied autostash." err &&
test_cmp file.5 file
'

test_expect_success 'octopus merge with --autostash' '
git reset --hard c1 &&
git merge-file file file.orig file.3 &&
Expand All @@ -721,6 +732,14 @@ test_expect_success 'octopus merge with --autostash' '
test_cmp result.1-3-5-9 file
'

test_expect_success 'failed merge (exit 2) with --autostash' '
git reset --hard c1 &&
git merge-file file file.orig file.5 &&
test_must_fail git merge -s recursive --autostash c2 c3 2>err &&
test_i18ngrep "Applied autostash." err &&
test_cmp result.1-5 file
'

test_expect_success 'conflicted merge with --autostash, --abort restores stash' '
git reset --hard c3 &&
cp file.1 file &&
Expand Down