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

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

[Johannes Schindelin]

Now you can see the submodule summaries inlined in the diff, instead of
not-quite-helpful SHA-1 pairs.

The format imitates what "git submodule summary" shows.

To do that, <path>/.git/objects/ is added to the alternate object
databases (if that directory exists).

This option was requested by Jens Lehmann at the GitTogether in Berlin.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/diff-options.txt |    4 ++
 Makefile                       |    2 +
 diff.c                         |   14 +++++
 diff.h                         |    1 +
 submodule.c                    |  108 ++++++++++++++++++++++++++++++++++++++++
 submodule.h                    |    8 +++
 6 files changed, 137 insertions(+), 0 deletions(-)
 create mode 100644 submodule.c
 create mode 100644 submodule.h

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 9276fae..5fcc5a8 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -87,6 +87,10 @@ endif::git-format-patch[]
 	Show only names and status of changed files. See the description
 	of the `--diff-filter` option on what the status letters mean.
 
+--submodule-summary::
+	Instead of showing pairs of commit names, list the commits in that
+	commit range in the same style as linkgit:git-submodule[1].
+
 --color::
 	Show colored diff.
 
diff --git a/Makefile b/Makefile
index 2c20922..56d4104 100644
--- a/Makefile
+++ b/Makefile
@@ -449,6 +449,7 @@ LIB_H += sideband.h
 LIB_H += sigchain.h
 LIB_H += strbuf.h
 LIB_H += string-list.h
+LIB_H += submodule.h
 LIB_H += tag.h
 LIB_H += transport.h
 LIB_H += tree.h
@@ -547,6 +548,7 @@ LIB_OBJS += sideband.o
 LIB_OBJS += sigchain.o
 LIB_OBJS += strbuf.o
 LIB_OBJS += string-list.o
+LIB_OBJS += submodule.o
 LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
 LIB_OBJS += trace.o
diff --git a/diff.c b/diff.c
index 9e00131..cdef322 100644
--- a/diff.c
+++ b/diff.c
@@ -13,6 +13,7 @@
 #include "utf8.h"
 #include "userdiff.h"
 #include "sigchain.h"
+#include "submodule.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -1557,6 +1558,17 @@ static void builtin_diff(const char *name_a,
 	const char *a_prefix, *b_prefix;
 	const char *textconv_one = NULL, *textconv_two = NULL;
 
+	if (DIFF_OPT_TST(o, SUMMARIZE_SUBMODULES) &&
+			(!one->mode || S_ISGITLINK(one->mode)) &&
+			(!two->mode || S_ISGITLINK(two->mode))) {
+		const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
+		const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
+		show_submodule_summary(o->file, one ? one->path : two->path,
+				one->sha1, two->sha1,
+				del, add, reset);
+		return;
+	}
+
 	if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
 		textconv_one = get_textconv(one);
 		textconv_two = get_textconv(two);
@@ -2771,6 +2783,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		DIFF_OPT_CLR(options, ALLOW_TEXTCONV);
 	else if (!strcmp(arg, "--ignore-submodules"))
 		DIFF_OPT_SET(options, IGNORE_SUBMODULES);
+	else if (!strcmp(arg, "--submodule-summary"))
+		DIFF_OPT_SET(options, SUMMARIZE_SUBMODULES);
 
 	/* misc options */
 	else if (!strcmp(arg, "-z"))
diff --git a/diff.h b/diff.h
index a7e7ccb..aa6976f 100644
--- a/diff.h
+++ b/diff.h
@@ -67,6 +67,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
 #define DIFF_OPT_DIRSTAT_BY_FILE     (1 << 20)
 #define DIFF_OPT_ALLOW_TEXTCONV      (1 << 21)
 #define DIFF_OPT_DIFF_FROM_CONTENTS  (1 << 22)
+#define DIFF_OPT_SUMMARIZE_SUBMODULES (1 << 23)
 #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
 #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)
 #define DIFF_OPT_CLR(opts, flag)    ((opts)->flags &= ~DIFF_OPT_##flag)
