threads / patch / 21129

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

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

## tl;dr

20 messages between Oct 4, 2009 and Oct 7, 2009. Diffs are folded; open one to read it.

replies: 19people: 4as markdown or json

Johannes Schindelin· Oct 4, 2009, 15:05 UTC · lore

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

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
Show changes to 6 files +137 −0

Documentation/diff-options.txt, Makefile, diff.c, diff.h, submodule.c, 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
Junio C Hamano· Oct 4, 2009, 22:19 UTC · re: Johannes Schindelin · lore
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.

Show 19 quoted lines
> 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.

Show 37 quoted lines
> 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?

Show 18 quoted lines
> +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?

Show 21 quoted lines
> +	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?

Show 11 quoted lines
> +	}
> +
> +	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?
Show 12 quoted lines
> +	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?
Show 6 quoted lines
> +			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.

Johannes Sixt· Oct 5, 2009, 06:18 UTC · re: Junio C Hamano · lore
Junio C Hamano schrieb:
Show 22 quoted lines
> 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
Johannes Schindelin· Oct 5, 2009, 09:00 UTC · re: Johannes Sixt · lore
Hi,
On Mon, 5 Oct 2009, Johannes Sixt wrote:
Show 33 quoted lines
> 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

Johannes Sixt· Oct 5, 2009, 09:09 UTC · re: Johannes Schindelin · lore
Johannes Schindelin schrieb:
Show 23 quoted lines
> 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
Johannes Schindelin· Oct 5, 2009, 09:20 UTC · re: Johannes Sixt · lore
Hi,
On Mon, 5 Oct 2009, Johannes Sixt wrote:
Show 36 quoted lines
> 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

Johannes Schindelin· Oct 5, 2009, 09:21 UTC · lore
Hi,
On Mon, 5 Oct 2009, Johannes Schindelin wrote:
Show 8 quoted lines
> 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 --
Show changes to submodule.c +8 −5
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);
Jens Lehmann· Oct 5, 2009, 11:22 UTC · lore
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:
Show changes to 2 files +2 −2

sub, submodule.c

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))
Jens Lehmann· Oct 5, 2009, 17:32 UTC · re: Jens Lehmann · lore

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(-)
Show changes to submodule.c +3 −3
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
Johannes Schindelin· Oct 5, 2009, 20:39 UTC · re: Jens Lehmann · lore
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

Junio C Hamano· Oct 5, 2009, 19:08 UTC · lore
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 8 quoted lines
>> > +		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.

Show 6 quoted lines
>> > +	}
>> > +
>> > +	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 ;-)
Show 11 quoted lines
> 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.

Johannes Schindelin· Oct 5, 2009, 21:08 UTC · re: Junio C Hamano · lore
Hi,
On Mon, 5 Oct 2009, Junio C Hamano wrote:
Show 13 quoted lines
> 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.
Show 19 quoted lines
> > 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.
Show 6 quoted lines
> > 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

Jens Lehmann· Oct 6, 2009, 10:58 UTC · re: Johannes Schindelin · lore
Johannes Schindelin schrieb:
Show 22 quoted lines
>>> 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
Junio C Hamano· Oct 6, 2009, 11:36 UTC · re: Jens Lehmann · lore
Jens Lehmann <Jens.Lehmann@web.de> writes:
Show 8 quoted lines
>> 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.
Johannes Schindelin· Oct 6, 2009, 11:45 UTC · re: Junio C Hamano · lore
Hi,
On Tue, 6 Oct 2009, Junio C Hamano wrote:
Show 15 quoted lines
> 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

Jens Lehmann· Oct 6, 2009, 11:51 UTC · re: Johannes Schindelin · lore
Johannes Schindelin schrieb:
Show 24 quoted lines
> 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
Johannes Schindelin· Oct 6, 2009, 12:10 UTC · re: Jens Lehmann · lore
Hi,
On Tue, 6 Oct 2009, Jens Lehmann wrote:
Show 27 quoted lines
> 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

Jens Lehmann· Oct 7, 2009, 19:32 UTC · re: Junio C Hamano · lore
Junio C Hamano schrieb:
Show 11 quoted lines
> 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
Junio C Hamano· Oct 7, 2009, 20:00 UTC · re: Jens Lehmann · lore
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.

Show 12 quoted lines
> 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?

Johannes Schindelin· Oct 7, 2009, 22:28 UTC · re: Junio C Hamano · lore
Hi,
On Wed, 7 Oct 2009, Junio C Hamano wrote:
Show 32 quoted lines
> 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

← back to recent threads