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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 22, 2022, 10:53 UTC
Message-ID: <220722.86k085s9g3.gmgdl@evledraar.gmail.com>
In-Reply-To: <f401bd5ad0dd7564412e72d19a4193ad3f64e638.1658466942.git.gitgitgadget@gmail.com>

On Fri, Jul 22 2022, Elijah Newren via GitGitGadget wrote:

Show 64 quoted lines

> 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 :)