On Wed, Jan 3, 2018 at 12:49 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Stefan Beller <sbeller@xxxxxxxxxx> writes: > >> Thanks Junio for review of this series! >> The only change in this version of the series is >> >> --- a/unpack-trees.c >> +++ b/unpack-trees.c >> @@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src, >> update |= CE_UPDATE; >> } >> if (S_ISGITLINK(old->ce_mode) && should_update_submodules() && >> - !verify_uptodate(old, o)) >> + o->update && !verify_uptodate(old, o)) >> update |= CE_UPDATE; >> add_entry(o, old, update, 0); >> > > Sounds OK. > > I wonder why o->update is not at the very beginning of the &&-chain, > though. After all, the one above this addition begins with o->reset > && o->update *not* because of the performance concern, but primarily > due to logic flow. I.e. "if we are resetting and updating the > working tree, then..." comes first before saying "we may need to > flip CE_UPDATE bit in update variable if the file in the working > tree is not up to date and it is within a narrow checkout area". It shows that I work too much with submodules. ;) "If we have a submodule and ..." seemed to be the important part when writing the patch. > Of course, because verify_uptodate() is rather expensive, checking > o->update before that makes sense from micro-optimization's point of > view, too. I would think S_ISGITLINK, should_update_submodules as well as o->update are all on the same order of magnitude of costs (some couple number of operations) when compared to verify_uptodate (spawning processes), so as long as verify_uptodate goes last we'd be fine. > > So after thinking aloud like the above, I am reasonably sure that > you want to check o->update as the very first thing in this new if > statement. Thanks for double checking and thinking about the code base with a less submodule centric point of view. Mind to squash it locally or want me to resend? For a resend I'll wait a couple of days to see if there are more comments needing to be addressed. > >> v2: >> I dropped the patch to `same()` as I realized we only need to fix the >> oneway_merge function, the others (two, three way merge) are fine as >> they have the checks already in place. > > This is a bit flawed argument, no? Checking working tree paths > unconditionally in same(), which does not even know if we are > touching the working tree paths, is broken. Unless "they have the > checks already in place" refers to checks that bypasses calls to > same() when we are not touching working tree paths, that is, but > obviously that is not what is going on. > > Will queue. Thanks for working on this. > >
