Re: [PATCH v2 4/4] builtin/stash: merge index in-core
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 24, 2026, 21:59 UTC
- Message-ID
- <xmqqse2yz4y4.fsf@gitster.g>
- In-Reply-To
- <e49936ee12aaf5d82a98dddcc618cee01ac3c681.1790168285.git.ben.knoble@gmail.com>
"D. Ben Knoble" <ben.knoble@gmail.com> writes:
Show 19 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 = "Upstream index";
> + o.branch2 = "Stashed index changes";
> + o.ancestor = "Stash base";
>
> - ret = apply_cached(&out);
> - strbuf_release(&out);
> - if (ret)So, we used to take a diff between w_commit^2^ and w_commit^2 and then give the resulting patch to "apply --cached". w_commit is the working tree state, w_commit^1 is the HEAD (i.e. b_tree) when the stash was created (i.e., "diff HEAD w_commit" is the change in the working tree), w_commit^2 is the contents of the index (i.e. i_tree), so we are computing a patch that represents what "diff --cached HEAD" would have shown when we created the stash. And the goal is to reflect this change on the current HEAD to recreate the "staged" changes in the current index.
IOW, we want to three-way merge the change that moves you from b_tree to i_tree into c_tree.
Show 8 quoted lines
> + 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); > + > + merge_incore_nonrecursive(&o, head, merge, merge_base, > + &result);
We are using the merge machinery to perform a cherry-pick of the changes to go from b_tree to i_tree into c_tree. but the three trees involved in this cherry-pick is named unnecessarily confusingly.
merge-incore-nonrecursive() takes the common ancestor ("merge_base") and two sides ("side1" and "side2") in this order. It takes the changes to go from common to side1 and computes the result of updating the remainder (side2) with such a change (or vice versa; a merge is symmetric). When cherry-picking, the first tree would be the b_tree, the second tree would be the i_tree, and the target tree would be the c_tree.
- b_tree serves as merge_base - i_tree serves as side1 - c_tree serves as side2
Am I following what the code should be doing correctly?
I am wondering if the order of the tree trees in the merge_incore_nonrecursive() call is correct. Shouldn't it be
merge_incore_nonrecursive(&o, merge_base, merge, head, &result);
(or merge and head swapped) if we want to update head (I would call it side2) in such a way that the change to go from it to the resulting tree is similar to the change between merge_base (b_tree) and merge (i_tree)?
Ahh, or perhaps the trees are indeed given in a wrong order, but not in a random wrong order. merge_ort_nonrecursive(), which is *not* the function you are using, takes head, merge, and merge_base in this order, and that order matches what you wrote.
Perhaps the true culprit in this confusion is that the order in which merge_ort_nonrecursive() takes its three trees (head, merge, and common) and the order in which merge_incore_nonrecursive() takes its trees (merge_base, side1, and side2) are different, and if we fix them to match, it would make it easier to work with?
The new test in the attached patch will fail with this step but if we revert the changes to builtin/stash.c in this step, it passes.
t/t3903-stash.sh | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
diff --git c/t/t3903-stash.sh w/t/t3903-stash.sh index 3958ab3c8d..0a87e62b11 100755 --- c/t/t3903-stash.sh +++ w/t/t3903-stash.sh @@ -374,6 +374,38 @@ test_expect_success 'stash apply -q --index refreshes the index' ' test_cmp expect actual ' + +test_expect_success 'stash apply --index does not revert unrelated upstream index changes' ' + test_when_finished "rm -fr playpen" && + mkdir playpen && + ( + cd playpen && + git init && + echo "base1" >file1 && + echo "base2" >file2 && + git add file1 file2 && + git commit -m "initial base" && + + # Make a staged change to file1 and stash it + echo "staged1" >file1 && + git add file1 && + git stash && + + # Upstream advances by modifying unrelated file2 + echo "upstream2" >file2 && + git add file2 && + git commit -m "upstream change to file2" && + + # Apply the stash with --index + git stash apply --index && + + # Verify working tree and index state + test "$(git show :file1)" = "staged1" && + test "$(git show :file2)" = "upstream2" && + test "$(git show HEAD:file2)" = "upstream2" + ) +' + test_expect_success 'stash apply --index leaves everything untouched on failure' ' git reset --hard && echo test >other-file &&