[PATCH v7 0/7] implement inline submodule diff format

[PATCH v7 0/7] implement inline submodule diff format

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

As suggested by Junio, I broke this patch into several pieces, and
made a common helper function for the submodule header. Note that there
are a couple of complicated modifications to the submodule header
portion which (should) still result in the same header but allow the
diff format a bit better control. It's a bit awkward but it does pass
all the current tests, and the format between the two should be similar.

I tried to break apart the patches properly, so I hope I didn't
accidentally munge them too badly.

Jacob Keller (6):
  graph: add support for --line-prefix on all graph-aware output
  diff: prepare for additional submodule formats
  submodule: allow do_submodule_path to work if given gitdir directly
  submodule: correct output of submodule log format
  submodule: refactor show_submodule_summary with helper function
  diff: teach diff to display submodule difference with an inline diff

Junio C Hamano (1):
  diff.c: remove output_prefix_length field

 Documentation/diff-config.txt                      |   9 +-
 Documentation/diff-options.txt                     |  20 +-
 builtin/rev-list.c                                 |  70 +-
 diff.c                                             |  52 +-
 diff.h                                             |  11 +-
 graph.c                                            | 106 +--
 graph.h                                            |  22 +-
 log-tree.c                                         |   5 +-
 path.c                                             |   4 +-
 submodule.c                                        | 162 ++++-
 submodule.h                                        |   6 +
 t/t4013-diff-various.sh                            |   6 +
 ...diff.diff_--line-prefix=abc_master_master^_side |  29 +
 t/t4013/diff.diff_--line-prefix_--cached_--_file0  |  15 +
 t/t4059-diff-submodule-option-diff-format.sh       | 733 +++++++++++++++++++++
 t/t4202-log.sh                                     | 323 +++++++++
 16 files changed, 1432 insertions(+), 141 deletions(-)
 create mode 100644 t/t4013/diff.diff_--line-prefix=abc_master_master^_side
 create mode 100644 t/t4013/diff.diff_--line-prefix_--cached_--_file0
 create mode 100755 t/t4059-diff-submodule-option-diff-format.sh

-- 
2.10.0.rc0.217.g609f9e8.dirty

[PATCH v7 1/7] diff.c: remove output_prefix_length field

[Jacob Keller]

From: Junio C Hamano <gitster@pobox.com>

"diff/log --stat" has a logic that determines the display columns
available for the diffstat part of the output and apportions it for
pathnames and diffstat graph automatically.

5e71a84a (Add output_prefix_length to diff_options, 2012-04-16)
added the output_prefix_length field to diff_options structure to
allow this logic to subtract the display columns used for the
history graph part from the total "terminal width"; this matters
when the "git log --graph -p" option is in use.

The field must be set to the number of display columns needed to
show the output from the output_prefix() callback, which is error
prone.  As there is only one user of the field, and the user has the
actual value of the prefix string, let's get rid of the field and
have the user count the display width itself.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c  | 2 +-
 diff.h  | 1 -
 graph.c | 2 --
 3 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index 534c12e28ea8..50bef1f07658 100644
--- a/diff.c
+++ b/diff.c
@@ -1625,7 +1625,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 	 */
 
 	if (options->stat_width == -1)
-		width = term_columns() - options->output_prefix_length;
+		width = term_columns() - strlen(line_prefix);
 	else
 		width = options->stat_width ? options->stat_width : 80;
 	number_width = decimal_width(max_change) > number_width ?
diff --git a/diff.h b/diff.h
index 7883729edf10..747a204d75a4 100644
--- a/diff.h
+++ b/diff.h
@@ -174,7 +174,6 @@ struct diff_options {
 	diff_format_fn_t format_callback;
 	void *format_callback_data;
 	diff_prefix_fn_t output_prefix;
-	int output_prefix_length;
 	void *output_prefix_data;
 
 	int diff_path_counter;
diff --git a/graph.c b/graph.c
index dd1720148dc5..a46803840511 100644
--- a/graph.c
+++ b/graph.c
@@ -197,7 +197,6 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void
 	assert(opt);
 	assert(graph);
 
-	opt->output_prefix_length = graph->width;
 	strbuf_reset(&msgbuf);
 	graph_padding_line(graph, &msgbuf);
 	return &msgbuf;
@@ -245,7 +244,6 @@ struct git_graph *graph_init(struct rev_info *opt)
 	 */
 	opt->diffopt.output_prefix = diff_output_prefix_callback;
 	opt->diffopt.output_prefix_data = graph;
-	opt->diffopt.output_prefix_length = 0;
 
 	return graph;
 }
-- 
2.10.0.rc0.217.g609f9e8.dirty

[PATCH v7 2/7] graph: add support for --line-prefix on all graph-aware output

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Add an extension to git-diff and git-log (and any other graph-aware
displayable output) such that "--line-prefix=<string>" will print the
additional line-prefix on every line of output.

To make this work, we have to fix a few bugs in the graph API that force
graph_show_commit_msg to be used only when you have a valid graph.
Additionally, we extend the default_diff_output_prefix handler to work
even when no graph is enabled.

This is somewhat of a hack on top of the graph API, but I think it
should be acceptable here.

This will be used by a future extension of submodule display which
displays the submodule diff as the actual diff between the pre and post
commit in the submodule project.

Add some tests for both git-log and git-diff to ensure that the prefix
is honored correctly.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/diff-options.txt                     |   3 +
 builtin/rev-list.c                                 |  70 ++---
 diff.c                                             |   7 +
 diff.h                                             |   2 +
 graph.c                                            | 104 ++++---
 graph.h                                            |  22 +-
 log-tree.c                                         |   5 +-
 t/t4013-diff-various.sh                            |   6 +
 ...diff.diff_--line-prefix=abc_master_master^_side |  29 ++
 t/t4013/diff.diff_--line-prefix_--cached_--_file0  |  15 +
 t/t4202-log.sh                                     | 323 +++++++++++++++++++++
 11 files changed, 506 insertions(+), 80 deletions(-)
 create mode 100644 t/t4013/diff.diff_--line-prefix=abc_master_master^_side
 create mode 100644 t/t4013/diff.diff_--line-prefix_--cached_--_file0

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 705a87394200..cc4342e2034d 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -569,5 +569,8 @@ endif::git-format-patch[]
 --no-prefix::
 	Do not show any source or destination prefix.
 
+--line-prefix=<prefix>::
+	Prepend an additional prefix to every line of output.
+
 For more detailed explanation on these common options, see also
 linkgit:gitdiffcore[7].
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 0ba82b1635b6..1a75a83538f4 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -122,48 +122,40 @@ static void show_commit(struct commit *commit, void *data)
 		ctx.fmt = revs->commit_format;
 		ctx.output_encoding = get_log_output_encoding();
 		pretty_print_commit(&ctx, commit, &buf);
-		if (revs->graph) {
-			if (buf.len) {
-				if (revs->commit_format != CMIT_FMT_ONELINE)
-					graph_show_oneline(revs->graph);
+		if (buf.len) {
+			if (revs->commit_format != CMIT_FMT_ONELINE)
+				graph_show_oneline(revs->graph);
 
-				graph_show_commit_msg(revs->graph, &buf);
+			graph_show_commit_msg(revs->graph, stdout, &buf);
 
-				/*
-				 * Add a newline after the commit message.
-				 *
-				 * Usually, this newline produces a blank
-				 * padding line between entries, in which case
-				 * we need to add graph padding on this line.
-				 *
-				 * However, the commit message may not end in a
-				 * newline.  In this case the newline simply
-				 * ends the last line of the commit message,
-				 * and we don't need any graph output.  (This
-				 * always happens with CMIT_FMT_ONELINE, and it
-				 * happens with CMIT_FMT_USERFORMAT when the
-				 * format doesn't explicitly end in a newline.)
-				 */
-				if (buf.len && buf.buf[buf.len - 1] == '\n')
-					graph_show_padding(revs->graph);
-				putchar('\n');
-			} else {
-				/*
-				 * If the message buffer is empty, just show
-				 * the rest of the graph output for this
-				 * commit.
-				 */
-				if (graph_show_remainder(revs->graph))
-					putchar('\n');
-				if (revs->commit_format == CMIT_FMT_ONELINE)
-					putchar('\n');
-			}
+			/*
+				* Add a newline after the commit message.
+				*
+				* Usually, this newline produces a blank
+				* padding line between entries, in which case
+				* we need to add graph padding on this line.
+				*
+				* However, the commit message may not end in a
+				* newline.  In this case the newline simply
+				* ends the last line of the commit message,
+				* and we don't need any graph output.  (This
+				* always happens with CMIT_FMT_ONELINE, and it
+				* happens with CMIT_FMT_USERFORMAT when the
+				* format doesn't explicitly end in a newline.)
+				*/
+			if (buf.len && buf.buf[buf.len - 1] == '\n')
+				graph_show_padding(revs->graph);
+			putchar('\n');
 		} else {
-			if (revs->commit_format != CMIT_FMT_USERFORMAT ||
-			    buf.len) {
-				fwrite(buf.buf, 1, buf.len, stdout);
-				putchar(info->hdr_termination);
-			}
+			/*
+				* If the message buffer is empty, just show
+				* the rest of the graph output for this
+				* commit.
+				*/
+			if (graph_show_remainder(revs->graph))
+				putchar('\n');
+			if (revs->commit_format == CMIT_FMT_ONELINE)
+				putchar('\n');
 		}
 		strbuf_release(&buf);
 	} else {
diff --git a/diff.c b/diff.c
index 50bef1f07658..e57cf39ad109 100644
--- a/diff.c
+++ b/diff.c
@@ -18,6 +18,7 @@
 #include "ll-merge.h"
 #include "string-list.h"
 #include "argv-array.h"
+#include "graph.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -3966,6 +3967,12 @@ int diff_opt_parse(struct diff_options *options,
 		options->a_prefix = optarg;
 		return argcount;
 	}
+	else if ((argcount = parse_long_opt("line-prefix", av, &optarg))) {
+		options->line_prefix = optarg;
+		options->line_prefix_length = strlen(options->line_prefix);
+		graph_setup_line_prefix(options);
+		return argcount;
+	}
 	else if ((argcount = parse_long_opt("dst-prefix", av, &optarg))) {
 		options->b_prefix = optarg;
 		return argcount;
diff --git a/diff.h b/diff.h
index 747a204d75a4..1f57aad25c71 100644
--- a/diff.h
+++ b/diff.h
@@ -115,6 +115,8 @@ struct diff_options {
 	const char *pickaxe;
 	const char *single_follow;
 	const char *a_prefix, *b_prefix;
+	const char *line_prefix;
+	size_t line_prefix_length;
 	unsigned flags;
 	unsigned touched_flags;
 
diff --git a/graph.c b/graph.c
index a46803840511..b140c096b7f3 100644
--- a/graph.c
+++ b/graph.c
@@ -2,7 +2,6 @@
 #include "commit.h"
 #include "color.h"
 #include "graph.h"
-#include "diff.h"
 #include "revision.h"
 
 /* Internal API */
@@ -28,8 +27,15 @@ static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);
  * responsible for printing this line's graph (perhaps via
  * graph_show_commit() or graph_show_oneline()) before calling
  * graph_show_strbuf().
+ *
+ * Note that unlike some other graph display functions, you must pass the file
+ * handle directly. It is assumed that this is the same file handle as the
+ * file specified by the graph diff options. This is necessary so that
+ * graph_show_strbuf can be called even with a NULL graph.
  */
-static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);
+static void graph_show_strbuf(struct git_graph *graph,
+			      FILE *file,
+			      struct strbuf const *sb);
 
 /*
  * TODO:
@@ -59,6 +65,17 @@ enum graph_state {
 	GRAPH_COLLAPSING
 };
 
+static void graph_show_line_prefix(const struct diff_options *diffopt)
+{
+	if (!diffopt || !diffopt->line_prefix)
+		return;
+
+	fwrite(diffopt->line_prefix,
+	       sizeof(char),
+	       diffopt->line_prefix_length,
+	       diffopt->file);
+}
+
 static const char **column_colors;
 static unsigned short column_colors_max;
 
@@ -195,13 +212,28 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void
 	static struct strbuf msgbuf = STRBUF_INIT;
 
 	assert(opt);
-	assert(graph);
 
 	strbuf_reset(&msgbuf);
-	graph_padding_line(graph, &msgbuf);
+	if (opt->line_prefix)
+		strbuf_add(&msgbuf, opt->line_prefix,
+			   opt->line_prefix_length);
+	if (graph)
+		graph_padding_line(graph, &msgbuf);
 	return &msgbuf;
 }
 
+static const struct diff_options *default_diffopt;
+
+void graph_setup_line_prefix(struct diff_options *diffopt)
+{
+	default_diffopt = diffopt;
+
+	/* setup an output prefix callback if necessary */
+	if (diffopt && !diffopt->output_prefix)
+		diffopt->output_prefix = diff_output_prefix_callback;
+}
+
+
 struct git_graph *graph_init(struct rev_info *opt)
 {
 	struct git_graph *graph = xmalloc(sizeof(struct git_graph));
@@ -1183,8 +1215,10 @@ void graph_show_commit(struct git_graph *graph)
 	struct strbuf msgbuf = STRBUF_INIT;
 	int shown_commit_line = 0;
 
-	if (!graph)
+	if (!graph) {
+		graph_show_line_prefix(default_diffopt);
 		return;
+	}
 
 	/*
 	 * When showing a diff of a merge against each of its parents, we
@@ -1198,6 +1232,7 @@ void graph_show_commit(struct git_graph *graph)
 
 	while (!shown_commit_line && !graph_is_commit_finished(graph)) {
 		shown_commit_line = graph_next_line(graph, &msgbuf);
+		graph_show_line_prefix(&graph->revs->diffopt);
 		fwrite(msgbuf.buf, sizeof(char), msgbuf.len,
 			graph->revs->diffopt.file);
 		if (!shown_commit_line)
@@ -1212,10 +1247,13 @@ void graph_show_oneline(struct git_graph *graph)
 {
 	struct strbuf msgbuf = STRBUF_INIT;
 
-	if (!graph)
+	if (!graph) {
+		graph_show_line_prefix(default_diffopt);
 		return;
+	}
 
 	graph_next_line(graph, &msgbuf);
+	graph_show_line_prefix(&graph->revs->diffopt);
 	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, graph->revs->diffopt.file);
 	strbuf_release(&msgbuf);
 }
@@ -1224,10 +1262,13 @@ void graph_show_padding(struct git_graph *graph)
 {
 	struct strbuf msgbuf = STRBUF_INIT;
 
-	if (!graph)
+	if (!graph) {
+		graph_show_line_prefix(default_diffopt);
 		return;
+	}
 
 	graph_padding_line(graph, &msgbuf);
+	graph_show_line_prefix(&graph->revs->diffopt);
 	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, graph->revs->diffopt.file);
 	strbuf_release(&msgbuf);
 }
@@ -1237,14 +1278,19 @@ int graph_show_remainder(struct git_graph *graph)
 	struct strbuf msgbuf = STRBUF_INIT;
 	int shown = 0;
 
-	if (!graph)
+	if (!graph) {
+		graph_show_line_prefix(default_diffopt);
 		return 0;
+	}
 
-	if (graph_is_commit_finished(graph))
+	if (graph_is_commit_finished(graph)) {
+		graph_show_line_prefix(&graph->revs->diffopt);
 		return 0;
+	}
 
 	for (;;) {
 		graph_next_line(graph, &msgbuf);
+		graph_show_line_prefix(&graph->revs->diffopt);
 		fwrite(msgbuf.buf, sizeof(char), msgbuf.len,
 			graph->revs->diffopt.file);
 		strbuf_setlen(&msgbuf, 0);
@@ -1260,17 +1306,12 @@ int graph_show_remainder(struct git_graph *graph)
 	return shown;
 }
 
-
-static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
+static void graph_show_strbuf(struct git_graph *graph,
+			      FILE *file,
+			      struct strbuf const *sb)
 {
 	char *p;
 
-	if (!graph) {
-		fwrite(sb->buf, sizeof(char), sb->len,
-			graph->revs->diffopt.file);
-		return;
-	}
-
 	/*
 	 * Print the strbuf line by line,
 	 * and display the graph info before each line but the first.
@@ -1285,7 +1326,7 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
 		} else {
 			len = (sb->buf + sb->len) - p;
 		}
-		fwrite(p, sizeof(char), len, graph->revs->diffopt.file);
+		fwrite(p, sizeof(char), len, file);
 		if (next_p && *next_p != '\0')
 			graph_show_oneline(graph);
 		p = next_p;
@@ -1293,29 +1334,20 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
 }
 
 void graph_show_commit_msg(struct git_graph *graph,
+			   FILE *file,
 			   struct strbuf const *sb)
 {
 	int newline_terminated;
 
-	if (!graph) {
-		/*
-		 * If there's no graph, just print the message buffer.
-		 *
-		 * The message buffer for CMIT_FMT_ONELINE and
-		 * CMIT_FMT_USERFORMAT are already missing a terminating
-		 * newline.  All of the other formats should have it.
-		 */
-		fwrite(sb->buf, sizeof(char), sb->len,
-			graph->revs->diffopt.file);
-		return;
-	}
-
-	newline_terminated = (sb->len && sb->buf[sb->len - 1] == '\n');
-
 	/*
 	 * Show the commit message
 	 */
