On Tue, Jan 2, 2018 at 11:43 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> 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)