diff --git a/submodule.c b/submodule.c
new file mode 100644
index 0000000..3f2590d
--- /dev/null
+++ b/submodule.c
@@ -0,0 +1,108 @@
+#include "cache.h"
+#include "submodule.h"
+#include "dir.h"
+#include "diff.h"
+#include "commit.h"
+#include "revision.h"
+
+int add_submodule_odb(const char *path)
+{
+	struct strbuf objects_directory = STRBUF_INIT;
+	struct alternate_object_database *alt_odb;
+
+	strbuf_addf(&objects_directory, "%s/.git/objects/", path);
+	if (!is_directory(objects_directory.buf))
+		return -1;
+
+	/* avoid adding it twice */
+	for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
+		if (alt_odb->name - alt_odb->base == objects_directory.len &&
+				!strncmp(alt_odb->base, objects_directory.buf,
+					objects_directory.len))
+			return 0;
+
+	alt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));
+	alt_odb->next = alt_odb_list;
+	strcpy(alt_odb->base, objects_directory.buf);
+	alt_odb->name = alt_odb->base + objects_directory.len;
+	alt_odb->name[2] = '/';
+	alt_odb->name[40] = '\0';
+	alt_odb->name[41] = '\0';
+	alt_odb_list = alt_odb;
+	prepare_alt_odb();
+	return 0;
+}
+
+void show_submodule_summary(FILE *f, const char *path,
+		unsigned char one[20], unsigned char two[20],
+		const char *del, const char *add, const char *reset)
+{
+	struct rev_info rev;
+	struct commit *commit, *left, *right;
+	struct commit_list *merge_bases, *list;
+	const char *message = NULL;
+	struct strbuf sb = STRBUF_INIT;
+	static const char *format = "    %m %s";
+	int fast_forward = 0, fast_backward = 0;
+
+	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)";
+
+	if (!message) {
+		init_revisions(&rev, NULL);
+		setup_revisions(0, NULL, &rev, NULL);
+		rev.left_right = 1;
+		left->object.flags |= SYMMETRIC_LEFT;
+		add_pending_object(&rev, &left->object, path);
+		add_pending_object(&rev, &right->object, path);
+		merge_bases = get_merge_bases(left, right, 1);
+		if (merge_bases) {
+			if (merge_bases->item == left)
+				fast_forward = 1;
+			else if (merge_bases->item == right)
+				fast_backward = 1;
+		}
+		for (list = merge_bases; list; list = list->next) {
+			list->item->object.flags |= UNINTERESTING;
+			add_pending_object(&rev, &list->item->object,
+				sha1_to_hex(list->item->object.sha1));
+		}
+		if (prepare_revision_walk(&rev))
+			message = "(revision walker failed)";
+	}
+
+	strbuf_addf(&sb, "Submodule %s %s..", path,
+			find_unique_abbrev(one, DEFAULT_ABBREV));
+	if (!fast_backward && !fast_forward)
+		strbuf_addch(&sb, '.');
+	strbuf_addf(&sb, "%s", find_unique_abbrev(two, DEFAULT_ABBREV));
+	if (message)
+		strbuf_addf(&sb, " %s\n", message);
+	else
+		strbuf_addf(&sb, "%s:\n", fast_backward ? " (rewind)" : "");
+	fwrite(sb.buf, sb.len, 1, f);
+
+	if (!message) {
+		while ((commit = get_revision(&rev))) {
+			strbuf_setlen(&sb, 0);
+			if (del)
+				strbuf_addstr(&sb, commit->object.flags &
+						SYMMETRIC_LEFT ? del : add);
+			format_commit_message(commit, format, &sb,
+					rev.date_mode);
+			if (del)
+				strbuf_addstr(&sb, reset);
+			strbuf_addch(&sb, '\n');
+			fwrite(sb.buf, sb.len, 1, f);
+		}
+		clear_commit_marks(left, ~0);
+		clear_commit_marks(right, ~0);
+	}
+}
diff --git a/submodule.h b/submodule.h
new file mode 100644
index 0000000..4c0269d
--- /dev/null
+++ b/submodule.h
@@ -0,0 +1,8 @@
+#ifndef SUBMODULE_H
+#define SUBMODULE_H
+
+void show_submodule_summary(FILE *f, const char *path,
+		unsigned char one[20], unsigned char two[20],
+		const char *del, const char *add, const char *reset);
+
+#endif
-- 
1.6.4.313.g3d9e3

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

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Now you can see the submodule summaries inlined in the diff, instead of
> not-quite-helpful SHA-1 pairs.

This looks useful, but I do not think this is --summary.  It is not part
of the "summary" output but is about making output for Submoodules more
verbose.  I'd suggest naming it --verbose-submodule or something.

> The format imitates what "git submodule summary" shows.

The output format needs to be described better here and also in
Documentation/diff-format.txt.

> To do that, <path>/.git/objects/ is added to the alternate object
> databases (if that directory exists).

Is it always true that <path>/.git is the GIT_DIR for that submodule?  Not
complaining but checking sanity.

