On Fri, Jul 22 2022, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@xxxxxxxxx>
>
> There are multiple issues at play here:
>
> 1) If `git merge` is invoked with staged changes, it should abort
> without doing any merging, and the user's working tree and index
> should be the same as before merge was invoked.
> 2) Merge strategies are responsible for enforcing the index == HEAD
> requirement. (See 9822175d2b ("Ensure index matches head before
> invoking merge machinery, round N", 2019-08-17) for some history
> around this.)
> 3) Merge strategies can bail saying they are not an appropriate
> handler for the merge in question (possibly allowing other
> strategies to be used instead).
> 4) Merge strategies can make changes to the index and working tree,
> and have no expectation to clean up after themselves, *even* if
> they bail out and say they are not an appropriate handler for
> the merge in question. (The `octopus` merge strategy does this,
> for example.)
> 5) Because of (3) and (4), builtin/merge.c stashes state before
> trying merge strategies and restores it afterward.
>
> Unfortunately, if users had staged changes before calling `git merge`,
> builtin/merge.c could do the following:
>
> * stash the changes, in order to clean up after the strategies
> * try all the merge strategies in turn, each of which report they
> cannot function due to the index not matching HEAD
> * restore the changes via "git stash apply"
>
> But that last step would have the net effect of unstaging the user's
> changes. Fix this by adding the "--index" option to "git stash apply".
> While at it, also squelch the stash apply output; we already report
> "Rewinding the tree to pristine..." and don't need a detailed `git
> status` report afterwards.
>
> Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
> ---
> builtin/merge.c | 5 +++--
> t/t6424-merge-unrelated-index-changes.sh | 7 ++++++-
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 4170c30317e..f807bf335bd 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -383,14 +383,15 @@ static void reset_hard(const struct object_id *oid, int verbose)
> static void restore_state(const struct object_id *head,
> const struct object_id *stash)
> {
> - const char *args[] = { "stash", "apply", NULL, NULL };
> + const char *args[] = { "stash", "apply", "--index", "--quiet",
> + NULL, NULL };
>
> if (is_null_oid(stash))
> return;
>
> reset_hard(head, 1);
>
> - args[2] = oid_to_hex(stash);
> + args[4] = oid_to_hex(stash);
>
> /*
> * It is OK to ignore error here, for example when there was
Just a nit/side comment: This is one of these older-style arg
constructions that we've replaced with strvec in most other places.
Let's leave this alone for now (especially in a v4), but FWIW I wouldn't
mind if these sort of changes were strvec converted while at it:
diff --git a/builtin/merge.c b/builtin/merge.c
index 64def49734a..c3a3a1fde50 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -383,21 +383,23 @@ static void reset_hard(const struct object_id *oid, int verbose)
static void restore_state(const struct object_id *head,
const struct object_id *stash)
{
- const char *args[] = { "stash", "apply", "--index", "--quiet",
- NULL, NULL };
+ struct strvec args = STRVEC_INIT;
+
+ strvec_pushl(&args, "stash", "apply", "--index", "--quiet", NULL);
reset_hard(head, 1);
if (is_null_oid(stash))
goto refresh_cache;
- args[4] = oid_to_hex(stash);
+ strvec_push(&args, oid_to_hex(stash));
/*
* It is OK to ignore error here, for example when there was
* nothing to restore.
*/
- run_command_v_opt(args, RUN_GIT_CMD);
+ run_command_v_opt(args.v, RUN_GIT_CMD);
+ strvec_clear(&args);
refresh_cache:
if (discard_cache() < 0 || read_cache() < 0)
I.e. it takes about as much mental energy to review that as counting the
args elements and seeing that 2 to 4 is correct :)
