Re: [PATCH v3 5/5] builtin/stash: merge index in-core

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

"D. Ben Knoble" <ben.knoble@gmail.com> writes:

> @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
>  		    oideq(&c_tree, &info->i_tree)) {
>  			has_index = 0;
>  		} else {
> -			struct strbuf out = STRBUF_INIT;
> +			struct merge_result result = { 0 };
>  
> -			if (diff_tree_binary(&out, &info->w_commit)) {
> -				strbuf_release(&out);
> -				return error(_("could not generate diff %s^!."),
> -					     oid_to_hex(&info->w_commit));
> -			}
> +			o.branch1 = "Current index";
> +			o.branch2 = "Stashed index changes";
> +			o.ancestor = "Stash base";
>  
> -			ret = apply_cached(&out);
> -			strbuf_release(&out);
> -			if (ret)
> +			o.verbosity = 0;

We realize that 'o' is a struct merge_options defined on the stack
for this function, initialized with init_ui_merge_options() fairly
early on.  It would have initialized '.verbosity' to the default
verbosity, the merge.verbosity configuration variable, or the
GIT_MERGE_VERBOSITY environment variable.

You drop the verbosity here, presumably because you want to match
the previous implementation 'diff-tree | apply --cached' (which I
guess was fairly quiet, but I do not use 'stash pop --index'
myself).

> +			head = lookup_tree(o.repo, &c_tree);
> +			merge = lookup_tree(o.repo, &info->i_tree);
> +			merge_base = lookup_tree(o.repo, &info->b_tree);
> +
> +			merge_incore_nonrecursive(&o, merge_base, head, merge,
> +						  &result);
> +
> +			oidcpy(&index_tree, &result.tree->object.oid);
> +			merge_finalize(&o, &result);

And then the (index) merge is quiet, which is nice.

> +
> +			if (!result.clean)
>  				return error(_("conflicts in index. "
>  					       "Try without --index."));
> -
> -			discard_index(the_repository->index);
> -			repo_read_index(the_repository);
> -			if (write_index_as_tree(&index_tree, the_repository->index,
> -						repo_get_index_file(the_repository), 0, NULL))
> -				return error(_("could not save index tree"));
> -
> -			reset_head();
> -			discard_index(the_repository->index);
> -			repo_read_index(the_repository);
>  		}
>  	}


But the thing is, this is not the end of the function, or the last
call to the merge machinery using 'o'.  We then use the same 'o' to
drive another three-way merge.  Yet nobody restores '.verbosity'
that was unconditionally turned off above for that second merge.

It is a bit surprising that the existing test suite did not catch
this.  Perhaps we do not test --quiet and the merge.verbosity
configuration in combination?

Anyway, I think you'd need something like the following (caveat
emptor: written against checked out 'seen' while reading the patch,
and not even compile tested).

Thanks.


 builtin/stash.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git c/builtin/stash.c w/builtin/stash.c
index 0f10b9c703..a165419d77 100644
--- c/builtin/stash.c
+++ w/builtin/stash.c
@@ -622,6 +622,9 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
 
 	init_ui_merge_options(&o, the_repository);
 
+	if (quiet)
+		o.verbosity = 0;
+
 	if (index) {
 		if (oideq(&info->b_tree, &info->i_tree) ||
 		    oideq(&c_tree, &info->i_tree)) {
@@ -633,8 +636,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
 			o.branch2 = "Stashed index changes";
 			o.ancestor = "Stash base";
 
-			o.verbosity = 0;
-
 			head = lookup_tree(o.repo, &c_tree);
 			merge = lookup_tree(o.repo, &info->i_tree);
 			merge_base = lookup_tree(o.repo, &info->b_tree);
@@ -658,9 +659,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
 	if (oideq(&info->b_tree, &c_tree))
 		o.branch1 = "Version stash was based on";
 
-	if (quiet)
-		o.verbosity = 0;
-
 	if (o.verbosity >= 3)
 		printf_ln(_("Merging %s with %s"), o.branch1, o.branch2);