Re: [PATCH] Add the --submodule-summary option to the diff option family

First: I already coded a test and am using this patch in git gui and
gitk (there was no way git submodule summary would have worked for
gitk, now it does). Looking really good, will post when this patch
solidifies ...

Johannes Schindelin schrieb:

That's something I will gladly do after I adjusted the format to look a 
bit more like "git submodule summary" (Jens noticed 4 differences), and 
after Jens patched git-submodule.sh to use the diff mode instead.

Actually there are two more and Junio pointed out #7: submodule summary
uses --first-parent and we don't (My testcases don't contain merges, so
i didn't notice).

The difference that is most obvious and was unintended is that the
shortlog is indented by four spaces instead of two. I appended an
interdiff to fix that.

As with the other six, i am not really sure if we should just copy the
behaviour of git submodule summary.

IOW in hindsight I would like to add the prefix "RFC/RFH" to the subject.

FullAck.

First some examples for the current behaviour (with the patch below
applied):

$ git diff --submodule-summary
Submodule sub 5431f52..3f35670:
  > sub3
$ git submodule summary --files
* sub 5431f52...3f35670 (1):
  > sub3

$ git diff-index --submodule-summary HEAD -p
Submodule sub 81d7059..3f35670:
  > sub3
  > sub2
$ git submodule summary
* sub 81d7059...3f35670 (2):
  > sub3
  > sub2

[$ git diff --cached --submodule-summary HEAD
Submodule sub 81d7059..5431f52:
  > sub2
$ git submodule summary --cached
* sub 81d7059...5431f52 (1):
  > sub2

$

So the differences are:

1) Dscho replaced the leading '*' with the more explicit "Submodule"
   I like the explicit version.

2) submodule summary prints out how many shortlog entries are following
3) submodule summary adds a newline after the shortlog
4) submodule summary uses --first-parent
   No idea about the intention here. Lars?

5) submodule summary always prints three '.' between the hashes
   Seems like submodule summary didn't do the hassle for performance
   reasons. But for consistency i would prefer the new version as we
   use it all over the place, no?

6) submodule summary can limit the number of shortlog lines
   Hm, i want to see them all. Any users of that -n option?


To take this a bit further: It might be a good idea to let git diff
generate output for submodules that is consistent with that for regular
files. So instead of:

git diff --cached --submodule-summary HEAD -p
Submodule sub 81d7059..5431f52:
    > sub2

we could produce:

diff --git a/sub b/sub
index 81d7059..3f35670 160000
--- a/sub
+++ b/sub

@@ -1 +1 @@
-Subproject commit 81d70590e6aaf9f995ebf4d6d284a7ebb355398d
+Subproject commit 3f356705649b5d566d97ff843cf193359229a453
  > sub2

(This is the output of a git diff without the --submodule-summary option
with an appended shortlog)

I like the second variant (mainly because of consistency). Opinions?



And here the promised indentation depth fix interdiff:
------------------8<-----------------

diff --git a/submodule.c b/submodule.c
index 3f2590d..11fce7d 100644
--- a/submodule.c
+++ b/submodule.c

@@ -42,7 +42,7 @@ void show_submodule_summary(FILE *f, const char *path,
        struct commit_list *merge_bases, *list;
        const char *message = NULL;
        struct strbuf sb = STRBUF_INIT;
-       static const char *format = "    %m %s";
+       static const char *format = "  %m %s";
        int fast_forward = 0, fast_backward = 0;

        if (add_submodule_odb(path))

A bug showed up in the tests, deleted submodules were labelled
"(not checked out)" instead of "(submodule deleted)". This patch
fixes that.

--------------------------8<--------------------
[PATCH] fix output for deleted submodules in git diff --submodule-summary

When a submodule has been deleted, add_submodule_odb() returns false
because the directory of the submodule is gone. So we have to test the
second sha for null before we call add_submodule_odb() to get the correct
output.

Signed-off-by: Jens Lehmann <redacted>
---
 submodule.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/submodule.c b/submodule.c
index 11fce7d..54c8de8 100644
--- a/submodule.c
+++ b/submodule.c

@@ -45,12 +45,12 @@ void show_submodule_summary(FILE *f, const char *path,
 	static const char *format = "  %m %s";
 	int fast_forward = 0, fast_backward = 0;

-	if (add_submodule_odb(path))
+	if (is_null_sha1(two))
+		message = "(submodule deleted)";
+	else if (add_submodule_odb(path))
 		message = "(not checked out)";
 	else if (is_null_sha1(one))
 		message = "(new submodule)";
-	else if (is_null_sha1(two))
-		message = "(submodule deleted)";
 	else if (!(left = lookup_commit_reference(one)) ||
 			!(right = lookup_commit_reference(two)))
 		message = "(commits not present)";

-- 
1.6.5.rc2.210.gac56a4.dirty

Hi,

On Mon, 5 Oct 2009, Jens Lehmann wrote:

A bug showed up in the tests, deleted submodules were labelled
"(not checked out)" instead of "(submodule deleted)". This patch
fixes that.

Good catch, thanks!

Ciao,
Dscho