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

[Jonathan Tan <jonathantanmy@google.com>]

Elijah Newren <newren@gmail.com> writes:
> 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.

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