> diff --git a/diff.c b/diff.c
> index 9e00131..cdef322 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1557,6 +1558,17 @@ static void builtin_diff(const char *name_a,
>  	const char *a_prefix, *b_prefix;
>  	const char *textconv_one = NULL, *textconv_two = NULL;
>  
> +	if (DIFF_OPT_TST(o, SUMMARIZE_SUBMODULES) &&
> +			(!one->mode || S_ISGITLINK(one->mode)) &&
> +			(!two->mode || S_ISGITLINK(two->mode))) {
> +		const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
> +		const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
> +		show_submodule_summary(o->file, one ? one->path : two->path,
> +				one->sha1, two->sha1,
> +				del, add, reset);
> +		return;
> +	}
> +

Isn't this "textual diff" codepath?

I would have expected --submodule-summary output to be near the --summary
output and to be generated in the same codepath to generate it; if this
were named --verbose-submodule, I would certainly understand it, and I
think the placement is saner in the textual diff.

> diff --git a/submodule.c b/submodule.c
> new file mode 100644
> index 0000000..3f2590d
> --- /dev/null
> +++ b/submodule.c
> @@ -0,0 +1,108 @@
> +#include "cache.h"
> +#include "submodule.h"
> +#include "dir.h"
> +#include "diff.h"
> +#include "commit.h"
> +#include "revision.h"
> +
> +int add_submodule_odb(const char *path)
> +{
> +	struct strbuf objects_directory = STRBUF_INIT;
> +	struct alternate_object_database *alt_odb;
> +
> +	strbuf_addf(&objects_directory, "%s/.git/objects/", path);
> +	if (!is_directory(objects_directory.buf))
> +		return -1;
> +
> +	/* avoid adding it twice */
> +	for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
> +		if (alt_odb->name - alt_odb->base == objects_directory.len &&
> +				!strncmp(alt_odb->base, objects_directory.buf,
> +					objects_directory.len))
> +			return 0;
> +	alt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));
> +	alt_odb->next = alt_odb_list;
> +	strcpy(alt_odb->base, objects_directory.buf);
> +	alt_odb->name = alt_odb->base + objects_directory.len;
> +	alt_odb->name[2] = '/';
> +	alt_odb->name[40] = '\0';
> +	alt_odb->name[41] = '\0';
> +	alt_odb_list = alt_odb;
> +	prepare_alt_odb();

The lines after "avoid adding it twice" look somewhat familiar.

Don't we already have other codepaths to add an alternate odb that need to
be careful in the same way (i.e. do not add twice, do not loop, etc.), and
if so don't you want to reuse it after refactoring?

> +void show_submodule_summary(FILE *f, const char *path,
> +		unsigned char one[20], unsigned char two[20],
> +		const char *del, const char *add, const char *reset)
> +{
> +	struct rev_info rev;
> +	struct commit *commit, *left, *right;
> +	struct commit_list *merge_bases, *list;
> +	const char *message = NULL;
> +	struct strbuf sb = STRBUF_INIT;
> +	static const char *format = "    %m %s";
> +	int fast_forward = 0, fast_backward = 0;
> +
> +	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)";

Are you sure about this?  Wouldn't "git diff HEAD" (not looking at the
index) give you the 0{40} on the "two" side when the path is modified?

> +	if (!message) {
> +		init_revisions(&rev, NULL);
> +		setup_revisions(0, NULL, &rev, NULL);
> +		rev.left_right = 1;
> +		left->object.flags |= SYMMETRIC_LEFT;
> +		add_pending_object(&rev, &left->object, path);
> +		add_pending_object(&rev, &right->object, path);
> +		merge_bases = get_merge_bases(left, right, 1);
> +		if (merge_bases) {
> +			if (merge_bases->item == left)
> +				fast_forward = 1;
> +			else if (merge_bases->item == right)
> +				fast_backward = 1;
> +		}
> +		for (list = merge_bases; list; list = list->next) {
> +			list->item->object.flags |= UNINTERESTING;
> +			add_pending_object(&rev, &list->item->object,
> +				sha1_to_hex(list->item->object.sha1));
> +		}
> +		if (prepare_revision_walk(&rev))
> +			message = "(revision walker failed)";

If prepare_revision_walk() failed for whatever reason, can we trust
fast_forward/fast_backward at this point?

> +	}
> +
> +	strbuf_addf(&sb, "Submodule %s %s..", path,
> +			find_unique_abbrev(one, DEFAULT_ABBREV));
> +	if (!fast_backward && !fast_forward)
> +		strbuf_addch(&sb, '.');
> +	strbuf_addf(&sb, "%s", find_unique_abbrev(two, DEFAULT_ABBREV));
> +	if (message)
> +		strbuf_addf(&sb, " %s\n", message);
> +	else
> +		strbuf_addf(&sb, "%s:\n", fast_backward ? " (rewind)" : "");

No corresponding "(fast-forward)" label?

