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

[Elijah Newren <newren@gmail.com>]

On Wed, May 11, 2022 at 6:01 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > @@ -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.