Re: [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules

[Stefan Beller <sbeller@google.com>]

On Tue, Jan 2, 2018 at 11:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index bf8b602901..ac59524a7f 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,
>>                           ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
>>                               update |= CE_UPDATE;
>>               }
>> +             if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
>> +                 !verify_uptodate(old, o))
>> +                     update |= CE_UPDATE;
>>               add_entry(o, old, update, 0);
>>               return 0;
>>       }
>
> This is more sensible than the previous one that broke same() by
> unconditionally checking the working tree state, even when the
> machinery is told not to look at the working tree.
>
> The code in the pre-context of this hunk, (i.e. the original one you
> are half-way mimicking), we realize that the original cache entry
> and the cache entry we are checking out are exactly the same and we
> overwrite when we know that the path in the working tree is dirty,
> and we are not told to "skip" that path in the working tree
> (i.e. sparse checkout).  That happens only when we are told to
> o->update and o->reset.
>
> Shouldn't we be setting the update bit only when o->update is in
> effect,

Yes, we should.

> just like we can see in the pre-context of this hunk?  I do
> not know if we need to require o->reset and/or need to pay attention
> to the skip worktree bit in this new case.

verify_uptodate takes care of the worktree bits
("o->skip_sparse_checkout && (ce->ce_flags & CE_NEW_SKIP_WORKTREE))")

o->reset is taken care of inside the nested verify_uptodate_1 function via

    ...
    else if (o->reset || ce_uptodate(ce))
        return 0;

So I'd think we only need to toss in the o->update check.

And that additional check would only be a performance optimization
as it would omit the check in case we do not want to update.
(verify_uptodate is expensive for submodules)