Re: [PATCH v2 5/6] merge: ensure we can actually restore pre-merge state
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 19, 2022, 22:57 UTC
- Message-ID
- <xmqq7d487lsq.fsf@gitster.g>
- In-Reply-To
- <a03075167c1f4410a1b4f415313f11a7e1c3a594.1655621424.git.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 28 quoted lines
> From: Elijah Newren <newren@gmail.com> > > Merge strategies can fail -- not just have conflicts, but give up and > say that they are unable to handle the current type of merge. However, > they can also make changes to the index and working tree before giving > up; merge-octopus does this, for example. Currently, we do not expect > the individual strategies to clean up after themselves, but instead > expect builtin/merge.c to do so. For it to be able to, it needs to save > the state before trying the merge strategy so it can have something to > restore to. Therefore, remove the shortcut bypassing the save_state() > call. > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > builtin/merge.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/builtin/merge.c b/builtin/merge.c > index 2dc56fab70b..aaee8f6a553 100644 > --- a/builtin/merge.c > +++ b/builtin/merge.c > @@ -1663,12 +1663,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > * tree in the index -- this means that the index must be in > * sync with the head commit. The strategies are responsible > * to ensure this. > + * > + * Stash away the local changes so that we can try more than one. > */
The comment explains why we limited the save_state() to avoid wasted cycles and SSD wear and tear by looking at the number of strategies. But because we are removing the restriction (which I am not 100% sure is a good idea), "so that we can try more than one" no longer applies as the reason why we run save_state() here.
Show 9 quoted lines
> - if (use_strategies_nr == 1 ||
> - /*
> - * Stash away the local changes so that we can try more than one.
> - */
> - save_state(&stash))
> + if (save_state(&stash))
> oidclr(&stash);
>
> for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {