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

From: Jens Lehmann <jens.lehmann@web.de>
Date: Oct 5, 2009, 11:22 UTC
Message-ID: <4AC9D6EB.8090002@web.de>
In-Reply-To: <alpine.DEB.1.00.0910051027010.4985@pacific.mpi-cbg.de>

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))