Re: [PATCH] Show submodules as modified when they contain a dirty work tree

Am 15.01.2010 00:13, schrieb Junio C Hamano:

Jens Lehmann [off-list ref] writes:

quoted

Subject: Show a modified submodule directory as dirty even if the refs match

When the submodules HEAD and the ref committed in the HEAD of the
superproject were the same, "git diff[-index] HEAD" did not show the
submodule as dirty when it should.

Signed-off-by: Jens Lehmann <redacted>
---
 diff-lib.c                |    3 ++-
 t/t4027-diff-submodule.sh |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 5ce226b..9cdf6da 100644
--- a/diff-lib.c
+++ b/diff-lib.c

@@ -233,7 +233,8 @@ static int get_stat_data(struct cache_entry *ce,
 			return -1;
 		}
 		changed = ce_match_stat(ce, &st, 0);
-		if (changed) {
+		if (changed
+		    || (S_ISGITLINK(ce->ce_mode) && is_submodule_modified(ce->name))) {

You had a check in your previous patch that decides to call or skip
diff_change() based on is_submodule_modified() for diff-files, but forgot
to have the same for diff-index, which this patch does.  Perhaps we want
to squash this into 4519d9c (Show submodules as modified when they contain
a dirty work tree, 2010-01-13).

Of course you are right, the change you quoted should have been in my
patch in the first place. So squashing it seems to be the right thing to
do (but AFAICS the tests i added might be a problem, as they use
expect_from_to() which your intermediate patch added. Maybe squash these
tests into your patch and the diff you quoted above into mine?).

The existing code is a bit unfortunate; by the time we come to the output
routine, the information we found from is_submodule_modified() is lost;
that is why my "would look like this" patch calls is_submodule_modified().

We may want to add one parameter to diff_change() and diff_addremove(), to
tell them if the work-tree side (if we are comparing something with the
work tree) is a modified submodule, and add one bit to the diff_filespec
structure to record that in diff_change() and diff_addremove() (obviously
only when adding).  That way, my "would looks like this" patch needs to
check the result of is_submodule_modified() the front-ends left in the
filespec, instead of running it again.

Good idea, i've been already exploring this line of thought too and came
to the same conclusion (i noticed that when calling plain "git diff" in a
repo with submodules, is_submodule_modified() gets called *three* times
for each submodule, which is not /that/ good for performance ;-). But i
intended to do this optimization in a subsequent patch (and in preparation
for "git diff --submodule" being able to print /how/ the submodule is
dirty without having to scan it again).