> +	fwrite(sb.buf, sb.len, 1, f);
> +
> +	if (!message) {
> +		while ((commit = get_revision(&rev))) {
> +			strbuf_setlen(&sb, 0);
> +			if (del)
> +				strbuf_addstr(&sb, commit->object.flags &
> +						SYMMETRIC_LEFT ? del : add);
> +			format_commit_message(commit, format, &sb,
> +					rev.date_mode);
> +			if (del)
> +				strbuf_addstr(&sb, reset);

Three points, two of which are minor.

 - Checking "del" to decide if you want to say "reset" feels funny.

 - In the "ANSI-terminal only" world view, adding colors to strbuf and
   writing it out together with the actual strings is an easy thing to do.
   Don't Windows folks have trouble converting this kind of code to their
   color control call that is separate from writing strings out?  If it is
   not a problem, I do not have any objection to it, but otherwise I'd
   suggest not to add any more code that stores color escape sequence in
   strbuf, so that we would not make later conversion by Windows folks
   harder than necessary.

 - The output is similar but different from "submodule --summary" and
   there is no justification described in the patch nor the proposed
   commit log message.

   o Why does "submodule --summary" use --first-parent and this patch
     doesn't?

   o "submodule --summary" allows users to limit the walk but this seems
     to give the full list---is it useful (or even sane) to always give
     a full list?

> +			strbuf_addch(&sb, '\n');
> +			fwrite(sb.buf, sb.len, 1, f);
> +		}
> +		clear_commit_marks(left, ~0);
> +		clear_commit_marks(right, ~0);
> +	}

Aren't object flags shared between the outer revision walker that walks
the log and this new walker?  If the supermodule history and submodule
history share commits (e.g. when later git.git starts using submodules to
bind gitk and git-gui), wouldn't this break "git log -p" a big way?

In any case, thanks for an interesting patch.  Looking forward to see how
this topic evolves.

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

[Johannes Sixt]

