On Fri, Jul 22, 2022 at 3:55 AM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
>
> 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 :)
I like this change and included it in my reroll. Thanks!