-	graph_show_strbuf(graph, sb);
+	graph_show_strbuf(graph, file, sb);
+
+	if (!graph)
+		return;
+
+	newline_terminated = (sb->len && sb->buf[sb->len - 1] == '\n');
 
 	/*
 	 * If there is more output needed for this commit, show it now
@@ -1327,7 +1359,7 @@ void graph_show_commit_msg(struct git_graph *graph,
 		 * new line.
 		 */
 		if (!newline_terminated)
-			putc('\n', graph->revs->diffopt.file);
+			putc('\n', file);
 
 		graph_show_remainder(graph);
 
@@ -1335,6 +1367,6 @@ void graph_show_commit_msg(struct git_graph *graph,
 		 * If sb ends with a newline, our output should too.
 		 */
 		if (newline_terminated)
-			putc('\n', graph->revs->diffopt.file);
+			putc('\n', file);
 	}
 }
diff --git a/graph.h b/graph.h
index 3f48c19b6208..af623390b605 100644
--- a/graph.h
+++ b/graph.h
@@ -1,9 +1,22 @@
 #ifndef GRAPH_H
 #define GRAPH_H
+#include "diff.h"
 
 /* A graph is a pointer to this opaque structure */
 struct git_graph;
 
+/*
+ * Called to setup global display of line_prefix diff option.
+ *
+ * Passed a diff_options structure which indicates the line_prefix and the
+ * file to output the prefix to. This is sort of a hack used so that the
+ * line_prefix will be honored by all flows which also honor "--graph"
+ * regardless of whether a graph has actually been setup. The normal graph
+ * flow will honor the exact diff_options passed, but a NULL graph will cause
+ * display of a line_prefix to stdout.
+ */
+void graph_setup_line_prefix(struct diff_options *diffopt);
+
 /*
  * Set up a custom scheme for column colors.
  *
@@ -113,7 +126,14 @@ int graph_show_remainder(struct git_graph *graph);
  * missing a terminating newline (including if it is empty), the output
  * printed by graph_show_commit_msg() will also be missing a terminating
  * newline.
+ *
+ * Note that unlike some other graph display functions, you must pass the file
+ * handle directly. It is assumed that this is the same file handle as the
+ * file specified by the graph diff options. This is necessary so that
+ * graph_show_commit_msg can be called even with a NULL graph.
  */
-void graph_show_commit_msg(struct git_graph *graph, struct strbuf const *sb);
+void graph_show_commit_msg(struct git_graph *graph,
+			   FILE *file,
+			   struct strbuf const *sb);
 
 #endif /* GRAPH_H */
diff --git a/log-tree.c b/log-tree.c
index bfb735c84556..8c2415747a26 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -715,10 +715,7 @@ void show_log(struct rev_info *opt)
 	else
 		opt->missing_newline = 0;
 
