Re: [PATCH v2 5/6] merge: ensure we can actually restore pre-merge state

[Junio C Hamano <gitster@pobox.com>]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

> -	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++) {