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

From: Elijah Newren <newren@gmail.com>
Date: May 12, 2022, 14:52 UTC
Message-ID: <CABPp-BFjPtzYcysjkr+q-FYJm3AJTti-YniVYnx6E8aQAkkWAw@mail.gmail.com>
In-Reply-To: <20220512010124.1800540-1-jonathantanmy@google.com>

On Wed, May 11, 2022 at 6:01 PM Jonathan Tan <jonathantanmy@google.com> wrote:

Show 19 quoted lines

>
> "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.