Jens Lehmann [off-list ref] 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.
What do you think
about doing that in a seperate patch?
Doing these in this same patch like you did is better, as it demonstrates
how the callers benefit by the addition of these new bits to the API.
Am 17.01.2010 21:01, schrieb Junio C Hamano:
Jens Lehmann [off-list ref] writes:
quoted
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.
Thanks for your explanation, will provide two independent dirty_submodule
bits there.
quoted
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.
This patch was about calling is_submodule_modified() /only once/, either
in run_diff_files() or in get_stat_data(), and reuse the result. But i
think we don't have to call it /at all/ when for example executing "git
status -s" and the HEAD of the submodule does not match the commit in the
index of the superproject. This information is enough to display an 'M'.
No need to check the submodule work tree for dirtyness, as it won't
change the output of the command anymore.