Re: [PATCH v3 5/6] stash: apply stash using 'merge_ort_nonrecursive()'

Elijah Newren [off-list ref] writes:

On Wed, May 11, 2022 at 6:01 PM Jonathan Tan [off-list ref] wrote:

quoted

"Victoria Dye via GitGitGadget" [off-list ref] writes:

quoted

@@ -551,10 +553,26 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
      if (o.verbosity >= 3)
              printf_ln(_("Merging %s with %s"), o.branch1, o.branch2);

-     bases[0] = &info->b_tree;
+     head = lookup_tree(o.repo, &c_tree);
+     merge = lookup_tree(o.repo, &info->w_tree);
+     merge_base = lookup_tree(o.repo, &info->b_tree);
+
+     repo_hold_locked_index(o.repo, &lock, LOCK_DIE_ON_ERROR);

What's the reason for locking the index? I would think that
merge_ort_nonrecursive() does not modify the index (in any case, I
removed the locking code and the tests still pass). And as for changes
in the worktree, I ran "strace" on the "stash apply" in the test and I
didn't see any changes in the worktree from here until the lock is
released.

merge_incore_nonrecursive() doesn't modify the index or working tree,
but merge_ort_nonrecursive() certainly modifies both in general (via
its call to merge_switch_to_result()).  I'm surprised you didn't see
working tree changes in a strace; was the stash being applied on top
of some commit that had equivalent changes already in its history, so
that the stash application involved a merge where the working tree was
already up-to-date?

More generally, I think it'd be nice if stash locked the index early
in the beginning of its process, did all the modifications, and then
released the lock.  But that's somewhat orthogonal to these changes,
and it'd require getting rid of the forking-subprocesses design of
git-stash.  Since stash is just a transliteration of a shell script,
it locks and unlocks after each individual subcomponent instead.

Thanks for the note. Yeah..I'm not sure what happened.