Re: [PATCH v4 5/7] merge: make restore_state() restore staged state too

[Elijah Newren <newren@gmail.com>]

On Fri, Jul 22, 2022 at 3:55 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Fri, Jul 22 2022, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > 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@gmail.com>
> > ---
> >  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!