-	if (opt->graph)
-		graph_show_commit_msg(opt->graph, &msgbuf);
-	else
-		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, opt->diffopt.file);
+	graph_show_commit_msg(opt->graph, opt->diffopt.file, &msgbuf);
 	if (opt->use_terminator && !commit_format_is_empty(opt->commit_format)) {
 		if (!opt->missing_newline)
 			graph_show_padding(opt->graph);
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 94ef5000e787..566817e2efdc 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -306,6 +306,8 @@ diff --no-index --name-status dir2 dir
 diff --no-index --name-status -- dir2 dir
 diff --no-index dir dir3
 diff master master^ side
+# Can't use spaces...
+diff --line-prefix=abc master master^ side
 diff --dirstat master~1 master~2
 diff --dirstat initial rearrange
 diff --dirstat-by-file initial rearrange
@@ -325,6 +327,10 @@ test_expect_success 'diff --cached -- file on unborn branch' '
 	git diff --cached -- file0 >result &&
 	test_cmp "$TEST_DIRECTORY/t4013/diff.diff_--cached_--_file0" result
 '
+test_expect_success 'diff --line-prefix with spaces' '
+	git diff --line-prefix="| | | " --cached -- file0 >result &&
+	test_cmp "$TEST_DIRECTORY/t4013/diff.diff_--line-prefix_--cached_--_file0" result
+'
 
 test_expect_success 'diff-tree --stdin with log formatting' '
 	cat >expect <<-\EOF &&
diff --git a/t/t4013/diff.diff_--line-prefix=abc_master_master^_side b/t/t4013/diff.diff_--line-prefix=abc_master_master^_side
new file mode 100644
index 000000000000..99f91e7f0e32
--- /dev/null
+++ b/t/t4013/diff.diff_--line-prefix=abc_master_master^_side
@@ -0,0 +1,29 @@
+$ git diff --line-prefix=abc master master^ side
+abcdiff --cc dir/sub
+abcindex cead32e,7289e35..992913c
+abc--- a/dir/sub
+abc+++ b/dir/sub
+abc@@@ -1,6 -1,4 +1,8 @@@
+abc  A
+abc  B
+abc +C
+abc +D
+abc +E
+abc +F
+abc+ 1
+abc+ 2
+abcdiff --cc file0
+abcindex b414108,f4615da..10a8a9f
+abc--- a/file0
+abc+++ b/file0
+abc@@@ -1,6 -1,6 +1,9 @@@
+abc  1
+abc  2
+abc  3
+abc +4
+abc +5
+abc +6
+abc+ A
+abc+ B
+abc+ C
+$
diff --git a/t/t4013/diff.diff_--line-prefix_--cached_--_file0 b/t/t4013/diff.diff_--line-prefix_--cached_--_file0
new file mode 100644
index 000000000000..f41ba4d36aa1
--- /dev/null
+++ b/t/t4013/diff.diff_--line-prefix_--cached_--_file0
@@ -0,0 +1,15 @@
+| | | diff --git a/file0 b/file0
+| | | new file mode 100644
+| | | index 0000000..10a8a9f
+| | | --- /dev/null
+| | | +++ b/file0
+| | | @@ -0,0 +1,9 @@
+| | | +1
+| | | +2
+| | | +3
+| | | +4
+| | | +5
+| | | +6
+| | | +A
+| | | +B
+| | | +C
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index e2db47c36e09..1ccbd5948a73 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -187,6 +187,16 @@ test_expect_success 'git log --no-walk=sorted <commits> sorts by commit time' '
 	test_cmp expect actual
 '
 
+cat > expect << EOF
+=== 804a787 sixth
+=== 394ef78 fifth
+=== 5d31159 fourth
+EOF
+test_expect_success 'git log --line-prefix="=== " --no-walk <commits> sorts by commit time' '
+	git log --line-prefix="=== " --no-walk --oneline 5d31159 804a787 394ef78 > actual &&
+	test_cmp expect actual
+'
+
 cat > expect << EOF
 5d31159 fourth
 804a787 sixth
@@ -284,6 +294,21 @@ test_expect_success 'simple log --graph' '
 	test_cmp expect actual
 '
 
+cat > expect <<EOF
+123 * Second
+123 * sixth
+123 * fifth
+123 * fourth
+123 * third
+123 * second
+123 * initial
+EOF
+
+test_expect_success 'simple log --graph --line-prefix="123 "' '
+	git log --graph --line-prefix="123 " --pretty=tformat:%s >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'set up merge history' '
 	git checkout -b side HEAD~4 &&
 	test_commit side-1 1 1 &&
@@ -313,6 +338,27 @@ test_expect_success 'log --graph with merge' '
 	test_cmp expect actual
 '
 
+cat > expect <<\EOF
+| | | *   Merge branch 'side'
+| | | |\
+| | | | * side-2
+| | | | * side-1
+| | | * | Second
+| | | * | sixth
+| | | * | fifth
+| | | * | fourth
+| | | |/
+| | | * third
+| | | * second
+| | | * initial
+EOF
+
+test_expect_success 'log --graph --line-prefix="| | | " with merge' '
+	git log --line-prefix="| | | " --graph --date-order --pretty=tformat:%s |
+		sed "s/ *\$//" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'log --raw --graph -m with merge' '
 	git log --raw --graph --oneline -m master | head -n 500 >actual &&
 	grep "initial" actual
@@ -867,6 +913,283 @@ test_expect_success 'log --graph with diff and stats' '
 	test_i18ncmp expect actual.sanitized
 '
 
+cat >expect <<\EOF
+*** *   commit COMMIT_OBJECT_NAME
+*** |\  Merge: MERGE_PARENTS
+*** | | Author: A U Thor <author@example.com>
+*** | |
+*** | |     Merge HEADS DESCRIPTION
+*** | |
+*** | * commit COMMIT_OBJECT_NAME
+*** | | Author: A U Thor <author@example.com>
+*** | |
+*** | |     reach
+*** | | ---
+*** | |  reach.t | 1 +
+*** | |  1 file changed, 1 insertion(+)
+*** | |
+*** | | diff --git a/reach.t b/reach.t
+*** | | new file mode 100644
+*** | | index 0000000..10c9591
+*** | | --- /dev/null
+*** | | +++ b/reach.t
+*** | | @@ -0,0 +1 @@
+*** | | +reach
+*** | |
+*** |  \
+*** *-. \   commit COMMIT_OBJECT_NAME
+*** |\ \ \  Merge: MERGE_PARENTS
+*** | | | | Author: A U Thor <author@example.com>
+*** | | | |
+*** | | | |     Merge HEADS DESCRIPTION
+*** | | | |
+*** | | * | commit COMMIT_OBJECT_NAME
+*** | | |/  Author: A U Thor <author@example.com>
+*** | | |
+*** | | |       octopus-b
+*** | | |   ---
+*** | | |    octopus-b.t | 1 +
+*** | | |    1 file changed, 1 insertion(+)
+*** | | |
+*** | | |   diff --git a/octopus-b.t b/octopus-b.t
+*** | | |   new file mode 100644
+*** | | |   index 0000000..d5fcad0
+*** | | |   --- /dev/null
+*** | | |   +++ b/octopus-b.t
+*** | | |   @@ -0,0 +1 @@
+*** | | |   +octopus-b
+*** | | |
+*** | * | commit COMMIT_OBJECT_NAME
+*** | |/  Author: A U Thor <author@example.com>
+*** | |
+*** | |       octopus-a
+*** | |   ---
+*** | |    octopus-a.t | 1 +
+*** | |    1 file changed, 1 insertion(+)
+*** | |
+*** | |   diff --git a/octopus-a.t b/octopus-a.t
+*** | |   new file mode 100644
+*** | |   index 0000000..11ee015
+*** | |   --- /dev/null
+*** | |   +++ b/octopus-a.t
+*** | |   @@ -0,0 +1 @@
+*** | |   +octopus-a
+*** | |
+*** * | commit COMMIT_OBJECT_NAME
+*** |/  Author: A U Thor <author@example.com>
+*** |
+*** |       seventh
+*** |   ---
+*** |    seventh.t | 1 +
+*** |    1 file changed, 1 insertion(+)
+*** |
+*** |   diff --git a/seventh.t b/seventh.t
+*** |   new file mode 100644
+*** |   index 0000000..9744ffc
+*** |   --- /dev/null
+*** |   +++ b/seventh.t
+*** |   @@ -0,0 +1 @@
+*** |   +seventh
+*** |
+*** *   commit COMMIT_OBJECT_NAME
+*** |\  Merge: MERGE_PARENTS
+*** | | Author: A U Thor <author@example.com>
+*** | |
+*** | |     Merge branch 'tangle'
+*** | |
+*** | *   commit COMMIT_OBJECT_NAME
+*** | |\  Merge: MERGE_PARENTS
+*** | | | Author: A U Thor <author@example.com>
+*** | | |
+*** | | |     Merge branch 'side' (early part) into tangle
+*** | | |
+*** | * |   commit COMMIT_OBJECT_NAME
+*** | |\ \  Merge: MERGE_PARENTS
+*** | | | | Author: A U Thor <author@example.com>
+*** | | | |
+*** | | | |     Merge branch 'master' (early part) into tangle
+*** | | | |
+*** | * | | commit COMMIT_OBJECT_NAME
+*** | | | | Author: A U Thor <author@example.com>
+*** | | | |
+*** | | | |     tangle-a
+*** | | | | ---
+*** | | | |  tangle-a | 1 +
+*** | | | |  1 file changed, 1 insertion(+)
+*** | | | |
+*** | | | | diff --git a/tangle-a b/tangle-a
+*** | | | | new file mode 100644
+*** | | | | index 0000000..7898192
+*** | | | | --- /dev/null
+*** | | | | +++ b/tangle-a
+*** | | | | @@ -0,0 +1 @@
+*** | | | | +a
+*** | | | |
+*** * | | |   commit COMMIT_OBJECT_NAME
+*** |\ \ \ \  Merge: MERGE_PARENTS
+*** | | | | | Author: A U Thor <author@example.com>
+*** | | | | |
+*** | | | | |     Merge branch 'side'
+*** | | | | |
+*** | * | | | commit COMMIT_OBJECT_NAME
+*** | | |_|/  Author: A U Thor <author@example.com>
+*** | |/| |
+*** | | | |       side-2
+*** | | | |   ---
+*** | | | |    2 | 1 +
+*** | | | |    1 file changed, 1 insertion(+)
+*** | | | |
+*** | | | |   diff --git a/2 b/2
+*** | | | |   new file mode 100644
+*** | | | |   index 0000000..0cfbf08
+*** | | | |   --- /dev/null
+*** | | | |   +++ b/2
+*** | | | |   @@ -0,0 +1 @@
+*** | | | |   +2
+*** | | | |
+*** | * | | commit COMMIT_OBJECT_NAME
+*** | | | | Author: A U Thor <author@example.com>
+*** | | | |
+*** | | | |     side-1
+*** | | | | ---
+*** | | | |  1 | 1 +
+*** | | | |  1 file changed, 1 insertion(+)
+*** | | | |
+*** | | | | diff --git a/1 b/1
+*** | | | | new file mode 100644
+*** | | | | index 0000000..d00491f
+*** | | | | --- /dev/null
+*** | | | | +++ b/1
+*** | | | | @@ -0,0 +1 @@
+*** | | | | +1
+*** | | | |
+*** * | | | commit COMMIT_OBJECT_NAME
+*** | | | | Author: A U Thor <author@example.com>
+*** | | | |
+*** | | | |     Second
+*** | | | | ---
+*** | | | |  one | 1 +
+*** | | | |  1 file changed, 1 insertion(+)
+*** | | | |
+*** | | | | diff --git a/one b/one
+*** | | | | new file mode 100644
+*** | | | | index 0000000..9a33383
+*** | | | | --- /dev/null
+*** | | | | +++ b/one
+*** | | | | @@ -0,0 +1 @@
+*** | | | | +case
+*** | | | |
+*** * | | | commit COMMIT_OBJECT_NAME
+*** | |_|/  Author: A U Thor <author@example.com>
+*** |/| |
+*** | | |       sixth
+*** | | |   ---
+*** | | |    a/two | 1 -
+*** | | |    1 file changed, 1 deletion(-)
+*** | | |
+*** | | |   diff --git a/a/two b/a/two
+*** | | |   deleted file mode 100644
+*** | | |   index 9245af5..0000000
+*** | | |   --- a/a/two
+*** | | |   +++ /dev/null
+*** | | |   @@ -1 +0,0 @@
+*** | | |   -ni
+*** | | |
+*** * | | commit COMMIT_OBJECT_NAME
+*** | | | Author: A U Thor <author@example.com>
+*** | | |
+*** | | |     fifth
+*** | | | ---
+*** | | |  a/two | 1 +
+*** | | |  1 file changed, 1 insertion(+)
+*** | | |
+*** | | | diff --git a/a/two b/a/two
+*** | | | new file mode 100644
+*** | | | index 0000000..9245af5
+*** | | | --- /dev/null
+*** | | | +++ b/a/two
+*** | | | @@ -0,0 +1 @@
+*** | | | +ni
+*** | | |
+*** * | | commit COMMIT_OBJECT_NAME
+*** |/ /  Author: A U Thor <author@example.com>
+*** | |
+*** | |       fourth
+*** | |   ---
+*** | |    ein | 1 +
+*** | |    1 file changed, 1 insertion(+)
+*** | |
+*** | |   diff --git a/ein b/ein
+*** | |   new file mode 100644
+*** | |   index 0000000..9d7e69f
+*** | |   --- /dev/null
+*** | |   +++ b/ein
+*** | |   @@ -0,0 +1 @@
+*** | |   +ichi
+*** | |
+*** * | commit COMMIT_OBJECT_NAME
+*** |/  Author: A U Thor <author@example.com>
+*** |
+*** |       third
+*** |   ---
+*** |    ichi | 1 +
+*** |    one  | 1 -
+*** |    2 files changed, 1 insertion(+), 1 deletion(-)
+*** |
+*** |   diff --git a/ichi b/ichi
+*** |   new file mode 100644
+*** |   index 0000000..9d7e69f
+*** |   --- /dev/null
+*** |   +++ b/ichi
+*** |   @@ -0,0 +1 @@
+*** |   +ichi
+*** |   diff --git a/one b/one
+*** |   deleted file mode 100644
+*** |   index 9d7e69f..0000000
+*** |   --- a/one
+*** |   +++ /dev/null
+*** |   @@ -1 +0,0 @@
+*** |   -ichi
+*** |
+*** * commit COMMIT_OBJECT_NAME
+*** | Author: A U Thor <author@example.com>
+*** |
+*** |     second
+*** | ---
+*** |  one | 2 +-
+*** |  1 file changed, 1 insertion(+), 1 deletion(-)
+*** |
+*** | diff --git a/one b/one
+*** | index 5626abf..9d7e69f 100644
+*** | --- a/one
+*** | +++ b/one
+*** | @@ -1 +1 @@
+*** | -one
+*** | +ichi
+*** |
+*** * commit COMMIT_OBJECT_NAME
+***   Author: A U Thor <author@example.com>
+***
+***       initial
+***   ---
+***    one | 1 +
+***    1 file changed, 1 insertion(+)
+***
+***   diff --git a/one b/one
+***   new file mode 100644
+***   index 0000000..5626abf
+***   --- /dev/null
+***   +++ b/one
+***   @@ -0,0 +1 @@
+***   +one
+EOF
+
+test_expect_success 'log --line-prefix="*** " --graph with diff and stats' '
+	git log --line-prefix="*** " --no-renames --graph --pretty=short --stat -p >actual &&
+	sanitize_output >actual.sanitized <actual &&
+	test_i18ncmp expect actual.sanitized
+'
+
 test_expect_success 'dotdot is a parent directory' '
 	mkdir -p a/b &&
 	( echo sixth && echo fifth ) >expect &&
-- 
2.10.0.rc0.217.g609f9e8.dirty

[PATCH v7 3/7] diff: prepare for additional submodule formats

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

A future patch will add a new format for displaying the difference of
a submodule. Make it easier by changing how we store the current
selected format. Replace the DIFF_OPT flag with an enumeration, as each
format will be mutually exclusive.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 diff.c | 12 ++++++------
 diff.h |  7 ++++++-
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/diff.c b/diff.c
index e57cf39ad109..d6b321da3d1d 100644
--- a/diff.c
+++ b/diff.c
@@ -132,9 +132,9 @@ static int parse_dirstat_params(struct diff_options *options, const char *params
 static int parse_submodule_params(struct diff_options *options, const char *value)
 {
 	if (!strcmp(value, "log"))
-		DIFF_OPT_SET(options, SUBMODULE_LOG);
+		options->submodule_format = DIFF_SUBMODULE_LOG;
 	else if (!strcmp(value, "short"))
-		DIFF_OPT_CLR(options, SUBMODULE_LOG);
+		options->submodule_format = DIFF_SUBMODULE_SHORT;
 	else
 		return -1;
 	return 0;
@@ -2300,9 +2300,9 @@ static void builtin_diff(const char *name_a,
 	struct strbuf header = STRBUF_INIT;
 	const char *line_prefix = diff_line_prefix(o);
 
-	if (DIFF_OPT_TST(o, SUBMODULE_LOG) &&
-			(!one->mode || S_ISGITLINK(one->mode)) &&
-			(!two->mode || S_ISGITLINK(two->mode))) {
+	if (o->submodule_format == DIFF_SUBMODULE_LOG &&
+	    (!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->path ? one->path : two->path,
@@ -3916,7 +3916,7 @@ int diff_opt_parse(struct diff_options *options,
 		DIFF_OPT_SET(options, OVERRIDE_SUBMODULE_CONFIG);
 		handle_ignore_submodules_arg(options, arg);
 	} else if (!strcmp(arg, "--submodule"))
-		DIFF_OPT_SET(options, SUBMODULE_LOG);
+		options->submodule_format = DIFF_SUBMODULE_LOG;
 	else if (skip_prefix(arg, "--submodule=", &arg))
 		return parse_submodule_opt(options, arg);
 	else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
diff --git a/diff.h b/diff.h
index 1f57aad25c71..ea5aba668eaa 100644
--- a/diff.h
+++ b/diff.h
@@ -83,7 +83,6 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 #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_SUBMODULE_LOG       (1 << 23)
 #define DIFF_OPT_DIRTY_SUBMODULES    (1 << 24)
 #define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 25)
 #define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 26)
@@ -110,6 +109,11 @@ enum diff_words_type {
 	DIFF_WORDS_COLOR
 };
 
+enum diff_submodule_format {
+	DIFF_SUBMODULE_SHORT = 0,
+	DIFF_SUBMODULE_LOG,
+};
+
 struct diff_options {
 	const char *orderfile;
 	const char *pickaxe;
@@ -157,6 +161,7 @@ struct diff_options {
 	int stat_count;
 	const char *word_regex;
 	enum diff_words_type word_diff;
+	enum diff_submodule_format submodule_format;
 
 	/* this is set by diffcore for DIFF_FORMAT_PATCH */
 	int found_changes;
-- 
2.10.0.rc0.217.g609f9e8.dirty

[PATCH v7 4/7] submodule: allow do_submodule_path to work if given gitdir directly

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Currently, do_submodule_path relies on read_gitfile, which will die() if
it can't read from the specified gitfile. Unfortunately, this means that
do_submodule_path will not work when given the path to a submodule which
is checked out directly, such as a newly added submodule which you
cloned and then "git submodule add". Instead, replace the call with
resolve_gitdir. This first checks to see if we've been given a gitdir
already.

Because resolve_gitdir may return the same buffer it was passed, we have
to check for this case as well, since strbuf_reset() will not work as
expected here, and indeed is not necessary.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 path.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/path.c b/path.c
index 17551c483476..d1af029152a2 100644
--- a/path.c
+++ b/path.c
@@ -477,8 +477,8 @@ static void do_submodule_path(struct strbuf *buf, const char *path,
 	strbuf_complete(buf, '/');
 	strbuf_addstr(buf, ".git");
 
-	git_dir = read_gitfile(buf->buf);
-	if (git_dir) {
+	git_dir = resolve_gitdir(buf->buf);
+	if (git_dir && git_dir != buf->buf) {
 		strbuf_reset(buf);
 		strbuf_addstr(buf, git_dir);
 	}
-- 
2.10.0.rc0.217.g609f9e8.dirty

[PATCH v7 5/7] submodule: correct output of submodule log format

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

The submodule log diff output incorrectly states that the submodule is
"not checked out" in cases where it wants to say the submodule is "not
initialized". Change the wording to reflect the actual check being
performed.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 submodule.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index 1b5cdfb7e784..e1a51b7506ff 100644
--- a/submodule.c
+++ b/submodule.c
@@ -348,7 +348,7 @@ void show_submodule_summary(FILE *f, const char *path,
 	if (is_null_sha1(two))
 		message = "(submodule deleted)";
 	else if (add_submodule_odb(path))
-		message = "(not checked out)";
+		message = "(not initialized)";
 	else if (is_null_sha1(one))
 		message = "(new submodule)";
 	else if (!(left = lookup_commit_reference(one)) ||
-- 
2.10.0.rc0.217.g609f9e8.dirty

[PATCH v7 6/7] submodule: refactor show_submodule_summary with helper function

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

A future patch is going to add a new submodule diff format which
displays an inline diff of the submodule changes. To make this easier,
and to ensure that both submodule diff formats use the same initial
header, factor out show_submodule_header() function which will print the
current submodule header line, and then leave the show_submodule_summary
function to lookup and print the submodule log format.

This does create one format change in that "(revision walker failed)"
will now be displayed on its own line rather than as part of the message
because we no longer perform this step directly in the header display
flow. However, this is a rare case and shouldn't impact much. In
addition, since we no longer need to run prepare_submodule_summary to
get the fast_backward and fast_forward variables, these have been
removed from that call, and the show_submodule_header() function does
its own mergebase lookup.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 submodule.c | 103 +++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 74 insertions(+), 29 deletions(-)

diff --git a/submodule.c b/submodule.c
index e1a51b7506ff..e341ca7ffefd 100644
--- a/submodule.c
+++ b/submodule.c
@@ -277,8 +277,7 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt,
 }
 
 static int prepare_submodule_summary(struct rev_info *rev, const char *path,
-		struct commit *left, struct commit *right,
-		int *fast_forward, int *fast_backward)
+		struct commit *left, struct commit *right)
 {
 	struct commit_list *merge_bases, *list;
 
@@ -290,12 +289,6 @@ static int prepare_submodule_summary(struct rev_info *rev, const char *path,
 	add_pending_object(rev, &left->object, path);
 	add_pending_object(rev, &right->object, path);
 	merge_bases = get_merge_bases(left, right);
-	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,
@@ -333,31 +326,23 @@ static void print_submodule_summary(struct rev_info *rev, FILE *f,
 	strbuf_release(&sb);
 }
 
-void show_submodule_summary(FILE *f, const char *path,
+/* Helper function to display the submodule header line prior to the full
+ * summary output. If it can locate the submodule objects directory it will
+ * attempt to lookup both the left and right commits and put them into the
+ * left and right pointers.
+ */
+static void show_submodule_header(FILE *f, const char *path,
 		const char *line_prefix,
 		unsigned char one[20], unsigned char two[20],
 		unsigned dirty_submodule, const char *meta,
-		const char *del, const char *add, const char *reset)
+		const char *reset,
+		struct commit **left, struct commit **right)
 {
-	struct rev_info rev;
-	struct commit *left = NULL, *right = NULL;
+	struct commit_list *merge_bases;
 	const char *message = NULL;
 	struct strbuf sb = STRBUF_INIT;
 	int fast_forward = 0, fast_backward = 0;
 
-	if (is_null_sha1(two))
-		message = "(submodule deleted)";
-	else if (add_submodule_odb(path))
-		message = "(not initialized)";
-	else if (is_null_sha1(one))
-		message = "(new submodule)";
-	else if (!(left = lookup_commit_reference(one)) ||
-		 !(right = lookup_commit_reference(two)))
-		message = "(commits not present)";
-	else if (prepare_submodule_summary(&rev, path, left, right,
-					   &fast_forward, &fast_backward))
-		message = "(revision walker failed)";
-
 	if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
 		fprintf(f, "%sSubmodule %s contains untracked content\n",
 			line_prefix, path);
@@ -365,11 +350,46 @@ void show_submodule_summary(FILE *f, const char *path,
 		fprintf(f, "%sSubmodule %s contains modified content\n",
 			line_prefix, path);
 
+	if (is_null_sha1(one))
+		message = "(new submodule)";
+	else if (is_null_sha1(two))
+		message = "(submodule deleted)";
+
+	if (add_submodule_odb(path)) {
+		if (!message)
+			message = "(submodule not initialized)";
+		goto output_header;
+	}
+
+	/*
+	 * Attempt to lookup the commit references, and determine if this is
+	 * a fast forward or fast backwards update
+	 */
+	*left = lookup_commit_reference(one);
+	*right = lookup_commit_reference(two);
+
+	/*
+	 * Warn about missing commits in the submodule project, but only if
+	 * they aren't null
+	 */
+	if ((!is_null_sha1(one) && !*left) ||
+	     (!is_null_sha1(two) && !*right))
+		message = "(commits not present)";
+
+	merge_bases = get_merge_bases(*left, *right);
+	if (merge_bases) {
+		if (merge_bases->item == *left)
+			fast_forward = 1;
+		else if (merge_bases->item == *right)
+			fast_backward = 1;
+	}
+
 	if (!hashcmp(one, two)) {
 		strbuf_release(&sb);
 		return;
 	}
 
+output_header:
 	strbuf_addf(&sb, "%s%sSubmodule %s %s..", line_prefix, meta, path,
 			find_unique_abbrev(one, DEFAULT_ABBREV));
 	if (!fast_backward && !fast_forward)
@@ -381,14 +401,39 @@ void show_submodule_summary(FILE *f, const char *path,
 		strbuf_addf(&sb, "%s:%s\n", fast_backward ? " (rewind)" : "", reset);
 	fwrite(sb.buf, sb.len, 1, f);
 
-	if (!message) /* only NULL if we succeeded in setting up the walk */
-		print_submodule_summary(&rev, f, line_prefix, del, add, reset);
+	strbuf_release(&sb);
+}
+
+void show_submodule_summary(FILE *f, const char *path,
+		const char *line_prefix,
+		unsigned char one[20], unsigned char two[20],
+		unsigned dirty_submodule, const char *meta,
+		const char *del, const char *add, const char *reset)
+{
+	struct rev_info rev;
+	struct commit *left = NULL, *right = NULL;
+
+	show_submodule_header(f, path, line_prefix, one, two, dirty_submodule,
+			      meta, reset, &left, &right);
+
+	/*
+	 * if we don't have both a left and a right pointer, then we can't
+	 * display a summary
+	 */
+	if (!left || !right)
+		return;
+
+	if (prepare_submodule_summary(&rev, path, left, right)) {
+		fprintf(f, "%s(revision walker failed)\n", line_prefix);
+		return;
+	}
+
+	print_submodule_summary(&rev, f, line_prefix, del, add, reset);
+
 	if (left)
 		clear_commit_marks(left, ~0);
 	if (right)
 		clear_commit_marks(right, ~0);
-
-	strbuf_release(&sb);
 }
 
 void set_config_fetch_recurse_submodules(int value)
-- 
2.10.0.rc0.217.g609f9e8.dirty

[PATCH v7 7/7] diff: teach diff to display submodule difference with an inline diff

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Teach git-diff and friends a new format for displaying the difference
of a submodule. The new format is an inline diff of the contents of the
submodule between the commit range of the update. This allows the user
to see the actual code change caused by a submodule update.

Add tests for the new format and option.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/diff-config.txt                |   9 +-
 Documentation/diff-options.txt               |  17 +-
 diff.c                                       |  31 +-
 diff.h                                       |   1 +
 submodule.c                                  |  61 +++
 submodule.h                                  |   6 +
 t/t4059-diff-submodule-option-diff-format.sh | 733 +++++++++++++++++++++++++++
 7 files changed, 838 insertions(+), 20 deletions(-)
 create mode 100755 t/t4059-diff-submodule-option-diff-format.sh

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index d5a5b17d5088..0eded24034b5 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -122,10 +122,11 @@ diff.suppressBlankEmpty::
 
 diff.submodule::
 	Specify the format in which differences in submodules are
-	shown.  The "log" format lists the commits in the range like
-	linkgit:git-submodule[1] `summary` does.  The "short" format
-	format just shows the names of the commits at the beginning
-	and end of the range.  Defaults to short.
+	shown.  The "short" format just shows the names of the commits
+	at the beginning and end of the range. The "log" format lists
+	the commits in the range like linkgit:git-submodule[1] `summary`
+	does. The "diff" format shows an inline diff of the changed
+	contents of the submodule. Defaults to "short".
 
 diff.wordRegex::
 	A POSIX Extended Regular Expression used to determine what is a "word"
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index cc4342e2034d..7805a0ccadf2 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -210,13 +210,16 @@ any of those replacements occurred.
 	of the `--diff-filter` option on what the status letters mean.
 
 --submodule[=<format>]::
-	Specify how differences in submodules are shown.  When `--submodule`
-	or `--submodule=log` is given, the 'log' format is used.  This format lists
-	the commits in the range like linkgit:git-submodule[1] `summary` does.
-	Omitting the `--submodule` option or specifying `--submodule=short`,
-	uses the 'short' format. This format just shows the names of the commits
-	at the beginning and end of the range.  Can be tweaked via the
-	`diff.submodule` configuration variable.
+	Specify how differences in submodules are shown.  When specifying
+	`--submodule=short` the 'short' format is used.  This format just
+	shows the names of the commits at the beginning and end of the range.
+	When `--submodule` or `--submodule=log` is specified, the 'log'
+	format is used.  This format lists the commits in the range like
+	linkgit:git-submodule[1] `summary` does.  When `--submodule=diff`
+	is specified, the 'diff' format is used.  This format shows an
+	inline diff of the changes in the submodule contents between the
+	commit range.  Defaults to `diff.submodule` or the 'short' format
+	if the config option is unset.
 
 --color[=<when>]::
 	Show colored diff.
diff --git a/diff.c b/diff.c
index d6b321da3d1d..e119758aba82 100644
--- a/diff.c
+++ b/diff.c
@@ -135,6 +135,8 @@ static int parse_submodule_params(struct diff_options *options, const char *valu
 		options->submodule_format = DIFF_SUBMODULE_LOG;
 	else if (!strcmp(value, "short"))
 		options->submodule_format = DIFF_SUBMODULE_SHORT;
+	else if (!strcmp(value, "diff"))
+		options->submodule_format = DIFF_SUBMODULE_INLINE_DIFF;
 	else
 		return -1;
 	return 0;
@@ -2300,6 +2302,15 @@ static void builtin_diff(const char *name_a,
 	struct strbuf header = STRBUF_INIT;
 	const char *line_prefix = diff_line_prefix(o);
 
+	diff_set_mnemonic_prefix(o, "a/", "b/");
+	if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
+		a_prefix = o->b_prefix;
+		b_prefix = o->a_prefix;
+	} else {
+		a_prefix = o->a_prefix;
+		b_prefix = o->b_prefix;
+	}
+
 	if (o->submodule_format == DIFF_SUBMODULE_LOG &&
 	    (!one->mode || S_ISGITLINK(one->mode)) &&
 	    (!two->mode || S_ISGITLINK(two->mode))) {
@@ -2311,6 +2322,17 @@ static void builtin_diff(const char *name_a,
 				two->dirty_submodule,
 				meta, del, add, reset);
 		return;
+	} else if (o->submodule_format == DIFF_SUBMODULE_INLINE_DIFF &&
+		   (!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_inline_diff(o->file, one->path ? one->path : two->path,
+				line_prefix,
+				one->oid.hash, two->oid.hash,
+				two->dirty_submodule,
+				meta, del, add, reset, o);
+		return;
 	}
 
 	if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
@@ -2318,15 +2340,6 @@ static void builtin_diff(const char *name_a,
 		textconv_two = get_textconv(two);
 	}
 
-	diff_set_mnemonic_prefix(o, "a/", "b/");
-	if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
-		a_prefix = o->b_prefix;
-		b_prefix = o->a_prefix;
-	} else {
-		a_prefix = o->a_prefix;
-		b_prefix = o->b_prefix;
-	}
-
 	/* Never use a non-valid filename anywhere if at all possible */
 	name_a = DIFF_FILE_VALID(one) ? name_a : name_b;
 	name_b = DIFF_FILE_VALID(two) ? name_b : name_a;
diff --git a/diff.h b/diff.h
index ea5aba668eaa..192c0eedd0ff 100644
--- a/diff.h
+++ b/diff.h
@@ -112,6 +112,7 @@ enum diff_words_type {
 enum diff_submodule_format {
 	DIFF_SUBMODULE_SHORT = 0,
 	DIFF_SUBMODULE_LOG,
+	DIFF_SUBMODULE_INLINE_DIFF,
 };
 
 struct diff_options {
diff --git a/submodule.c b/submodule.c
index e341ca7ffefd..e5f1138f4362 100644
--- a/submodule.c
+++ b/submodule.c
@@ -436,6 +436,67 @@ void show_submodule_summary(FILE *f, const char *path,
 		clear_commit_marks(right, ~0);
 }
 
+void show_submodule_inline_diff(FILE *f, const char *path,
+		const char *line_prefix,
+		unsigned char one[20], unsigned char two[20],
+		unsigned dirty_submodule, const char *meta,
+		const char *del, const char *add, const char *reset,
+		const struct diff_options *o)
+{
+	const char *old = EMPTY_TREE_SHA1_BIN, *new = EMPTY_TREE_SHA1_BIN;
+	struct commit *left = NULL, *right = NULL;
+	struct strbuf submodule_dir = STRBUF_INIT;
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	show_submodule_header(f, path, line_prefix, one, two, dirty_submodule,
+			      meta, reset, &left, &right);
+
+	/* We need a valid left and right commit to display a difference */
+	if (!(left || is_null_sha1(one)) ||
+	    !(right || is_null_sha1(two)))
+		goto done;
+
+	if (left)
+		old = one;
+	if (right)
+		new = two;
+
+	fflush(f);
+	cp.git_cmd = 1;
+	cp.dir = path;
+	cp.out = dup(fileno(f));
+	cp.no_stdin = 1;
+
+	/* TODO: other options may need to be passed here. */
+	argv_array_pushl(&cp.args, "diff");
+	argv_array_pushf(&cp.args, "--line-prefix=%s", line_prefix);
+	if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
+		argv_array_pushf(&cp.args, "--src-prefix=%s%s/",
+				 o->b_prefix, path);
+		argv_array_pushf(&cp.args, "--dst-prefix=%s%s/",
+				 o->a_prefix, path);
+	} else {
+		argv_array_pushf(&cp.args, "--src-prefix=%s%s/",
+				 o->a_prefix, path);
+		argv_array_pushf(&cp.args, "--dst-prefix=%s%s/",
+				 o->b_prefix, path);
+	}
+	argv_array_push(&cp.args, sha1_to_hex(old));
+	/* If the submodule has modified content, we will diff against the
+	 * work tree, under the assumption that the user has asked for the
+	 * diff format and wishes to actually see all differences even if they
+	 * haven't yet been committed to the submodule yet.
+	 */
+	if (!(dirty_submodule & DIRTY_SUBMODULE_MODIFIED))
+		argv_array_push(&cp.args, sha1_to_hex(new));
+
+	if (run_command(&cp))
+		fprintf(f, "(diff failed)\n");
+
+done:
+	strbuf_release(&submodule_dir);
+}
+
 void set_config_fetch_recurse_submodules(int value)
 {
 	config_fetch_recurse_submodules = value;
diff --git a/submodule.h b/submodule.h
index 2af939099819..e2ebc0756f6a 100644
--- a/submodule.h
+++ b/submodule.h
@@ -46,6 +46,12 @@ void show_submodule_summary(FILE *f, const char *path,
 		unsigned char one[20], unsigned char two[20],
 		unsigned dirty_submodule, const char *meta,
 		const char *del, const char *add, const char *reset);
+void show_submodule_inline_diff(FILE *f, const char *path,
+		const char *line_prefix,
+		unsigned char one[20], unsigned char two[20],
+		unsigned dirty_submodule, const char *meta,
+		const char *del, const char *add, const char *reset,
+		const struct diff_options *opt);
 void set_config_fetch_recurse_submodules(int value);
 void check_for_new_submodule_commits(unsigned char new_sha1[20]);
 int fetch_populated_submodules(const struct argv_array *options,
diff --git a/t/t4059-diff-submodule-option-diff-format.sh b/t/t4059-diff-submodule-option-diff-format.sh
new file mode 100755
index 000000000000..88484f1ba847
--- /dev/null
+++ b/t/t4059-diff-submodule-option-diff-format.sh
@@ -0,0 +1,733 @@
+#!/bin/sh
+#
+# Copyright (c) 2009 Jens Lehmann, based on t7401 by Ping Yin
+# Copyright (c) 2011 Alexey Shumkin (+ non-UTF-8 commit encoding tests)
+# Copyright (c) 2016 Jacob Keller (copy + convert to --submodule=diff)
+#
+
+test_description='Support for diff format verbose submodule difference in git diff
+
+This test tries to verify the sanity of --submodule=diff option of git diff.
+'
+
+. ./test-lib.sh
+
+# Tested non-UTF-8 encoding
+test_encoding="ISO8859-1"
+
+# String "added" in German (translated with Google Translate), encoded in UTF-8,
+# used in sample commit log messages in add_file() function below.
+added=$(printf "hinzugef\303\274gt")
+add_file () {
+	(
+		cd "$1" &&
+		shift &&
+		for name
+		do
+			echo "$name" >"$name" &&
+			git add "$name" &&
+			test_tick &&
+			# "git commit -m" would break MinGW, as Windows refuse to pass
+			# $test_encoding encoded parameter to git.
+			echo "Add $name ($added $name)" | iconv -f utf-8 -t $test_encoding |
+			git -c "i18n.commitEncoding=$test_encoding" commit -F -
+		done >/dev/null &&
+		git rev-parse --short --verify HEAD
+	)
+}
+commit_file () {
+	test_tick &&
+	git commit "$@" -m "Commit $*" >/dev/null
+}
+
+test_create_repo sm1 &&
+add_file . foo >/dev/null
+
+head1=$(add_file sm1 foo1 foo2)
+fullhead1=$(cd sm1; git rev-parse --verify HEAD)
+
+test_expect_success 'added submodule' '
+	git add sm1 &&
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 0000000...1beffeb (new submodule)
+	diff --git a/sm1/foo1 b/sm1/foo1
+	new file mode 100644
+	index 0000000..1715acd
+	--- /dev/null
+	+++ b/sm1/foo1
+	@@ -0,0 +1 @@
+	+foo1
+	diff --git a/sm1/foo2 b/sm1/foo2
+	new file mode 100644
+	index 0000000..54b060e
+	--- /dev/null
+	+++ b/sm1/foo2
+	@@ -0,0 +1 @@
+	+foo2
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'added submodule, set diff.submodule' '
+	git config diff.submodule log &&
+	git add sm1 &&
+	git diff --cached >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 0000000...$head1 (new submodule)
+	EOF
+	git config --unset diff.submodule &&
+	test_cmp expected actual
+'
+
+test_expect_success '--submodule=short overrides diff.submodule' '
+	test_config diff.submodule log &&
+	git add sm1 &&
+	git diff --submodule=short --cached >actual &&
+	cat >expected <<-EOF &&
+	diff --git a/sm1 b/sm1
+	new file mode 160000
+	index 0000000..$head1
+	--- /dev/null
+	+++ b/sm1
+	@@ -0,0 +1 @@
+	+Subproject commit $fullhead1
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'diff.submodule does not affect plumbing' '
+	test_config diff.submodule log &&
+	git diff-index -p HEAD >actual &&
+	cat >expected <<-EOF &&
+	diff --git a/sm1 b/sm1
+	new file mode 160000
+	index 0000000..$head1
+	--- /dev/null
+	+++ b/sm1
+	@@ -0,0 +1 @@
+	+Subproject commit $fullhead1
+	EOF
+	test_cmp expected actual
+'
+
+commit_file sm1 &&
+head2=$(add_file sm1 foo3)
+
+test_expect_success 'modified submodule(forward)' '
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 1beffeb..30b9670:
+	diff --git a/sm1/foo3 b/sm1/foo3
+	new file mode 100644
+	index 0000000..c1ec6c6
+	--- /dev/null
+	+++ b/sm1/foo3
+	@@ -0,0 +1 @@
+	+foo3
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'modified submodule(forward)' '
+	git diff --submodule=diff >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 1beffeb..30b9670:
+	diff --git a/sm1/foo3 b/sm1/foo3
+	new file mode 100644
+	index 0000000..c1ec6c6
+	--- /dev/null
+	+++ b/sm1/foo3
+	@@ -0,0 +1 @@
+	+foo3
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'modified submodule(forward) --submodule' '
+	git diff --submodule >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 $head1..$head2:
+	  > Add foo3 ($added foo3)
+	EOF
+	test_cmp expected actual
+'
+
+fullhead2=$(cd sm1; git rev-parse --verify HEAD)
+test_expect_success 'modified submodule(forward) --submodule=short' '
+	git diff --submodule=short >actual &&
+	cat >expected <<-EOF &&
+	diff --git a/sm1 b/sm1
+	index $head1..$head2 160000
+	--- a/sm1
+	+++ b/sm1
+	@@ -1 +1 @@
+	-Subproject commit $fullhead1
+	+Subproject commit $fullhead2
+	EOF
+	test_cmp expected actual
+'
+
+commit_file sm1 &&
+head3=$(
+	cd sm1 &&
+	git reset --hard HEAD~2 >/dev/null &&
+	git rev-parse --short --verify HEAD
+)
+
+test_expect_success 'modified submodule(backward)' '
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 30b9670..dafb207 (rewind):
+	diff --git a/sm1/foo2 b/sm1/foo2
+	deleted file mode 100644
+	index 54b060e..0000000
+	--- a/sm1/foo2
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-foo2
+	diff --git a/sm1/foo3 b/sm1/foo3
+	deleted file mode 100644
+	index c1ec6c6..0000000
+	--- a/sm1/foo3
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-foo3
+	EOF
+	test_cmp expected actual
+'
+
+head4=$(add_file sm1 foo4 foo5)
+test_expect_success 'modified submodule(backward and forward)' '
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 30b9670...d176589:
+	diff --git a/sm1/foo2 b/sm1/foo2
+	deleted file mode 100644
+	index 54b060e..0000000
+	--- a/sm1/foo2
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-foo2
+	diff --git a/sm1/foo3 b/sm1/foo3
+	deleted file mode 100644
+	index c1ec6c6..0000000
+	--- a/sm1/foo3
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-foo3
+	diff --git a/sm1/foo4 b/sm1/foo4
+	new file mode 100644
+	index 0000000..a0016db
+	--- /dev/null
+	+++ b/sm1/foo4
+	@@ -0,0 +1 @@
+	+foo4
+	diff --git a/sm1/foo5 b/sm1/foo5
+	new file mode 100644
+	index 0000000..d6f2413
+	--- /dev/null
+	+++ b/sm1/foo5
+	@@ -0,0 +1 @@
+	+foo5
+	EOF
+	test_cmp expected actual
+'
+
+commit_file sm1 &&
+mv sm1 sm1-bak &&
+echo sm1 >sm1 &&
+head5=$(git hash-object sm1 | cut -c1-7) &&
+git add sm1 &&
+rm -f sm1 &&
+mv sm1-bak sm1
+
+test_expect_success 'typechanged submodule(submodule->blob), --cached' '
+	git diff --submodule=diff --cached >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 d176589...0000000 (submodule deleted)
+	diff --git a/sm1/foo1 b/sm1/foo1
+	deleted file mode 100644
+	index 1715acd..0000000
+	--- a/sm1/foo1
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-foo1
+	diff --git a/sm1/foo4 b/sm1/foo4
+	deleted file mode 100644
+	index a0016db..0000000
+	--- a/sm1/foo4
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-foo4
+	diff --git a/sm1/foo5 b/sm1/foo5
+	deleted file mode 100644
+	index d6f2413..0000000
+	--- a/sm1/foo5
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-foo5
+	diff --git a/sm1 b/sm1
+	new file mode 100644
+	index 0000000..9da5fb8
+	--- /dev/null
+	+++ b/sm1
+	@@ -0,0 +1 @@
+	+sm1
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'typechanged submodule(submodule->blob)' '
+	git diff --submodule=diff >actual &&
+	cat >expected <<-EOF &&
+	diff --git a/sm1 b/sm1
+	deleted file mode 100644
+	index 9da5fb8..0000000
+	--- a/sm1
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-sm1
+	Submodule sm1 0000000...d176589 (new submodule)
+	diff --git a/sm1/foo1 b/sm1/foo1
+	new file mode 100644
+	index 0000000..1715acd
+	--- /dev/null
+	+++ b/sm1/foo1
+	@@ -0,0 +1 @@
+	+foo1
+	diff --git a/sm1/foo4 b/sm1/foo4
+	new file mode 100644
+	index 0000000..a0016db
+	--- /dev/null
+	+++ b/sm1/foo4
+	@@ -0,0 +1 @@
+	+foo4
+	diff --git a/sm1/foo5 b/sm1/foo5
+	new file mode 100644
+	index 0000000..d6f2413
+	--- /dev/null
+	+++ b/sm1/foo5
+	@@ -0,0 +1 @@
+	+foo5
+	EOF
+	test_cmp expected actual
+'
+
+rm -rf sm1 &&
+git checkout-index sm1
+test_expect_success 'typechanged submodule(submodule->blob)' '
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 $head4...0000000 (submodule deleted)
+	diff --git a/sm1 b/sm1
+	new file mode 100644
+	index 0000000..$head5
+	--- /dev/null
+	+++ b/sm1
+	@@ -0,0 +1 @@
+	+sm1
+	EOF
+	test_cmp expected actual
+'
+
+rm -f sm1 &&
+test_create_repo sm1 &&
+head6=$(add_file sm1 foo6 foo7)
+fullhead6=$(cd sm1; git rev-parse --verify HEAD)
+test_expect_success 'nonexistent commit' '
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 d176589...17243c9 (commits not present)
+	EOF
+	test_cmp expected actual
+'
+
+commit_file
+test_expect_success 'typechanged submodule(blob->submodule)' '
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	diff --git a/sm1 b/sm1
+	deleted file mode 100644
+	index 9da5fb8..0000000
+	--- a/sm1
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-sm1
+	Submodule sm1 0000000...17243c9 (new submodule)
+	diff --git a/sm1/foo6 b/sm1/foo6
+	new file mode 100644
+	index 0000000..462398b
+	--- /dev/null
+	+++ b/sm1/foo6
+	@@ -0,0 +1 @@
+	+foo6
+	diff --git a/sm1/foo7 b/sm1/foo7
+	new file mode 100644
+	index 0000000..6e9262c
+	--- /dev/null
+	+++ b/sm1/foo7
+	@@ -0,0 +1 @@
+	+foo7
+	EOF
+	test_cmp expected actual
+'
+
+commit_file sm1 &&
+test_expect_success 'submodule is up to date' '
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'submodule contains untracked content' '
+	echo new > sm1/new-file &&
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 contains untracked content
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'submodule contains untracked content (untracked ignored)' '
+	git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+	! test -s actual
+'
+
+test_expect_success 'submodule contains untracked content (dirty ignored)' '
+	git diff-index -p --ignore-submodules=dirty --submodule=diff HEAD >actual &&
+	! test -s actual
+'
+
+test_expect_success 'submodule contains untracked content (all ignored)' '
+	git diff-index -p --ignore-submodules=all --submodule=diff HEAD >actual &&
+	! test -s actual
+'
+
+test_expect_success 'submodule contains untracked and modified content' '
+	echo new > sm1/foo6 &&
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 contains untracked content
+	Submodule sm1 contains modified content
+	diff --git a/sm1/foo6 b/sm1/foo6
+	index 462398b..3e75765 100644
+	--- a/sm1/foo6
+	+++ b/sm1/foo6
+	@@ -1 +1 @@
+	-foo6
+	+new
+	EOF
+	test_cmp expected actual
+'
+
+# NOT OK
+test_expect_success 'submodule contains untracked and modified content (untracked ignored)' '
+	echo new > sm1/foo6 &&
+	git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 contains modified content
+	diff --git a/sm1/foo6 b/sm1/foo6
+	index 462398b..3e75765 100644
+	--- a/sm1/foo6
+	+++ b/sm1/foo6
+	@@ -1 +1 @@
+	-foo6
+	+new
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'submodule contains untracked and modified content (dirty ignored)' '
+	echo new > sm1/foo6 &&
+	git diff-index -p --ignore-submodules=dirty --submodule=diff HEAD >actual &&
+	! test -s actual
+'
+
+test_expect_success 'submodule contains untracked and modified content (all ignored)' '
+	echo new > sm1/foo6 &&
+	git diff-index -p --ignore-submodules --submodule=diff HEAD >actual &&
+	! test -s actual
+'
+
+test_expect_success 'submodule contains modified content' '
+	rm -f sm1/new-file &&
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 contains modified content
+	diff --git a/sm1/foo6 b/sm1/foo6
+	index 462398b..3e75765 100644
+	--- a/sm1/foo6
+	+++ b/sm1/foo6
+	@@ -1 +1 @@
+	-foo6
+	+new
+	EOF
+	test_cmp expected actual
+'
+
+(cd sm1; git commit -mchange foo6 >/dev/null) &&
+head8=$(cd sm1; git rev-parse --short --verify HEAD) &&
+test_expect_success 'submodule is modified' '
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 17243c9..cfce562:
+	diff --git a/sm1/foo6 b/sm1/foo6
+	index 462398b..3e75765 100644
+	--- a/sm1/foo6
+	+++ b/sm1/foo6
+	@@ -1 +1 @@
+	-foo6
+	+new
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'modified submodule contains untracked content' '
+	echo new > sm1/new-file &&
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 contains untracked content
+	Submodule sm1 17243c9..cfce562:
+	diff --git a/sm1/foo6 b/sm1/foo6
+	index 462398b..3e75765 100644
+	--- a/sm1/foo6
+	+++ b/sm1/foo6
+	@@ -1 +1 @@
+	-foo6
+	+new
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'modified submodule contains untracked content (untracked ignored)' '
+	git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 17243c9..cfce562:
+	diff --git a/sm1/foo6 b/sm1/foo6
+	index 462398b..3e75765 100644
+	--- a/sm1/foo6
+	+++ b/sm1/foo6
+	@@ -1 +1 @@
+	-foo6
+	+new
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'modified submodule contains untracked content (dirty ignored)' '
+	git diff-index -p --ignore-submodules=dirty --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 17243c9..cfce562:
+	diff --git a/sm1/foo6 b/sm1/foo6
+	index 462398b..3e75765 100644
+	--- a/sm1/foo6
+	+++ b/sm1/foo6
+	@@ -1 +1 @@
+	-foo6
+	+new
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'modified submodule contains untracked content (all ignored)' '
+	git diff-index -p --ignore-submodules=all --submodule=diff HEAD >actual &&
+	! test -s actual
+'
+
+test_expect_success 'modified submodule contains untracked and modified content' '
+	echo modification >> sm1/foo6 &&
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 contains untracked content
+	Submodule sm1 contains modified content
+	Submodule sm1 17243c9..cfce562:
+	diff --git a/sm1/foo6 b/sm1/foo6
+	index 462398b..dfda541 100644
+	--- a/sm1/foo6
+	+++ b/sm1/foo6
+	@@ -1 +1,2 @@
+	-foo6
+	+new
+	+modification
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'modified submodule contains untracked and modified content (untracked ignored)' '
+	echo modification >> sm1/foo6 &&
+	git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 contains modified content
+	Submodule sm1 17243c9..cfce562:
+	diff --git a/sm1/foo6 b/sm1/foo6
+	index 462398b..e20e2d9 100644
+	--- a/sm1/foo6
+	+++ b/sm1/foo6
+	@@ -1 +1,3 @@
+	-foo6
+	+new
+	+modification
+	+modification
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'modified submodule contains untracked and modified content (dirty ignored)' '
+	echo modification >> sm1/foo6 &&
+	git diff-index -p --ignore-submodules=dirty --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 17243c9..cfce562:
+	diff --git a/sm1/foo6 b/sm1/foo6
+	index 462398b..3e75765 100644
+	--- a/sm1/foo6
+	+++ b/sm1/foo6
+	@@ -1 +1 @@
+	-foo6
+	+new
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'modified submodule contains untracked and modified content (all ignored)' '
+	echo modification >> sm1/foo6 &&
+	git diff-index -p --ignore-submodules --submodule=diff HEAD >actual &&
+	! test -s actual
+'
+
+# NOT OK
+test_expect_success 'modified submodule contains modified content' '
+	rm -f sm1/new-file &&
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 contains modified content
+	Submodule sm1 17243c9..cfce562:
+	diff --git a/sm1/foo6 b/sm1/foo6
+	index 462398b..ac466ca 100644
+	--- a/sm1/foo6
+	+++ b/sm1/foo6
+	@@ -1 +1,5 @@
+	-foo6
+	+new
+	+modification
+	+modification
+	+modification
+	+modification
+	EOF
+	test_cmp expected actual
+'
+
+rm -rf sm1
+test_expect_success 'deleted submodule' '
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 17243c9...0000000 (submodule deleted)
+	EOF
+	test_cmp expected actual
+'
+
+test_create_repo sm2 &&
+head7=$(add_file sm2 foo8 foo9) &&
+git add sm2
+
+test_expect_success 'multiple submodules' '
+	git diff-index -p --submodule=diff HEAD >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 17243c9...0000000 (submodule deleted)
+	Submodule sm2 0000000...a5a65c9 (new submodule)
+	diff --git a/sm2/foo8 b/sm2/foo8
+	new file mode 100644
+	index 0000000..db9916b
+	--- /dev/null
+	+++ b/sm2/foo8
+	@@ -0,0 +1 @@
+	+foo8
+	diff --git a/sm2/foo9 b/sm2/foo9
+	new file mode 100644
+	index 0000000..9c3b4f6
+	--- /dev/null
+	+++ b/sm2/foo9
+	@@ -0,0 +1 @@
+	+foo9
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'path filter' '
+	git diff-index -p --submodule=diff HEAD sm2 >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm2 0000000...a5a65c9 (new submodule)
+	diff --git a/sm2/foo8 b/sm2/foo8
+	new file mode 100644
+	index 0000000..db9916b
+	--- /dev/null
+	+++ b/sm2/foo8
+	@@ -0,0 +1 @@
+	+foo8
+	diff --git a/sm2/foo9 b/sm2/foo9
+	new file mode 100644
+	index 0000000..9c3b4f6
+	--- /dev/null
+	+++ b/sm2/foo9
+	@@ -0,0 +1 @@
+	+foo9
+	EOF
+	test_cmp expected actual
+'
+
+commit_file sm2
+test_expect_success 'given commit' '
+	git diff-index -p --submodule=diff HEAD^ >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 17243c9...0000000 (submodule deleted)
+	Submodule sm2 0000000...a5a65c9 (new submodule)
+	diff --git a/sm2/foo8 b/sm2/foo8
+	new file mode 100644
+	index 0000000..db9916b
+	--- /dev/null
+	+++ b/sm2/foo8
+	@@ -0,0 +1 @@
+	+foo8
+	diff --git a/sm2/foo9 b/sm2/foo9
+	new file mode 100644
+	index 0000000..9c3b4f6
+	--- /dev/null
+	+++ b/sm2/foo9
+	@@ -0,0 +1 @@
+	+foo9
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'setup .git file for sm2' '
+	(cd sm2 &&
+	 REAL="$(pwd)/../.real" &&
+	 mv .git "$REAL"
+	 echo "gitdir: $REAL" >.git)
+'
+
+test_expect_success 'diff --submodule=diff with .git file' '
+	git diff --submodule=diff HEAD^ >actual &&
+	cat >expected <<-EOF &&
+	Submodule sm1 17243c9...0000000 (submodule deleted)
+	Submodule sm2 0000000...a5a65c9 (new submodule)
+	diff --git a/sm2/foo8 b/sm2/foo8
+	new file mode 100644
+	index 0000000..db9916b
+	--- /dev/null
+	+++ b/sm2/foo8
+	@@ -0,0 +1 @@
+	+foo8
+	diff --git a/sm2/foo9 b/sm2/foo9
+	new file mode 100644
+	index 0000000..9c3b4f6
+	--- /dev/null
+	+++ b/sm2/foo9
+	@@ -0,0 +1 @@
+	+foo9
+	EOF
+	test_cmp expected actual
+'
+
+test_done
-- 
2.10.0.rc0.217.g609f9e8.dirty

Re: [PATCH v7 6/7] submodule: refactor show_submodule_summary with helper function

[David Aguilar]

On Wed, Aug 17, 2016 at 05:51:30PM -0700, Jacob Keller wrote:
> [snip]
> @@ -333,31 +326,23 @@ static void print_submodule_summary(struct rev_info *rev, FILE *f,
>  	strbuf_release(&sb);
>  }
>  
> -void show_submodule_summary(FILE *f, const char *path,
> +/* Helper function to display the submodule header line prior to the full
> + * summary output. If it can locate the submodule objects directory it will
> + * attempt to lookup both the left and right commits and put them into the
> + * left and right pointers.
> + */
> +static void show_submodule_header(FILE *f, const char *path,
>  		const char *line_prefix,
>  		unsigned char one[20], unsigned char two[20],
>  		unsigned dirty_submodule, const char *meta,
> -		const char *del, const char *add, const char *reset)
> +		const char *reset,
> +		struct commit **left, struct commit **right)


Note the pre-existing unsigned char[20]s in the signature above...


> [snip]
> @@ -381,14 +401,39 @@ void show_submodule_summary(FILE *f, const char *path,
>  		strbuf_addf(&sb, "%s:%s\n", fast_backward ? " (rewind)" : "", reset);
>  	fwrite(sb.buf, sb.len, 1, f);
>  
> -	if (!message) /* only NULL if we succeeded in setting up the walk */
> -		print_submodule_summary(&rev, f, line_prefix, del, add, reset);
> +	strbuf_release(&sb);
> +}
> +
> +void show_submodule_summary(FILE *f, const char *path,
> +		const char *line_prefix,
> +		unsigned char one[20], unsigned char two[20],
> +		unsigned dirty_submodule, const char *meta,
> +		const char *del, const char *add, const char *reset)


... and here too.

There's an ongoing effort to replace unsigned char sha1[20]
with struct object_id.  It might make sense to pass "one" and
"two" as "unsigned char*" instead of hard-coding the SHA1-length
here.  Alternatively, we could pass in the struct object_id*
instead.

The [20] in the show_submodule_header() function above is
pre-existing and not added by this patch, but that function's
signature is touched by this patch so it might make sense to
tidy that up while we're in here, possibly as a separate patch.
-- 
David

Re: [PATCH v7 6/7] submodule: refactor show_submodule_summary with helper function

[Jacob Keller]

On Thu, Aug 18, 2016 at 12:00 AM, David Aguilar <davvid@gmail.com> wrote:
>> +void show_submodule_summary(FILE *f, const char *path,
>> +             const char *line_prefix,
>> +             unsigned char one[20], unsigned char two[20],
>> +             unsigned dirty_submodule, const char *meta,
>> +             const char *del, const char *add, const char *reset)
>
>
> ... and here too.
>
> There's an ongoing effort to replace unsigned char sha1[20]
> with struct object_id.  It might make sense to pass "one" and
> "two" as "unsigned char*" instead of hard-coding the SHA1-length
> here.  Alternatively, we could pass in the struct object_id*
> instead.
>
> The [20] in the show_submodule_header() function above is
> pre-existing and not added by this patch, but that function's
> signature is touched by this patch so it might make sense to
> tidy that up while we're in here, possibly as a separate patch.
> --

I'll add a patch to the series that does this inbetween these commits.

Thanks,
Jake

Re: [PATCH v7 2/7] graph: add support for --line-prefix on all graph-aware output

[Stefan Beller]

On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> Add an extension to git-diff and git-log (and any other graph-aware
> displayable output) such that "--line-prefix=<string>" will print the
> additional line-prefix on every line of output.
>
> To make this work, we have to fix a few bugs in the graph API that force
> graph_show_commit_msg to be used only when you have a valid graph.
> Additionally, we extend the default_diff_output_prefix handler to work
> even when no graph is enabled.
>
> This is somewhat of a hack on top of the graph API, but I think it
> should be acceptable here.
>
> This will be used by a future extension of submodule display which
> displays the submodule diff as the actual diff between the pre and post
> commit in the submodule project.
>
> Add some tests for both git-log and git-diff to ensure that the prefix
> is honored correctly.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
>  Documentation/diff-options.txt                     |   3 +
>  builtin/rev-list.c                                 |  70 ++---
>  diff.c                                             |   7 +
>  diff.h                                             |   2 +
>  graph.c                                            | 104 ++++---
>  graph.h                                            |  22 +-
>  log-tree.c                                         |   5 +-
>  t/t4013-diff-various.sh                            |   6 +
>  ...diff.diff_--line-prefix=abc_master_master^_side |  29 ++
>  t/t4013/diff.diff_--line-prefix_--cached_--_file0  |  15 +
>  t/t4202-log.sh                                     | 323 +++++++++++++++++++++
>  11 files changed, 506 insertions(+), 80 deletions(-)
>  create mode 100644 t/t4013/diff.diff_--line-prefix=abc_master_master^_side
>  create mode 100644 t/t4013/diff.diff_--line-prefix_--cached_--_file0
>
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 705a87394200..cc4342e2034d 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -569,5 +569,8 @@ endif::git-format-patch[]
>  --no-prefix::
>         Do not show any source or destination prefix.
>
> +--line-prefix=<prefix>::
> +       Prepend an additional prefix to every line of output.
> +
>  For more detailed explanation on these common options, see also
>  linkgit:gitdiffcore[7].
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index 0ba82b1635b6..1a75a83538f4 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -122,48 +122,40 @@ static void show_commit(struct commit *commit, void *data)
>                 ctx.fmt = revs->commit_format;
>                 ctx.output_encoding = get_log_output_encoding();
>                 pretty_print_commit(&ctx, commit, &buf);
> -               if (revs->graph) {
> -                       if (buf.len) {
> -                               if (revs->commit_format != CMIT_FMT_ONELINE)
> -                                       graph_show_oneline(revs->graph);
> +               if (buf.len) {
> +                       if (revs->commit_format != CMIT_FMT_ONELINE)
> +                               graph_show_oneline(revs->graph);
>
> -                               graph_show_commit_msg(revs->graph, &buf);
> +                       graph_show_commit_msg(revs->graph, stdout, &buf);
>
> -                               /*
> -                                * Add a newline after the commit message.
> -                                *
> -                                * Usually, this newline produces a blank
> -                                * padding line between entries, in which case
> -                                * we need to add graph padding on this line.
> -                                *
> -                                * However, the commit message may not end in a
> -                                * newline.  In this case the newline simply
> -                                * ends the last line of the commit message,
> -                                * and we don't need any graph output.  (This
> -                                * always happens with CMIT_FMT_ONELINE, and it
> -                                * happens with CMIT_FMT_USERFORMAT when the
> -                                * format doesn't explicitly end in a newline.)
> -                                */
> -                               if (buf.len && buf.buf[buf.len - 1] == '\n')
> -                                       graph_show_padding(revs->graph);
> -                               putchar('\n');
> -                       } else {
> -                               /*
> -                                * If the message buffer is empty, just show
> -                                * the rest of the graph output for this
> -                                * commit.
> -                                */
> -                               if (graph_show_remainder(revs->graph))
> -                                       putchar('\n');
> -                               if (revs->commit_format == CMIT_FMT_ONELINE)
> -                                       putchar('\n');
> -                       }
> +                       /*
> +                               * Add a newline after the commit message.

I wondered if it is my webmailer displaying things wrongly here so I checked
it at public inbox, and fetched the mails and applied them.

It seems to me as if this long comment is broken in indentation
(i.e. you removed a blank ' ' directly in front of the '*' instead of a '\t' ?)


> +                               *
> +                               * Usually, this newline produces a blank
> +                               * padding line between entries, in which case
> +                               * we need to add graph padding on this line.
> +                               *
> +                               * However, the commit message may not end in a
> +                               * newline.  In this case the newline simply
> +                               * ends the last line of the commit message,
> +                               * and we don't need any graph output.  (This
> +                               * always happens with CMIT_FMT_ONELINE, and it
> +                               * happens with CMIT_FMT_USERFORMAT when the
> +                               * format doesn't explicitly end in a newline.)
> +                               */

> - if (!graph)
> + if (!graph) {
> +         graph_show_line_prefix(default_diffopt);
>           return;
> + }

> -       if (graph_is_commit_finished(graph))
> +       if (graph_is_commit_finished(graph)) {
> +               graph_show_line_prefix(&graph->revs->diffopt);
>                 return 0;
> +       }

This seems to be a reoccurring pattern, i.e. we need to add a lot of
one off instructions before a return. Is it possible to make the call
earlier instead
of just before the returns? (or later for that matter) ?


Thanks,
Stefan

Re: [PATCH v7 3/7] diff: prepare for additional submodule formats

[Stefan Beller]

On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> A future patch will add a new format for displaying the difference of
> a submodule. Make it easier by changing how we store the current
> selected format. Replace the DIFF_OPT flag with an enumeration, as each
> format will be mutually exclusive.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---

Looks good,
Thanks
Stefan

Re: [PATCH v7 4/7] submodule: allow do_submodule_path to work if given gitdir directly

[Stefan Beller]

On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> Currently, do_submodule_path relies on read_gitfile, which will die() if
> it can't read from the specified gitfile. Unfortunately, this means that
> do_submodule_path will not work when given the path to a submodule which
> is checked out directly, such as a newly added submodule which you
> cloned and then "git submodule add". Instead, replace the call with
> resolve_gitdir. This first checks to see if we've been given a gitdir
> already.
>
> Because resolve_gitdir may return the same buffer it was passed, we have
> to check for this case as well, since strbuf_reset() will not work as
> expected here, and indeed is not necessary.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
>  path.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Looks good,
Thanks,
Stefan

Re: [PATCH v7 5/7] submodule: correct output of submodule log format

[Stefan Beller]

On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> The submodule log diff output incorrectly states that the submodule is
> "not checked out" in cases where it wants to say the submodule is "not
> initialized". Change the wording to reflect the actual check being
> performed.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
>  submodule.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/submodule.c b/submodule.c
> index 1b5cdfb7e784..e1a51b7506ff 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -348,7 +348,7 @@ void show_submodule_summary(FILE *f, const char *path,
>         if (is_null_sha1(two))
>                 message = "(submodule deleted)";
>         else if (add_submodule_odb(path))
> -               message = "(not checked out)";
> +               message = "(not initialized)";

I think "not checked" out is actually correct here.

    $ git clone https://gerrit.googlesource.com/gerrit
    $ cd gerrit
    $ git submodule update --init
      ...
    $ git diff cc82b24..5222e66 plugins/
    Submodule plugins/cookbook-plugin 2d40ee2..69b8f9f:
     > Organize imports
    $ rm -rf plugins/cookbook-plugin/
    $ git diff cc82b24..5222e66 plugins/
    Submodule plugins/cookbook-plugin 2d40ee2...69b8f9f (not checked out)
    $

Mind that by "rm -rf" of the working dir I create the "not checked out,
but initialized state and even cloned state".

So as a long term TODO:
    I guess we could teach `add_submodule_odb` to operate
    inside its git directory instead of its working directory, to have
    it working whenever we have the object database (no matter if
    checked out or not). Although this may collide with the plan of a
    different refs backend? I dunno.

add_submodule_odb is used in a variety of places:
    show_submodule_header
    submodule_needs_pushing
    push_submodule
    is_submodule_commit_present
    merge_submodule

And all of them seem to not require a checkout, but the presence of
objects is fine.

Re: [PATCH v7 2/7] graph: add support for --line-prefix on all graph-aware output

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

On Thu, Aug 18, 2016 at 10:56 AM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>> From: Jacob Keller <jacob.keller@gmail.com>
>> -                               /*
>> -                                * Add a newline after the commit message.
>> -                                *
>> -                                * Usually, this newline produces a blank
>> -                                * padding line between entries, in which case
>> -                                * we need to add graph padding on this line.
>> -                                *
>> -                                * However, the commit message may not end in a
>> -                                * newline.  In this case the newline simply
>> -                                * ends the last line of the commit message,
>> -                                * and we don't need any graph output.  (This
>> -                                * always happens with CMIT_FMT_ONELINE, and it
>> -                                * happens with CMIT_FMT_USERFORMAT when the
>> -                                * format doesn't explicitly end in a newline.)
>> -                                */
>> -                               if (buf.len && buf.buf[buf.len - 1] == '\n')
>> -                                       graph_show_padding(revs->graph);
>> -                               putchar('\n');
>> -                       } else {
>> -                               /*
>> -                                * If the message buffer is empty, just show
>> -                                * the rest of the graph output for this
>> -                                * commit.
>> -                                */
>> -                               if (graph_show_remainder(revs->graph))
>> -                                       putchar('\n');
>> -                               if (revs->commit_format == CMIT_FMT_ONELINE)
>> -                                       putchar('\n');
>> -                       }
>> +                       /*
>> +                               * Add a newline after the commit message.
>
> I wondered if it is my webmailer displaying things wrongly here so I checked
> it at public inbox, and fetched the mails and applied them.
>
> It seems to me as if this long comment is broken in indentation
> (i.e. you removed a blank ' ' directly in front of the '*' instead of a '\t' ?)
>

Yea it is indeed broken. Something like the following squash should fix it.
We could also re-flow the text if desired too, but I don't really see the
advantage.

8<-----

From 345bbaa47cc14aba7049738e99c3649e2c06748c Mon Sep 17 00:00:00 2001
From: Jacob Keller <jacob.keller@gmail.com>
Date: Thu, 18 Aug 2016 11:13:04 -0700
Subject: [PATCH] SQUASH: fix indentation of comment in builtin/rev-list.c

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 builtin/rev-list.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 1a75a83538f4..21cde8dd6b31 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -129,20 +129,20 @@ static void show_commit(struct commit *commit, void *data)
 			graph_show_commit_msg(revs->graph, stdout, &buf);
 
 			/*
-				* Add a newline after the commit message.
-				*
-				* Usually, this newline produces a blank
-				* padding line between entries, in which case
-				* we need to add graph padding on this line.
-				*
-				* However, the commit message may not end in a
-				* newline.  In this case the newline simply
-				* ends the last line of the commit message,
-				* and we don't need any graph output.  (This
-				* always happens with CMIT_FMT_ONELINE, and it
-				* happens with CMIT_FMT_USERFORMAT when the
-				* format doesn't explicitly end in a newline.)
-				*/
+			* Add a newline after the commit message.
+			*
+			* Usually, this newline produces a blank
+			* padding line between entries, in which case
+			* we need to add graph padding on this line.
+			*
+			* However, the commit message may not end in a
+			* newline.  In this case the newline simply
+			* ends the last line of the commit message,
+			* and we don't need any graph output.  (This
+			* always happens with CMIT_FMT_ONELINE, and it
+			* happens with CMIT_FMT_USERFORMAT when the
+			* format doesn't explicitly end in a newline.)
+			*/
 			if (buf.len && buf.buf[buf.len - 1] == '\n')
 				graph_show_padding(revs->graph);
 			putchar('\n');
-- 
2.10.0.rc0.217.g609f9e8.dirty

Re: [PATCH v7 2/7] graph: add support for --line-prefix on all graph-aware output

[Jacob Keller]

On Thu, Aug 18, 2016 at 10:56 AM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>> - if (!graph)
>> + if (!graph) {
>> +         graph_show_line_prefix(default_diffopt);
>>           return;
>> + }
>
>> -       if (graph_is_commit_finished(graph))
>> +       if (graph_is_commit_finished(graph)) {
>> +               graph_show_line_prefix(&graph->revs->diffopt);
>>                 return 0;
>> +       }
>
> This seems to be a reoccurring pattern, i.e. we need to add a lot of
> one off instructions before a return. Is it possible to make the call
> earlier instead
> of just before the returns? (or later for that matter) ?
>

Not sure what you mean by this...? Just move the call up above this? I
think that will work, let me give it a try.

Thanks,
Jake

>
> Thanks,
> Stefan

Re: [PATCH v7 5/7] submodule: correct output of submodule log format

[Jacob Keller]

On Thu, Aug 18, 2016 at 11:25 AM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> The submodule log diff output incorrectly states that the submodule is
>> "not checked out" in cases where it wants to say the submodule is "not
>> initialized". Change the wording to reflect the actual check being
>> performed.
>>
>> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
>> ---
>>  submodule.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/submodule.c b/submodule.c
>> index 1b5cdfb7e784..e1a51b7506ff 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -348,7 +348,7 @@ void show_submodule_summary(FILE *f, const char *path,
>>         if (is_null_sha1(two))
>>                 message = "(submodule deleted)";
>>         else if (add_submodule_odb(path))
>> -               message = "(not checked out)";
>> +               message = "(not initialized)";
>
> I think "not checked" out is actually correct here.
>

Hmmm.. It shouldn't say not checked out. I was running iinto problems
in some of my tests which indicated that the entire sub module didn't
exist. I think I had already assumed it wasn't failing when no
checkout. I think I have some fix for add_submodule_odb that can help
with this, making submodule dir behave correctly for this case.

I want add_submodule_odb to work in both of these cases:

a) we don't have a working checkout but we have the objects in the
usual location
b) we have a working directory with objects inside a .git folder but
it hasn't yet been moved into .git/modules/<>

I'll take a look and see what I can do.

>     $ git clone https://gerrit.googlesource.com/gerrit
>     $ cd gerrit
>     $ git submodule update --init
>       ...
>     $ git diff cc82b24..5222e66 plugins/
>     Submodule plugins/cookbook-plugin 2d40ee2..69b8f9f:
>      > Organize imports
>     $ rm -rf plugins/cookbook-plugin/
>     $ git diff cc82b24..5222e66 plugins/
>     Submodule plugins/cookbook-plugin 2d40ee2...69b8f9f (not checked out)
>     $
>
> Mind that by "rm -rf" of the working dir I create the "not checked out,
> but initialized state and even cloned state".
>
> So as a long term TODO:
>     I guess we could teach `add_submodule_odb` to operate
>     inside its git directory instead of its working directory, to have
>     it working whenever we have the object database (no matter if
>     checked out or not). Although this may collide with the plan of a
>     different refs backend? I dunno.
>
> add_submodule_odb is used in a variety of places:
>     show_submodule_header
>     submodule_needs_pushing
>     push_submodule
>     is_submodule_commit_present
>     merge_submodule
>
> And all of them seem to not require a checkout, but the presence of
> objects is fine.

Yea, they really only need objects, not the working tree.

Thanks,
Jake

Re: [PATCH v7 4/7] submodule: allow do_submodule_path to work if given gitdir directly

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> From: Jacob Keller <jacob.keller@gmail.com>
>
> Currently, do_submodule_path relies on read_gitfile, which will die() if
> it can't read from the specified gitfile. Unfortunately, this means that
> do_submodule_path will not work when given the path to a submodule which
> is checked out directly, such as a newly added submodule which you
> cloned and then "git submodule add". 

Hmm, are you sure about that?

A call to read_gitfile() turns into a call to read_gitfile_gently()
with the return_error_code parameter set to NULL.  The function does
a stat(2), and if the given path is not a file (e.g. we created the
submodule working tree and repository in-place ourselves, instead of
cloning an existing project from elsewhere, in which case we would
see a directory there), it says READ_GIT_FILE_ERR_NOT_A_FILE and
returns NULL, because that is not a fatal error condition.  The same
thing happens if path does not yet exist.

This caller is given <path>, prepares "<path>/.git" in buf, and
calls read_gitfile().  If it returns a non-NULL, it replaces what is
in buf and continues, but if it returns a NULL (i.e. the two cases I
mentioned in the above paragraph), it continues with "<path>/.git".

While I do not think changing it to resolve_gitdir() is wrong per-se,
I am not sure if it is necessary.

I must be misreading something in the existing code.

> Instead, replace the call with
> resolve_gitdir. This first checks to see if we've been given a gitdir
> already.
>
> Because resolve_gitdir may return the same buffer it was passed, we have
> to check for this case as well, since strbuf_reset() will not work as
> expected here, and indeed is not necessary.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
>  path.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/path.c b/path.c
> index 17551c483476..d1af029152a2 100644
> --- a/path.c
> +++ b/path.c
> @@ -477,8 +477,8 @@ static void do_submodule_path(struct strbuf *buf, const char *path,
>  	strbuf_complete(buf, '/');
>  	strbuf_addstr(buf, ".git");
>  
> -	git_dir = read_gitfile(buf->buf);
> -	if (git_dir) {
> +	git_dir = resolve_gitdir(buf->buf);
> +	if (git_dir && git_dir != buf->buf) {
>  		strbuf_reset(buf);
>  		strbuf_addstr(buf, git_dir);
>  	}

Re: [PATCH v7 4/7] submodule: allow do_submodule_path to work if given gitdir directly

[Jacob Keller]

On Thu, Aug 18, 2016 at 11:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> Currently, do_submodule_path relies on read_gitfile, which will die() if
>> it can't read from the specified gitfile. Unfortunately, this means that
>> do_submodule_path will not work when given the path to a submodule which
>> is checked out directly, such as a newly added submodule which you
>> cloned and then "git submodule add".
>
> Hmm, are you sure about that?
>
> A call to read_gitfile() turns into a call to read_gitfile_gently()
> with the return_error_code parameter set to NULL.  The function does
> a stat(2), and if the given path is not a file (e.g. we created the
> submodule working tree and repository in-place ourselves, instead of
> cloning an existing project from elsewhere, in which case we would
> see a directory there), it says READ_GIT_FILE_ERR_NOT_A_FILE and
> returns NULL, because that is not a fatal error condition.  The same
> thing happens if path does not yet exist.
>
> This caller is given <path>, prepares "<path>/.git" in buf, and
> calls read_gitfile().  If it returns a non-NULL, it replaces what is
> in buf and continues, but if it returns a NULL (i.e. the two cases I
> mentioned in the above paragraph), it continues with "<path>/.git".
>
> While I do not think changing it to resolve_gitdir() is wrong per-se,
> I am not sure if it is necessary.
>
> I must be misreading something in the existing code.
>

No I think you're correct. I was under the assumption that it would
die here. I think it's probably better to stick with read_gitfile()
here, it should work. The main issue is what happens after this needs
to be fixed.

I'll investigate this more.

Thanks,
Jake

Re: [PATCH v7 6/7] submodule: refactor show_submodule_summary with helper function

[Stefan Beller]

On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> A future patch is going to add a new submodule diff format which
> displays an inline diff of the submodule changes. To make this easier,
> and to ensure that both submodule diff formats use the same initial
> header, factor out show_submodule_header() function which will print the
> current submodule header line, and then leave the show_submodule_summary
> function to lookup and print the submodule log format.
>
> This does create one format change in that "(revision walker failed)"
> will now be displayed on its own line rather than as part of the message
> because we no longer perform this step directly in the header display
> flow. However, this is a rare case and shouldn't impact much. In
> addition, since we no longer need to run prepare_submodule_summary to
> get the fast_backward and fast_forward variables, these have been
> removed from that call, and the show_submodule_header() function does
> its own mergebase lookup.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
>  submodule.c | 103 +++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 74 insertions(+), 29 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index e1a51b7506ff..e341ca7ffefd 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -277,8 +277,7 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt,
>  }
>
>  static int prepare_submodule_summary(struct rev_info *rev, const char *path,
> -               struct commit *left, struct commit *right,
> -               int *fast_forward, int *fast_backward)
> +               struct commit *left, struct commit *right)
>  {
>         struct commit_list *merge_bases, *list;
>
> @@ -290,12 +289,6 @@ static int prepare_submodule_summary(struct rev_info *rev, const char *path,
>         add_pending_object(rev, &left->object, path);
>         add_pending_object(rev, &right->object, path);
>         merge_bases = get_merge_bases(left, right);
> -       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,
> @@ -333,31 +326,23 @@ static void print_submodule_summary(struct rev_info *rev, FILE *f,
>         strbuf_release(&sb);
>  }
>
> -void show_submodule_summary(FILE *f, const char *path,
> +/* Helper function to display the submodule header line prior to the full
> + * summary output. If it can locate the submodule objects directory it will
> + * attempt to lookup both the left and right commits and put them into the
> + * left and right pointers.
> + */
> +static void show_submodule_header(FILE *f, const char *path,
>                 const char *line_prefix,
>                 unsigned char one[20], unsigned char two[20],
>                 unsigned dirty_submodule, const char *meta,
> -               const char *del, const char *add, const char *reset)
> +               const char *reset,
> +               struct commit **left, struct commit **right)
>  {
> -       struct rev_info rev;
> -       struct commit *left = NULL, *right = NULL;
> +       struct commit_list *merge_bases;
>         const char *message = NULL;
>         struct strbuf sb = STRBUF_INIT;
>         int fast_forward = 0, fast_backward = 0;
>
> -       if (is_null_sha1(two))
> -               message = "(submodule deleted)";
> -       else if (add_submodule_odb(path))
> -               message = "(not initialized)";
> -       else if (is_null_sha1(one))
> -               message = "(new submodule)";
> -       else if (!(left = lookup_commit_reference(one)) ||
> -                !(right = lookup_commit_reference(two)))
> -               message = "(commits not present)";
> -       else if (prepare_submodule_summary(&rev, path, left, right,
> -                                          &fast_forward, &fast_backward))
> -               message = "(revision walker failed)";
> -
>         if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
>                 fprintf(f, "%sSubmodule %s contains untracked content\n",
>                         line_prefix, path);
> @@ -365,11 +350,46 @@ void show_submodule_summary(FILE *f, const char *path,
>                 fprintf(f, "%sSubmodule %s contains modified content\n",
>                         line_prefix, path);
>
> +       if (is_null_sha1(one))
> +               message = "(new submodule)";
> +       else if (is_null_sha1(two))
> +               message = "(submodule deleted)";
> +
> +       if (add_submodule_odb(path)) {
> +               if (!message)
> +                       message = "(submodule not initialized)";

Before it was  "(not initialized)" for this case, I think we'd rather keep that?
Though this code path is only used by the porcelain commands, we'd rather not
want to change this in a subtle way in this commit.

If we were to change those, we could discuss if we want to go with
full sentences
all the time:

    submodule is new
    submodule is deleted
    submodule is not initialized



> +               goto output_header;
> +       }
> +
> +       /*
> +        * Attempt to lookup the commit references, and determine if this is
> +        * a fast forward or fast backwards update

nit: end the sentence with a period

> +        */
> +       *left = lookup_commit_reference(one);
> +       *right = lookup_commit_reference(two);
> +
> +       /*
> +        * Warn about missing commits in the submodule project, but only if
> +        * they aren't null

nit: end the sentence with a period

> +
> +void show_submodule_summary(FILE *f, const char *path,
> +               const char *line_prefix,
> +               unsigned char one[20], unsigned char two[20],
> +               unsigned dirty_submodule, const char *meta,
> +               const char *del, const char *add, const char *reset)
> +{
> +       struct rev_info rev;
> +       struct commit *left = NULL, *right = NULL;
> +
> +       show_submodule_header(f, path, line_prefix, one, two, dirty_submodule,
> +                             meta, reset, &left, &right);
> +
> +       /*
> +        * if we don't have both a left and a right pointer, then we can't
> +        * display a summary
> +        */


nit: Start the sentence with capital letters and end the sentence with a period.
Do we need to do another thing here before the return? (Maybe that could
go into the comment as well?)

Re: [PATCH v7 7/7] diff: teach diff to display submodule difference with an inline diff

[Stefan Beller]

On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:



>         if (o->submodule_format == DIFF_SUBMODULE_LOG &&
>             (!one->mode || S_ISGITLINK(one->mode)) &&
>             (!two->mode || S_ISGITLINK(two->mode))) {
> @@ -2311,6 +2322,17 @@ static void builtin_diff(const char *name_a,
>                                 two->dirty_submodule,
>                                 meta, del, add, reset);
>                 return;
> +       } else if (o->submodule_format == DIFF_SUBMODULE_INLINE_DIFF &&
> +                  (!one->mode || S_ISGITLINK(one->mode)) &&
> +                  (!two->mode || S_ISGITLINK(two->mode))) {

The ! mode is for added and deleted submodules, I guess?

> diff --git a/diff.h b/diff.h
> index ea5aba668eaa..192c0eedd0ff 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -112,6 +112,7 @@ enum diff_words_type {
>  enum diff_submodule_format {
>         DIFF_SUBMODULE_SHORT = 0,
>         DIFF_SUBMODULE_LOG,
> +       DIFF_SUBMODULE_INLINE_DIFF,
>  };
>
>  struct diff_options {
> diff --git a/submodule.c b/submodule.c
> index e341ca7ffefd..e5f1138f4362 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -436,6 +436,67 @@ void show_submodule_summary(FILE *f, const char *path,
>                 clear_commit_marks(right, ~0);
>  }
>
> +void show_submodule_inline_diff(FILE *f, const char *path,
> +               const char *line_prefix,
> +               unsigned char one[20], unsigned char two[20],
> +               unsigned dirty_submodule, const char *meta,
> +               const char *del, const char *add, const char *reset,
> +               const struct diff_options *o)
> +{
> +       const char *old = EMPTY_TREE_SHA1_BIN, *new = EMPTY_TREE_SHA1_BIN;

submodule.c: In function ‘show_submodule_inline_diff’:
cache.h:957:3: warning: pointer targets in initialization differ in
signedness [-Wpointer-sign]
   ((const unsigned char *) EMPTY_TREE_SHA1_BIN_LITERAL)

submodule.c:446:20: note: in expansion of macro ‘EMPTY_TREE_SHA1_BIN’
  const char *old = EMPTY_TREE_SHA1_BIN, *new = EMPTY_TREE_SHA1_BIN;



> +       struct commit *left = NULL, *right = NULL;
> +       struct strbuf submodule_dir = STRBUF_INIT;
> +       struct child_process cp = CHILD_PROCESS_INIT;
> +
> +       show_submodule_header(f, path, line_prefix, one, two, dirty_submodule,
> +                             meta, reset, &left, &right);
> +
> +       /* We need a valid left and right commit to display a difference */
> +       if (!(left || is_null_sha1(one)) ||
> +           !(right || is_null_sha1(two)))
> +               goto done;
> +
> +       if (left)
> +               old = one;

submodule.c:460:7: warning: pointer targets in assignment differ in
signedness [-Wpointer-sign]
   old = one;



> +       if (right)
> +               new = two;
> +
> +       fflush(f);
> +       cp.git_cmd = 1;
> +       cp.dir = path;
> +       cp.out = dup(fileno(f));
> +       cp.no_stdin = 1;
> +
> +       /* TODO: other options may need to be passed here. */
> +       argv_array_pushl(&cp.args, "diff");
> +       argv_array_pushf(&cp.args, "--line-prefix=%s", line_prefix);
> +       if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
> +               argv_array_pushf(&cp.args, "--src-prefix=%s%s/",
> +                                o->b_prefix, path);
> +               argv_array_pushf(&cp.args, "--dst-prefix=%s%s/",
> +                                o->a_prefix, path);
> +       } else {
> +               argv_array_pushf(&cp.args, "--src-prefix=%s%s/",
> +                                o->a_prefix, path);
> +               argv_array_pushf(&cp.args, "--dst-prefix=%s%s/",
> +                                o->b_prefix, path);
> +       }
> +       argv_array_push(&cp.args, sha1_to_hex(old));

submodule.c:484:2: warning: pointer targets in passing argument 1 of
‘sha1_to_hex’ differ in signedness [-Wpointer-sign]
  argv_array_push(&cp.args, sha1_to_hex(old));


/*
 * nit: the following comment doesn't adhere to Gits way of doing comments:
 */

> +       /* If the submodule has modified content, we will diff against the
> +        * work tree, under the assumption that the user has asked for the
> +        * diff format and wishes to actually see all differences even if they
> +        * haven't yet been committed to the submodule yet.
> +        */

Makes sort of sense when new is HEAD.

However if I have given an explicit sha1 in the superproject, e.g.:

    # (in the e.g. gerrit with a dirty submodule, I'd still expect to
    # get the dirtyness ignored, but the diff of those two states?)
    git diff --submodule=diff  cc82b24..5222e66 plugins/

> +       if (!(dirty_submodule & DIRTY_SUBMODULE_MODIFIED))
> +               argv_array_push(&cp.args, sha1_to_hex(new));
> +


> +# Tested non-UTF-8 encoding
> +test_encoding="ISO8859-1"
...
> +# String "added" in German (translated with Google Translate), encoded in UTF-8,
> +# used in sample commit log messages in add_file() function below.
> +added=$(printf "hinzugef\303\274gt")


> +add_file () {
> +       (
> +               cd "$1" &&
> +               shift &&
> +               for name
> +               do
> +                       echo "$name" >"$name" &&
> +                       git add "$name" &&
> +                       test_tick &&
> +                       # "git commit -m" would break MinGW, as Windows refuse to pass
> +                       # $test_encoding encoded parameter to git.
> +                       echo "Add $name ($added $name)" | iconv -f utf-8 -t $test_encoding |
> +                       git -c "i18n.commitEncoding=$test_encoding" commit -F -
> +               done >/dev/null &&
> +               git rev-parse --short --verify HEAD
> +       )
> +}
> +commit_file () {
> +       test_tick &&
> +       git commit "$@" -m "Commit $*" >/dev/null
> +}
> +
> +test_create_repo sm1 &&
> +add_file . foo >/dev/null
> +
> +head1=$(add_file sm1 foo1 foo2)
> +fullhead1=$(cd sm1; git rev-parse --verify HEAD)
> +
> +test_expect_success 'added submodule' '
> +       git add sm1 &&
> +       git diff-index -p --submodule=diff HEAD >actual &&
> +       cat >expected <<-EOF &&
> +       Submodule sm1 0000000...1beffeb (new submodule)

Do we also want to make the 1beffeb a variable?

> +       cat >expected <<-EOF &&
> +       Submodule sm1 0000000...$head1 (new submodule)

In the prior test we have spelled out the sha1s, here we refer to a variable?


> +       EOF
> +       git config --unset diff.submodule &&

    Use this one weird trick to make the tests more readable!
    Use "test_config" from test-lib-functions.sh
    (# Set git config, automatically unsetting it after the test is over.)
    (I am involved in Git for 3 years now, but just recently was pointed at it)

Re: [PATCH v7 7/7] diff: teach diff to display submodule difference with an inline diff

[Jacob Keller]

On Thu, Aug 18, 2016 at 12:47 PM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>
>
>
>>         if (o->submodule_format == DIFF_SUBMODULE_LOG &&
>>             (!one->mode || S_ISGITLINK(one->mode)) &&
>>             (!two->mode || S_ISGITLINK(two->mode))) {
>> @@ -2311,6 +2322,17 @@ static void builtin_diff(const char *name_a,
>>                                 two->dirty_submodule,
>>                                 meta, del, add, reset);
>>                 return;
>> +       } else if (o->submodule_format == DIFF_SUBMODULE_INLINE_DIFF &&
>> +                  (!one->mode || S_ISGITLINK(one->mode)) &&
>> +                  (!two->mode || S_ISGITLINK(two->mode))) {
>
> The ! mode is for added and deleted submodules, I guess?
>

I think so? I don't really know, but it was there before for
show_submodule_summary so I left it this way.

>> diff --git a/diff.h b/diff.h
>> index ea5aba668eaa..192c0eedd0ff 100644
>> --- a/diff.h
>> +++ b/diff.h
>> @@ -112,6 +112,7 @@ enum diff_words_type {
>>  enum diff_submodule_format {
>>         DIFF_SUBMODULE_SHORT = 0,
>>         DIFF_SUBMODULE_LOG,
>> +       DIFF_SUBMODULE_INLINE_DIFF,
>>  };
>>
>>  struct diff_options {
>> diff --git a/submodule.c b/submodule.c
>> index e341ca7ffefd..e5f1138f4362 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -436,6 +436,67 @@ void show_submodule_summary(FILE *f, const char *path,
>>                 clear_commit_marks(right, ~0);
>>  }
>>
>> +void show_submodule_inline_diff(FILE *f, const char *path,
>> +               const char *line_prefix,
>> +               unsigned char one[20], unsigned char two[20],
>> +               unsigned dirty_submodule, const char *meta,
>> +               const char *del, const char *add, const char *reset,
>> +               const struct diff_options *o)
>> +{
>> +       const char *old = EMPTY_TREE_SHA1_BIN, *new = EMPTY_TREE_SHA1_BIN;
>
> submodule.c: In function ‘show_submodule_inline_diff’:
> cache.h:957:3: warning: pointer targets in initialization differ in
> signedness [-Wpointer-sign]
>    ((const unsigned char *) EMPTY_TREE_SHA1_BIN_LITERAL)
>
> submodule.c:446:20: note: in expansion of macro ‘EMPTY_TREE_SHA1_BIN’
>   const char *old = EMPTY_TREE_SHA1_BIN, *new = EMPTY_TREE_SHA1_BIN;
>

This should actually be "EMPTY_TREE_SHA1_BIN_LITERAL, and these should
be unsigned, you're right. This will be fixed differently by using
struct object_id * instead, though.

Thanks,
Jake

>
>
>> +       struct commit *left = NULL, *right = NULL;
>> +       struct strbuf submodule_dir = STRBUF_INIT;
>> +       struct child_process cp = CHILD_PROCESS_INIT;
>> +
>> +       show_submodule_header(f, path, line_prefix, one, two, dirty_submodule,
>> +                             meta, reset, &left, &right);
>> +
>> +       /* We need a valid left and right commit to display a difference */
>> +       if (!(left || is_null_sha1(one)) ||
>> +           !(right || is_null_sha1(two)))
>> +               goto done;
>> +
>> +       if (left)
>> +               old = one;
>
> submodule.c:460:7: warning: pointer targets in assignment differ in
> signedness [-Wpointer-sign]
>    old = one;
>
>

This will also be fixed by switching to object_id.

>
>> +       if (right)
>> +               new = two;
>> +
>> +       fflush(f);
>> +       cp.git_cmd = 1;
>> +       cp.dir = path;
>> +       cp.out = dup(fileno(f));
>> +       cp.no_stdin = 1;
>> +
>> +       /* TODO: other options may need to be passed here. */
>> +       argv_array_pushl(&cp.args, "diff");
>> +       argv_array_pushf(&cp.args, "--line-prefix=%s", line_prefix);
>> +       if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
>> +               argv_array_pushf(&cp.args, "--src-prefix=%s%s/",
>> +                                o->b_prefix, path);
>> +               argv_array_pushf(&cp.args, "--dst-prefix=%s%s/",
>> +                                o->a_prefix, path);
>> +       } else {
>> +               argv_array_pushf(&cp.args, "--src-prefix=%s%s/",
>> +                                o->a_prefix, path);
>> +               argv_array_pushf(&cp.args, "--dst-prefix=%s%s/",
>> +                                o->b_prefix, path);
>> +       }
>> +       argv_array_push(&cp.args, sha1_to_hex(old));
>
> submodule.c:484:2: warning: pointer targets in passing argument 1 of
> ‘sha1_to_hex’ differ in signedness [-Wpointer-sign]
>   argv_array_push(&cp.args, sha1_to_hex(old));
>
>
> /*
>  * nit: the following comment doesn't adhere to Gits way of doing comments:
>  */
>

Yea, sorry. I have a habbit of the other format because the Linux
kernel networking tree requires this style, despite the rest of the
Linux kernel requiring the style used by git. So, I have to break out
of that habbit but sometimes I miss them.

>> +       /* If the submodule has modified content, we will diff against the
>> +        * work tree, under the assumption that the user has asked for the
>> +        * diff format and wishes to actually see all differences even if they
>> +        * haven't yet been committed to the submodule yet.
>> +        */
>
> Makes sort of sense when new is HEAD.
>
> However if I have given an explicit sha1 in the superproject, e.g.:
>
>     # (in the e.g. gerrit with a dirty submodule, I'd still expect to
>     # get the dirtyness ignored, but the diff of those two states?)
>     git diff --submodule=diff  cc82b24..5222e66 plugins/
>

AFAIK it actually does, because dirty_submodule won't be set with
DIRTY_SUBMODULE_MODIFIED unless you're actually diffing against the
index or worktree directly. So if you diff two commits they never show
these flags regardless of what's in your work tree.

If you diff some commit against your work tree then you might have
this flag set.

>> +
>> +test_expect_success 'added submodule' '
>> +       git add sm1 &&
>> +       git diff-index -p --submodule=diff HEAD >actual &&
>> +       cat >expected <<-EOF &&
>> +       Submodule sm1 0000000...1beffeb (new submodule)
>
> Do we also want to make the 1beffeb a variable?
>

Sure

>> +       cat >expected <<-EOF &&
>> +       Submodule sm1 0000000...$head1 (new submodule)
>
> In the prior test we have spelled out the sha1s, here we refer to a variable?
>

I copied these from another file and modified them to use diff, but I
can work this to use variables consistently.

>
>> +       EOF
>> +       git config --unset diff.submodule &&
>
>     Use this one weird trick to make the tests more readable!
>     Use "test_config" from test-lib-functions.sh
>     (# Set git config, automatically unsetting it after the test is over.)
>     (I am involved in Git for 3 years now, but just recently was pointed at it)

Yea, this was copied directly from another test file and modified to
use "inline diff" format instead of log format, which is why some of
these are done this way. I'll switch this to test_config.

Thanks,
Jake

Re: [PATCH v7 6/7] submodule: refactor show_submodule_summary with helper function

[Jacob Keller]

On Thu, Aug 18, 2016 at 12:04 PM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>> +       if (is_null_sha1(one))
>> +               message = "(new submodule)";
>> +       else if (is_null_sha1(two))
>> +               message = "(submodule deleted)";
>> +
>> +       if (add_submodule_odb(path)) {
>> +               if (!message)
>> +                       message = "(submodule not initialized)";
>
> Before it was  "(not initialized)" for this case, I think we'd rather keep that?
> Though this code path is only used by the porcelain commands, we'd rather not
> want to change this in a subtle way in this commit.
>
> If we were to change those, we could discuss if we want to go with
> full sentences
> all the time:
>
>     submodule is new
>     submodule is deleted
>     submodule is not initialized
>
>

I agree, I'll make a new patch that does this as a cleanup prior to
this re-work.

Thanks,
Jake

Re: [PATCH v7 6/7] submodule: refactor show_submodule_summary with helper function

[Stefan Beller]

On Thu, Aug 18, 2016 at 1:24 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Thu, Aug 18, 2016 at 12:04 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>>> +       if (is_null_sha1(one))
>>> +               message = "(new submodule)";
>>> +       else if (is_null_sha1(two))
>>> +               message = "(submodule deleted)";
>>> +
>>> +       if (add_submodule_odb(path)) {
>>> +               if (!message)
>>> +                       message = "(submodule not initialized)";
>>
>> Before it was  "(not initialized)" for this case, I think we'd rather keep that?
>> Though this code path is only used by the porcelain commands, we'd rather not
>> want to change this in a subtle way in this commit.
>>
>> If we were to change those, we could discuss if we want to go with
>> full sentences
>> all the time:
>>
>>     submodule is new
>>     submodule is deleted
>>     submodule is not initialized
>>
>>
>
> I agree, I'll make a new patch that does this as a cleanup prior to
> this re-work.

I was actually not suggesting to change that. I rather wanted to point out
that if we'd want that we'd rather do it consistently and in a different patch.
Sorry for not being clear.

Thanks,
Stefan

>
> Thanks,
> Jake

Re: [PATCH v7 6/7] submodule: refactor show_submodule_summary with helper function

[Jacob Keller]

On Thu, Aug 18, 2016 at 1:39 PM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Aug 18, 2016 at 1:24 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Thu, Aug 18, 2016 at 12:04 PM, Stefan Beller <sbeller@google.com> wrote:
>>> On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>>>> +       if (is_null_sha1(one))
>>>> +               message = "(new submodule)";
>>>> +       else if (is_null_sha1(two))
>>>> +               message = "(submodule deleted)";
>>>> +
>>>> +       if (add_submodule_odb(path)) {
>>>> +               if (!message)
>>>> +                       message = "(submodule not initialized)";
>>>
>>> Before it was  "(not initialized)" for this case, I think we'd rather keep that?
>>> Though this code path is only used by the porcelain commands, we'd rather not
>>> want to change this in a subtle way in this commit.
>>>
>>> If we were to change those, we could discuss if we want to go with
>>> full sentences
>>> all the time:
>>>
>>>     submodule is new
>>>     submodule is deleted
>>>     submodule is not initialized
>>>
>>>
>>
>> I agree, I'll make a new patch that does this as a cleanup prior to
>> this re-work.
>
> I was actually not suggesting to change that. I rather wanted to point out
> that if we'd want that we'd rather do it consistently and in a different patch.
> Sorry for not being clear.
>
> Thanks,
> Stefan
>

Sorry for being unclear myself, too. I'm keeping it as "not
initialized" and updating the description of the patch that changed it
from "not checked out" to "not initialized"

Thanks,
Jake

>>
>> Thanks,
>> Jake

Re: [PATCH v7 6/7] submodule: refactor show_submodule_summary with helper function

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

>>>> If we were to change those, we could discuss if we want to go with
>>>> full sentences
>>>> all the time:
>>>>
>>>>     submodule is new
>>>>     submodule is deleted
>>>>     submodule is not initialized
>>>
>>> I agree, I'll make a new patch that does this as a cleanup prior to
>>> this re-work.
>> ...
> Sorry for being unclear myself, too. I'm keeping it as "not
> initialized" and updating the description of the patch that changed it
> from "not checked out" to "not initialized"

Whether it is done inside or outside the scope of this series, the
other two "is new"/"is deleted" updates look very sensible ones to
make in the longer run.  I'd further suggest to unify "commits not
present" and "revision walker failed" into one error class.  From
the end user's point of view, they aren't very different.

Re: [PATCH v7 6/7] submodule: refactor show_submodule_summary with helper function

[Jacob Keller]

On Thu, Aug 18, 2016 at 1:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>>>>> If we were to change those, we could discuss if we want to go with
>>>>> full sentences
>>>>> all the time:
>>>>>
>>>>>     submodule is new
>>>>>     submodule is deleted
>>>>>     submodule is not initialized
>>>>
>>>> I agree, I'll make a new patch that does this as a cleanup prior to
>>>> this re-work.
>>> ...
>> Sorry for being unclear myself, too. I'm keeping it as "not
>> initialized" and updating the description of the patch that changed it
>> from "not checked out" to "not initialized"
>
> Whether it is done inside or outside the scope of this series, the
> other two "is new"/"is deleted" updates look very sensible ones to
> make in the longer run.  I'd further suggest to unify "commits not
> present" and "revision walker failed" into one error class.  From
> the end user's point of view, they aren't very different.

That won't work exactly due to the new way we separate the header
format from the revision walk. I can change the error case though. The
new show_submodule_header will not actually attempt a revision walk at
all and thus won't know to display "revision walker failed" at the
same place as the header fails. However I think almost all of the time
it should succeed if commits are present.

Thanks,
Jake