Junio C Hamano schrieb:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> +	fwrite(sb.buf, sb.len, 1, f);
>> +
>> +	if (!message) {
>> +		while ((commit = get_revision(&rev))) {
>> +			strbuf_setlen(&sb, 0);
>> +			if (del)
>> +				strbuf_addstr(&sb, commit->object.flags &
>> +						SYMMETRIC_LEFT ? del : add);
>> +			format_commit_message(commit, format, &sb,
>> +					rev.date_mode);
>> +			if (del)
>> +				strbuf_addstr(&sb, reset);
> 
>  - In the "ANSI-terminal only" world view, adding colors to strbuf and
>    writing it out together with the actual strings is an easy thing to do.
>    Don't Windows folks have trouble converting this kind of code to their
>    color control call that is separate from writing strings out?  If it is
>    not a problem, I do not have any objection to it, but otherwise I'd
>    suggest not to add any more code that stores color escape sequence in
>    strbuf, so that we would not make later conversion by Windows folks
>    harder than necessary.

Thanks for noticing this! To store color escapes in strbuf is not a
problem as long as the string is finally written using printf, fprintf, or
fputs.

>> +			strbuf_addch(&sb, '\n');
>> +			fwrite(sb.buf, sb.len, 1, f);

Outch! fwrite doesn't interpret color escapes. AFAICS, this sequence is
easy to change such that it uses fprintf().

-- Hannes

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

[Johannes Schindelin]

Hi,

On Mon, 5 Oct 2009, Johannes Sixt wrote:

> Junio C Hamano schrieb:
> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >> +	fwrite(sb.buf, sb.len, 1, f);
> >> +
> >> +	if (!message) {
> >> +		while ((commit = get_revision(&rev))) {
> >> +			strbuf_setlen(&sb, 0);
> >> +			if (del)
> >> +				strbuf_addstr(&sb, commit->object.flags &
> >> +						SYMMETRIC_LEFT ? del : add);
> >> +			format_commit_message(commit, format, &sb,
> >> +					rev.date_mode);
> >> +			if (del)
> >> +				strbuf_addstr(&sb, reset);
> > 
> >  - In the "ANSI-terminal only" world view, adding colors to strbuf and
> >    writing it out together with the actual strings is an easy thing to do.
> >    Don't Windows folks have trouble converting this kind of code to their
> >    color control call that is separate from writing strings out?  If it is
> >    not a problem, I do not have any objection to it, but otherwise I'd
> >    suggest not to add any more code that stores color escape sequence in
> >    strbuf, so that we would not make later conversion by Windows folks
> >    harder than necessary.
> 
> Thanks for noticing this! To store color escapes in strbuf is not a
> problem as long as the string is finally written using printf, fprintf, or
> fputs.
> 
> >> +			strbuf_addch(&sb, '\n');
> >> +			fwrite(sb.buf, sb.len, 1, f);
> 
> Outch! fwrite doesn't interpret color escapes. AFAICS, this sequence is
> easy to change such that it uses fprintf().

Good point.  I changed it to

                        fprintf(f, "%s", sb.buf);

BTW we probably need to remove the "TODO: write" from compat/winansi.c...

Ciao,
Dscho

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

[Johannes Sixt]

Johannes Schindelin schrieb:
> On Mon, 5 Oct 2009, Johannes Sixt wrote:
>> Junio C Hamano schrieb:
>>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>>> +	fwrite(sb.buf, sb.len, 1, f);
>>>> +
>>>> +	if (!message) {
>>>> +		while ((commit = get_revision(&rev))) {
>>>> +			strbuf_setlen(&sb, 0);
>>>> +			if (del)
>>>> +				strbuf_addstr(&sb, commit->object.flags &
>>>> +						SYMMETRIC_LEFT ? del : add);
>>>> +			format_commit_message(commit, format, &sb,
>>>> +					rev.date_mode);
>>>> +			if (del)
>>>> +				strbuf_addstr(&sb, reset);
>>>> +			strbuf_addch(&sb, '\n');
>>>> +			fwrite(sb.buf, sb.len, 1, f);
>> Outch! fwrite doesn't interpret color escapes. AFAICS, this sequence is
>> easy to change such that it uses fprintf().
> 
> Good point.  I changed it to
> 
>                         fprintf(f, "%s", sb.buf);

Thanks. But notice how you are constructing the string in sb from pieces.
I meant to change it to

	fprintf(f, "%s%s%s\n",
			del ? (commit->object.flags & SYMMETRIC_LEFT
					 ? del : add) : "",
			format_commit_message(commit, format, &sb,
					rev.date_mode),
			del ? reset : "");

or similar. We already use this idiom elsewhere.

-- Hannes

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

[Johannes Schindelin]

Hi,

On Mon, 5 Oct 2009, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> > On Mon, 5 Oct 2009, Johannes Sixt wrote:
> >> Junio C Hamano schrieb:
> >>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >>>> +	fwrite(sb.buf, sb.len, 1, f);
> >>>> +
> >>>> +	if (!message) {
> >>>> +		while ((commit = get_revision(&rev))) {
> >>>> +			strbuf_setlen(&sb, 0);
> >>>> +			if (del)
> >>>> +				strbuf_addstr(&sb, commit->object.flags &
> >>>> +						SYMMETRIC_LEFT ? del : add);
> >>>> +			format_commit_message(commit, format, &sb,
> >>>> +					rev.date_mode);
> >>>> +			if (del)
> >>>> +				strbuf_addstr(&sb, reset);
> >>>> +			strbuf_addch(&sb, '\n');
> >>>> +			fwrite(sb.buf, sb.len, 1, f);
> >> Outch! fwrite doesn't interpret color escapes. AFAICS, this sequence is
> >> easy to change such that it uses fprintf().
> > 
> > Good point.  I changed it to
> > 
> >                         fprintf(f, "%s", sb.buf);
> 
> Thanks. But notice how you are constructing the string in sb from pieces.
> I meant to change it to
> 
> 	fprintf(f, "%s%s%s\n",
> 			del ? (commit->object.flags & SYMMETRIC_LEFT
> 					 ? del : add) : "",
> 			format_commit_message(commit, format, &sb,
> 					rev.date_mode),
> 			del ? reset : "");
> 
> or similar. We already use this idiom elsewhere.

And I find it utterly ugly and unreadable there, too. So this is why I did 
not do it.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi,

On Mon, 5 Oct 2009, Johannes Schindelin wrote:

> On Sun, 4 Oct 2009, Junio C Hamano wrote:
> 
> >  - Checking "del" to decide if you want to say "reset" feels funny.
> 
> Right.  But I wanted to avoid checking for del, add and reset 
> separately.
> 
> So I was wrong, and will fix.

This is the current interdiff:

-- snipsnap --
diff --git a/submodule.c b/submodule.c
index 3f2590d..b441ea1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -92,15 +92,18 @@ void show_submodule_summary(FILE *f, const char *path,
 	if (!message) {
 		while ((commit = get_revision(&rev))) {
 			strbuf_setlen(&sb, 0);
-			if (del)
-				strbuf_addstr(&sb, commit->object.flags &
-						SYMMETRIC_LEFT ? del : add);
+			if (commit->object.flags & SYMMETRIC_LEFT) {
+				if (del)
+					strbuf_addstr(&sb, del);
+			}
+			else if (add)
+				strbuf_addstr(&sb, add);
 			format_commit_message(commit, format, &sb,
 					rev.date_mode);
-			if (del)
+			if (reset)
 				strbuf_addstr(&sb, reset);
 			strbuf_addch(&sb, '\n');
-			fwrite(sb.buf, sb.len, 1, f);
+			fprintf(f, "%s", sb.buf);
 		}
 		clear_commit_marks(left, ~0);
 		clear_commit_marks(right, ~0);

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

[Jens Lehmann]

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

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

[Jens Lehmann]

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 <Jens.Lehmann@web.de>
---
 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

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > +		if (prepare_revision_walk(&rev))
>> > +			message = "(revision walker failed)";
>> 
>> If prepare_revision_walk() failed for whatever reason, can we trust
>> fast_forward/fast_backward at this point?
>
> No, but it is not used in that case, either, because message is not NULL 
> anymore.

It is used in that case a few lines below to decide if you add the third
dot.  That's why I asked.

>> > +	}
>> > +
>> > +	strbuf_addf(&sb, "Submodule %s %s..", path,
>> > +			find_unique_abbrev(one, DEFAULT_ABBREV));
>> > +	if (!fast_backward && !fast_forward)
>> > +		strbuf_addch(&sb, '.');

> Our output methods translate ANSI, so the strbufs only hold the ANSI 
> sequences.

I'll always trust two Johannes's on Windows matters ;-)

> I have no idea why "submodule --summary" uses --first-parent, but 
> personally, I would _hate_ it not to see the merged commits in the diff.
>
> For a summary, you might get away with seeing
>
> 	> Merge bla
> 	> Merge blub
> 	> Merge this
> 	> Merge that
>
> but in a diff that does not cut it at all.

As long as bla/blub/this/that are descriptive enough, I do not see at all
why you think "summary" is Ok and "diff" is not.  If your response were
"it is just a matter of taste; to some people (or project) --first-parent
is useful and for others it is not", I would understand it, and it would
make sense to use (or not use) --first-parent consistently between this
codepath and "submodule --summary", though.

> In any case, just to safe-guard against sick minds, I can add a check that 
> says that left, right, and all the merge bases _cannot_ have any flags 
> set, otherwise we output "(you should visit a psychiatrist)" or some such.

I wouldn't suggest adding such a kludge.  Being insulting to the user when
we hit a corner case _we_ cannot handle does not help anybody, does it?

I see two saner options.  Doing this list walking in a subprocess so that
you wouldn't have to worry about object flags at all in this case would
certainly be easier; the other option obviously is to have a separate
object pool ala libgit2, but that would be a much larger change.

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

[Johannes Schindelin]

Hi,

On Mon, 5 Oct 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > +		if (prepare_revision_walk(&rev))
> >> > +			message = "(revision walker failed)";
> >> 
> >> If prepare_revision_walk() failed for whatever reason, can we trust
> >> fast_forward/fast_backward at this point?
> >
> > No, but it is not used in that case, either, because message is not NULL 
> > anymore.
> 
> It is used in that case a few lines below to decide if you add the third
> dot.  That's why I asked.

Well, fair enough.

The answer is: yes, we can still trust fast_forward/fast_backward, as 
there is no question that if the first merge base (which must be the only 
merge base by definition, in this case) is either "left" or "right", it is 
fast_forward or fast_backward, respectively.

So: no worries.

> > I have no idea why "submodule --summary" uses --first-parent, but 
> > personally, I would _hate_ it not to see the merged commits in the 
> > diff.
> >
> > For a summary, you might get away with seeing
> >
> > 	> Merge bla
> > 	> Merge blub
> > 	> Merge this
> > 	> Merge that
> >
> > but in a diff that does not cut it at all.
> 
> As long as bla/blub/this/that are descriptive enough, I do not see at all
> why you think "summary" is Ok and "diff" is not.  If your response were
> "it is just a matter of taste; to some people (or project) --first-parent
> is useful and for others it is not", I would understand it, and it would
> make sense to use (or not use) --first-parent consistently between this
> codepath and "submodule --summary", though.

You may be used to git.git's quality of naming the branches you merge.

Sadly, this is not the common case.

> > In any case, just to safe-guard against sick minds, I can add a check that 
> > says that left, right, and all the merge bases _cannot_ have any flags 
> > set, otherwise we output "(you should visit a psychiatrist)" or some such.
> 
> I wouldn't suggest adding such a kludge.  Being insulting to the user when
> we hit a corner case _we_ cannot handle does not help anybody, does it?

Well, I was a little exasperated when I wrote that that you want to handle 
that case.

But of course, I should heed Postel's law, and handle the case. Maybe say 
something like "(uses superproject's commits)".

> I see two saner options.  Doing this list walking in a subprocess so that
> you wouldn't have to worry about object flags at all in this case would
> certainly be easier; the other option obviously is to have a separate
> object pool ala libgit2, but that would be a much larger change.

The reason why I insist avoiding a subprocess is performance.  The same 
reason holds for a separate object pool: it would just impede the speed, 
AFAICT.

Besides, I vividly remember what happened to a patch I posted to be able 
to just clear the current object pool.  And I cannot imagine a patch 
introducing a second pool to be any less complicated.

If you really want the case I illustrated (that the submodule actually 
contains commits that already have been shown in the superproject) to be 
handled showing the correct submodule summary (and with --first-parent, I 
think you will agree that it is a summary, even if it is embedded in a 
diff), I could imagine calling a subprocess (for simplicity reasons) _iff_ 
left, right, or any of the merge bases has a flag set.

But I really, really, really want to avoid a fork() in the common case.  I 
do have some users on Windows, and I do have a few submodules in that 
project.  Having too many fork() calls there would just give Git a bad 
reputation.  And it has enough of that, it does not need more.

Ciao,
Dscho

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

[Jens Lehmann]

Johannes Schindelin schrieb:
>>> I have no idea why "submodule --summary" uses --first-parent, but 
>>> personally, I would _hate_ it not to see the merged commits in the 
>>> diff.
>>>
>>> For a summary, you might get away with seeing
>>>
>>> 	> Merge bla
>>> 	> Merge blub
>>> 	> Merge this
>>> 	> Merge that
>>>
>>> but in a diff that does not cut it at all.
>> As long as bla/blub/this/that are descriptive enough, I do not see at all
>> why you think "summary" is Ok and "diff" is not.  If your response were
>> "it is just a matter of taste; to some people (or project) --first-parent
>> is useful and for others it is not", I would understand it, and it would
>> make sense to use (or not use) --first-parent consistently between this
>> codepath and "submodule --summary", though.
> 
> You may be used to git.git's quality of naming the branches you merge.
> 
> Sadly, this is not the common case.

IMHO both arguments are valid, using --first-parent really is a matter of
taste *and* it is dependent on the quality of branch naming whether it is
useful or not.

But when both commands shall produce the same output, i think we have to
use --first-parent as default, no? And maybe we could add another option
to diff which can change that behaviour according to users taste?


> But I really, really, really want to avoid a fork() in the common case.  I 
> do have some users on Windows, and I do have a few submodules in that 
> project.  Having too many fork() calls there would just give Git a bad 
> reputation.  And it has enough of that, it does not need more.

Me too thinks performance matters here. We do have a repo at my dayjob
with more than a handful of submodules and its main target platform is
windows ... so having that perform nicely is a win for us.


Jens

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

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

>> But I really, really, really want to avoid a fork() in the common case.  I 
>> do have some users on Windows, and I do have a few submodules in that 
>> project.  Having too many fork() calls there would just give Git a bad 
>> reputation.  And it has enough of that, it does not need more.
>
> Me too thinks performance matters here. We do have a repo at my dayjob
> with more than a handful of submodules and its main target platform is
> windows ... so having that perform nicely is a win for us.

Numbers?

I'd prefer to avoid kludges that favors unsubstantiated performance
argument over correctness.

Thanks.

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

[Johannes Schindelin]

Hi,

On Tue, 6 Oct 2009, Junio C Hamano wrote:

> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
> >> But I really, really, really want to avoid a fork() in the common case.  I 
> >> do have some users on Windows, and I do have a few submodules in that 
> >> project.  Having too many fork() calls there would just give Git a bad 
> >> reputation.  And it has enough of that, it does not need more.
> >
> > Me too thinks performance matters here. We do have a repo at my dayjob
> > with more than a handful of submodules and its main target platform is
> > windows ... so having that perform nicely is a win for us.
> 
> Numbers?
> 
> I'd prefer to avoid kludges that favors unsubstantiated performance
> argument over correctness.

Well, having worked with msysGit for such a long time, I just _know_ that 
a subprocess costs a substantial amount of time.

But as you don't trust my words, maybe Jens could be so kind as to perform 
some benchmarks?  I am short on Git time budget, but I will make a commit 
on my submodule-summary branch that allows to start a subprocess always.

Ciao,
Dscho

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

[Jens Lehmann]

Johannes Schindelin schrieb:
> Hi,
> 
> On Tue, 6 Oct 2009, Junio C Hamano wrote:
> 
>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>>
>>>> But I really, really, really want to avoid a fork() in the common case.  I 
>>>> do have some users on Windows, and I do have a few submodules in that 
>>>> project.  Having too many fork() calls there would just give Git a bad 
>>>> reputation.  And it has enough of that, it does not need more.
>>> Me too thinks performance matters here. We do have a repo at my dayjob
>>> with more than a handful of submodules and its main target platform is
>>> windows ... so having that perform nicely is a win for us.
>> Numbers?
>>
>> I'd prefer to avoid kludges that favors unsubstantiated performance
>> argument over correctness.
> 
> Well, having worked with msysGit for such a long time, I just _know_ that 
> a subprocess costs a substantial amount of time.
> 
> But as you don't trust my words, maybe Jens could be so kind as to perform 
> some benchmarks?  I am short on Git time budget, but I will make a commit 
> on my submodule-summary branch that allows to start a subprocess always.

Sure, will do.

Jens

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

[Johannes Schindelin]

Hi,

On Tue, 6 Oct 2009, Jens Lehmann wrote:

> Johannes Schindelin schrieb:
> > Hi,
> > 
> > On Tue, 6 Oct 2009, Junio C Hamano wrote:
> > 
> >> Jens Lehmann <Jens.Lehmann@web.de> writes:
> >>
> >>>> But I really, really, really want to avoid a fork() in the common case.  I 
> >>>> do have some users on Windows, and I do have a few submodules in that 
> >>>> project.  Having too many fork() calls there would just give Git a bad 
> >>>> reputation.  And it has enough of that, it does not need more.
> >>> Me too thinks performance matters here. We do have a repo at my dayjob
> >>> with more than a handful of submodules and its main target platform is
> >>> windows ... so having that perform nicely is a win for us.
> >> Numbers?
> >>
> >> I'd prefer to avoid kludges that favors unsubstantiated performance
> >> argument over correctness.
> > 
> > Well, having worked with msysGit for such a long time, I just _know_ that 
> > a subprocess costs a substantial amount of time.
> > 
> > But as you don't trust my words, maybe Jens could be so kind as to perform 
> > some benchmarks?  I am short on Git time budget, but I will make a commit 
> > on my submodule-summary branch that allows to start a subprocess always.
> 
> Sure, will do.

Okay, it is there.  It is quick and dirty, so you don't even want to look 
at the commit message.

Could you please run something like "time git diff --submodule-summary 
--all" with and without this patch?

Thanks,
Dscho

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

[Jens Lehmann]

Junio C Hamano schrieb:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>>> But I really, really, really want to avoid a fork() in the common case.  I 
>>> do have some users on Windows, and I do have a few submodules in that 
>>> project.  Having too many fork() calls there would just give Git a bad 
>>> reputation.  And it has enough of that, it does not need more.
>> Me too thinks performance matters here. We do have a repo at my dayjob
>> with more than a handful of submodules and its main target platform is
>> windows ... so having that perform nicely is a win for us.
> 
> Numbers?

Here they are:

First i did them with the repo at hand, current msysgit master with
Dscho's git-repo checked out:

  without fork : real	0m0.672s
  with fork    : real	0m0.781s

So here it's a about 16% slower when using fork() (and both are
generating about 7270 shortlog entries).

But i thought this to be a rather unusual situation, having only one
submodule being changed by 7270 commits ...

So i took a live repo from my dayjob containing 8 submodules. In each
submodule i did a "git checkout HEAD^" to simulate one change. And
then i got:

  without fork : real	0m0.203s
  with fork    : real	0m0.453s

This is a degradation of more than 120% because of the fork()s. And
just for fun i ran the scripted submodule summary too:

  scripted     : real	0m3.437s

So the forked version outperforms the scripted version by a factor of
7, while the speedup from Dscho's original proposal is almost 17fold.
(If i did my computations right, the extra costs for each changed
submodule are a bit more than 30ms when fork()ing. Dscho's version
doesn't seem to suffer from changed submodules at all, i measured
0.203s for both versions before i did the submodule init and update).

(Best of three, "time git diff --submodule-summary". My system is an
Athlon64x2 4600+ with WindowsXP)


Jens

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

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> So i took a live repo from my dayjob containing 8 submodules. In each
> submodule i did a "git checkout HEAD^" to simulate one change.

It appears that your "simulation" is to have changes in all submodules and
nowhere else in the tree.  I have to wonder how common would that be.  If
I have been working in the subprojects without changing the superproject
at all, I would likely to be in one of the subproject directories and my
"git diff" would not even have to trigger this codepath.  If we have on
the other hand changes in the superproject, showing them would cost us
equally in both approaches.

> then i got:
>
>   without fork : real	0m0.203s
>   with fork    : real	0m0.453s
>
> This is a degradation of more than 120% because of the fork()s. And
> just for fun i ran the scripted submodule summary too:
>
>   scripted     : real	0m3.437s
>
> So the forked version outperforms the scripted version by a factor of
> 7, while the speedup from Dscho's original proposal is almost 17fold.

Thanks.  I was worried if the forking to ensure correctness may sactifice
performance so much to be unusable, but it does not seem to be the case,
given the above comparision.

I didn't look at the code but presumably it uses the run_command()
interface and cleanly written?

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

[Johannes Schindelin]

Hi,

On Wed, 7 Oct 2009, Junio C Hamano wrote:

> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
> > So i took a live repo from my dayjob containing 8 submodules. In each
> > submodule i did a "git checkout HEAD^" to simulate one change.
> 
> It appears that your "simulation" is to have changes in all submodules 
> and nowhere else in the tree.  I have to wonder how common would that 
> be.  If I have been working in the subprojects without changing the 
> superproject at all, I would likely to be in one of the subproject 
> directories and my "git diff" would not even have to trigger this 
> codepath.  If we have on the other hand changes in the superproject, 
> showing them would cost us equally in both approaches.
> 
> > then i got:
> >
> >   without fork : real	0m0.203s
> >   with fork    : real	0m0.453s
> >
> > This is a degradation of more than 120% because of the fork()s. And
> > just for fun i ran the scripted submodule summary too:
> >
> >   scripted     : real	0m3.437s
> >
> > So the forked version outperforms the scripted version by a factor of
> > 7, while the speedup from Dscho's original proposal is almost 17fold.
> 
> Thanks.  I was worried if the forking to ensure correctness may sactifice
> performance so much to be unusable, but it does not seem to be the case,
> given the above comparision.
> 
> I didn't look at the code but presumably it uses the run_command()
> interface and cleanly written?

You seem to be rather unconvinced by rather convincing numbers.

Therefore, I will continue working on the --submodule-summary stuff in 
4msysgit.git.

Ciao,
Dscho