Re: [PATCH] Performance optimization for detection of modified submodules

> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> So here is my first attempt of implementing your proposal. The test suite
>> runs fine, but a few more eyeballs would really be appreciated as i am not
>> very familiar with the code and its corner cases (See diff_change(), is it
>> sufficient to only set "two->dirty_submodule", even if the REVERSE_DIFF
>> option is set? Apart from that i am not so sure about the four changes to
>> tree-diff.c).
> 
> The effect of REVERSE_DIFF bit is contained in the output layer.  The
> order frontends (e.g. diff-files and diff-lib.c::run_diff_files()) feed
> the entries from two hierarchies is not affected.
> 
> The current callers of addremove() may always give the work tree side as
> the second one, but the API is meant to be usable by any other new callers
> and for some of them feeding the work tree side as the first one _might_
> be more sensible (we are talking about futureproofing, so by definition we
> won't know).  It might even be the case where an unanticipated new caller
> might be comparing two trees both living in the work tree (hence you might
> require two independent dirty_submodule bits to the call to show which
> side is dirty, and such a caller may say "both sides are dirty").
> 
> So it would be most future-proof if you add two independent "dirty" bits
> to the API if you are changing it: "is the left side of the comparision a
> dirty submodule?" and "is the right side ...?".  Especially I don't think
> assuming "setting two->dirty is enough for the current implementation" is
> the right way going forward.

>> I think we could skip the call to is_submodule_modified() in
>> run_diff_files() and get_stat_data() when the changed flag is already
>> set and only short output (without calling diff_populate_gitlink(), e.g.
>> "git status -s" or "git diff-files") is requested.
> 
> I am puzzled by your "we could skip"; isn't it what you already have done
> in this patch?  More importantly, I think that is the whole point of the
> change to diff API this patch brings in.