Re: [PATCH v3 5/5] builtin/stash: merge index in-core
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 27, 2026, 18:59 UTC
- Message-ID
- <xmqqo6dir04i.fsf@gitster.g>
- In-Reply-To
- <fde7fb7988b695707c6f2776adc18eec7fe4696a.1790425008.git.ben.knoble@gmail.com>
"D. Ben Knoble" <ben.knoble@gmail.com> writes:
Show 20 quoted lines
> @@ -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).
Show 9 quoted lines
> + 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.
Show 16 quoted lines
> +
> + 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);