{"thread":{"id":"43859","subject":"[PATCH v6 0/3] Add support for displaying submodules as a proper diff","startedAt":"2016-08-15T23:07:14Z","lastAt":"2016-08-16T21:31:25Z","messageCount":13,"participants":["Jacob Keller","Junio C Hamano"],"isPatch":true,"patchVersion":6,"patchTotal":3},"messages":[{"id":"299421","messageId":"20160815230702.30817-1-jacob.e.keller@intel.com","threadId":"43859","inReplyTo":null,"subject":"[PATCH v6 0/3] Add support for displaying submodules as a proper diff","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-08-15T23:06:59Z","receivedAt":"2016-08-15T23:07:14Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nThis patch series adds support for displaying a submodule as a\ndifference between the pre and post commits. This allows projects who\nfrequently update submodule contents to view the submodule in the log as\nif it were just one squashed commit that updates all the files in the\nsubmodule together, even if you mix-match the submodule part with a\nregular part you just see a complete diff that shows all the changes.\n\nTo make this work, I also extended the graph-aware output with a\n--line-prefix option. This option extends both diff and log to show a\nprefix. It's a bit of a hack in someways, but it works for showing a\nprefix both when graph is enabled and when its not. I think it works\nquite well.\n\nI added several tests to the line-prefix, and also a few tests for the\nnew submodule format.\n\nI welcome comments on how to improve the graph line-prefix code, as well\nas the actual submodule diff format.\n\nJacob Keller (2):\n graph: add support for --line-prefix on all graph-aware output\n diff: add SUBMODULE_DIFF format to display submodule diff\n\nJunio C Hamano (1):\n diff.c: remove output_prefix_length field\n\n Documentation/diff-config.txt | 3 +-\n Documentation/diff-options.txt | 10 +-\n builtin/rev-list.c | 70 +-\n diff.c | 49 +-\n diff.h | 11 +-\n graph.c | 105 +--\n graph.h | 22 +-\n log-tree.c | 5 +-\n submodule.c | 130 ++++\n submodule.h | 6 +\n t/t4013-diff-various.sh | 6 +\n ...diff.diff_--line-prefix=abc_master_master^_side | 29 +\n t/t4013/diff.diff_--line-prefix_--cached_--_file0 | 15 +\n t/t4059-diff-submodule-option-diff-format.sh | 738 +++++++++++++++++++++\n t/t4202-log.sh | 323 +++++++++\n 15 files changed, 1419 insertions(+), 103 deletions(-)\n create mode 100644 t/t4013/diff.diff_--line-prefix=abc_master_master^_side\n create mode 100644 t/t4013/diff.diff_--line-prefix_--cached_--_file0\n create mode 100755 t/t4059-diff-submodule-option-diff-format.sh\n\n-- \n2.9.2.873.g47c31b4\n\n"},{"id":"299422","messageId":"20160815230702.30817-2-jacob.e.keller@intel.com","threadId":"43859","inReplyTo":"20160815230702.30817-1-jacob.e.keller@intel.com","subject":"[PATCH v6 1/3] diff.c: remove output_prefix_length field","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-08-15T23:07:00Z","receivedAt":"2016-08-15T23:07:23Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Junio C Hamano \n\n\"diff/log --stat\" has a logic that determines the display columns\navailable for the diffstat part of the output and apportions it for\npathnames and diffstat graph automatically.\n\n5e71a84a (Add output_prefix_length to diff_options, 2012-04-16)\nadded the output_prefix_length field to diff_options structure to\nallow this logic subtract the display columns used for display the\nhistory graph part from the total \"terminal width\"; this matters\nwhen the \"git log --graph -p\" option is in use.\n\nThe field be set to the number of display columns needed to show the\noutput from the output_prefix() callback. Any new output_prefix()\ncallback must also update the field accordingly, which is error\nprone. As there is only one user of the field, and the user has the\nactual value of the prefix string, let's get rid of the field and\nhave the user count the display width itself.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 2 +-\n diff.h | 1 -\n graph.c | 2 --\n 3 files changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex b43d3dd2ecb7..ae069c303077 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1625,7 +1625,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t */\n \n \tif (options->stat_width == -1)\n-\t\twidth = term_columns() - options->output_prefix_length;\n+\t\twidth = term_columns() - strlen(line_prefix);\n \telse\n \t\twidth = options->stat_width ? options->stat_width : 80;\n \tnumber_width = decimal_width(max_change) > number_width ?\ndiff --git a/diff.h b/diff.h\nindex 125447be09eb..49e4aaafb2da 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -174,7 +174,6 @@ struct diff_options {\n \tdiff_format_fn_t format_callback;\n \tvoid *format_callback_data;\n \tdiff_prefix_fn_t output_prefix;\n-\tint output_prefix_length;\n \tvoid *output_prefix_data;\n \n \tint diff_path_counter;\ndiff --git a/graph.c b/graph.c\nindex dd1720148dc5..a46803840511 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -197,7 +197,6 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void\n \tassert(opt);\n \tassert(graph);\n \n-\topt->output_prefix_length = graph->width;\n \tstrbuf_reset(&msgbuf);\n \tgraph_padding_line(graph, &msgbuf);\n \treturn &msgbuf;\n@@ -245,7 +244,6 @@ struct git_graph *graph_init(struct rev_info *opt)\n \t */\n \topt->diffopt.output_prefix = diff_output_prefix_callback;\n \topt->diffopt.output_prefix_data = graph;\n-\topt->diffopt.output_prefix_length = 0;\n \n \treturn graph;\n }\n-- \n2.9.2.873.g47c31b4\n\n"},{"id":"299423","messageId":"20160815230702.30817-3-jacob.e.keller@intel.com","threadId":"43859","inReplyTo":"20160815230702.30817-1-jacob.e.keller@intel.com","subject":"[PATCH v6 2/3] graph: add support for --line-prefix on all graph-aware output","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-08-15T23:07:01Z","receivedAt":"2016-08-15T23:07:27Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nAdd an extension to git-diff and git-log (and any other graph-aware\ndisplayable output) such that \"--line-prefix=\" will print the\nadditional line-prefix on every line of output.\n\nTo make this work, we have to fix a few bugs in the graph API that force\ngraph_show_commit_msg to be used only when you have a valid graph.\nAdditionally, we extend the default_diff_output_prefix handler to work\neven when no graph is enabled.\n\nThis is somewhat of a hack on top of the graph API, but I think it\nshould be acceptable here.\n\nThis will be used by a future extension of submodule display which\ndisplays the submodule diff as the actual diff between the pre and post\ncommit in the submodule project.\n\nAdd some tests for both git-log and git-diff to ensure that the prefix\nis honored correctly.\n\nSigned-off-by: Jacob Keller \n---\n Documentation/diff-options.txt | 3 +\n builtin/rev-list.c | 70 ++---\n diff.c | 6 +\n diff.h | 1 +\n graph.c | 103 ++++---\n graph.h | 22 +-\n log-tree.c | 5 +-\n t/t4013-diff-various.sh | 6 +\n ...diff.diff_--line-prefix=abc_master_master^_side | 29 ++\n t/t4013/diff.diff_--line-prefix_--cached_--_file0 | 15 +\n t/t4202-log.sh | 323 +++++++++++++++++++++\n 11 files changed, 503 insertions(+), 80 deletions(-)\n create mode 100644 t/t4013/diff.diff_--line-prefix=abc_master_master^_side\n create mode 100644 t/t4013/diff.diff_--line-prefix_--cached_--_file0\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 705a87394200..cc4342e2034d 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -569,5 +569,8 @@ endif::git-format-patch[]\n --no-prefix::\n \tDo not show any source or destination prefix.\n \n+--line-prefix=::\n+\tPrepend an additional prefix to every line of output.\n+\n For more detailed explanation on these common options, see also\n linkgit:gitdiffcore[7].\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 0ba82b1635b6..1a75a83538f4 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -122,48 +122,40 @@ static void show_commit(struct commit *commit, void *data)\n \t\tctx.fmt = revs->commit_format;\n \t\tctx.output_encoding = get_log_output_encoding();\n \t\tpretty_print_commit(&ctx, commit, &buf);\n-\t\tif (revs->graph) {\n-\t\t\tif (buf.len) {\n-\t\t\t\tif (revs->commit_format != CMIT_FMT_ONELINE)\n-\t\t\t\t\tgraph_show_oneline(revs->graph);\n+\t\tif (buf.len) {\n+\t\t\tif (revs->commit_format != CMIT_FMT_ONELINE)\n+\t\t\t\tgraph_show_oneline(revs->graph);\n \n-\t\t\t\tgraph_show_commit_msg(revs->graph, &buf);\n+\t\t\tgraph_show_commit_msg(revs->graph, stdout, &buf);\n \n-\t\t\t\t/*\n-\t\t\t\t * Add a newline after the commit message.\n-\t\t\t\t *\n-\t\t\t\t * Usually, this newline produces a blank\n-\t\t\t\t * padding line between entries, in which case\n-\t\t\t\t * we need to add graph padding on this line.\n-\t\t\t\t *\n-\t\t\t\t * However, the commit message may not end in a\n-\t\t\t\t * newline. In this case the newline simply\n-\t\t\t\t * ends the last line of the commit message,\n-\t\t\t\t * and we don't need any graph output. (This\n-\t\t\t\t * always happens with CMIT_FMT_ONELINE, and it\n-\t\t\t\t * happens with CMIT_FMT_USERFORMAT when the\n-\t\t\t\t * format doesn't explicitly end in a newline.)\n-\t\t\t\t */\n-\t\t\t\tif (buf.len && buf.buf[buf.len - 1] == '\\n')\n-\t\t\t\t\tgraph_show_padding(revs->graph);\n-\t\t\t\tputchar('\\n');\n-\t\t\t} else {\n-\t\t\t\t/*\n-\t\t\t\t * If the message buffer is empty, just show\n-\t\t\t\t * the rest of the graph output for this\n-\t\t\t\t * commit.\n-\t\t\t\t */\n-\t\t\t\tif (graph_show_remainder(revs->graph))\n-\t\t\t\t\tputchar('\\n');\n-\t\t\t\tif (revs->commit_format == CMIT_FMT_ONELINE)\n-\t\t\t\t\tputchar('\\n');\n-\t\t\t}\n+\t\t\t/*\n+\t\t\t\t* Add a newline after the commit message.\n+\t\t\t\t*\n+\t\t\t\t* Usually, this newline produces a blank\n+\t\t\t\t* padding line between entries, in which case\n+\t\t\t\t* we need to add graph padding on this line.\n+\t\t\t\t*\n+\t\t\t\t* However, the commit message may not end in a\n+\t\t\t\t* newline. In this case the newline simply\n+\t\t\t\t* ends the last line of the commit message,\n+\t\t\t\t* and we don't need any graph output. (This\n+\t\t\t\t* always happens with CMIT_FMT_ONELINE, and it\n+\t\t\t\t* happens with CMIT_FMT_USERFORMAT when the\n+\t\t\t\t* format doesn't explicitly end in a newline.)\n+\t\t\t\t*/\n+\t\t\tif (buf.len && buf.buf[buf.len - 1] == '\\n')\n+\t\t\t\tgraph_show_padding(revs->graph);\n+\t\t\tputchar('\\n');\n \t\t} else {\n-\t\t\tif (revs->commit_format != CMIT_FMT_USERFORMAT ||\n-\t\t\t buf.len) {\n-\t\t\t\tfwrite(buf.buf, 1, buf.len, stdout);\n-\t\t\t\tputchar(info->hdr_termination);\n-\t\t\t}\n+\t\t\t/*\n+\t\t\t\t* If the message buffer is empty, just show\n+\t\t\t\t* the rest of the graph output for this\n+\t\t\t\t* commit.\n+\t\t\t\t*/\n+\t\t\tif (graph_show_remainder(revs->graph))\n+\t\t\t\tputchar('\\n');\n+\t\t\tif (revs->commit_format == CMIT_FMT_ONELINE)\n+\t\t\t\tputchar('\\n');\n \t\t}\n \t\tstrbuf_release(&buf);\n \t} else {\ndiff --git a/diff.c b/diff.c\nindex ae069c303077..e286897b51e6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -18,6 +18,7 @@\n #include \"ll-merge.h\"\n #include \"string-list.h\"\n #include \"argv-array.h\"\n+#include \"graph.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -3966,6 +3967,11 @@ int diff_opt_parse(struct diff_options *options,\n \t\toptions->a_prefix = optarg;\n \t\treturn argcount;\n \t}\n+\telse if ((argcount = parse_long_opt(\"line-prefix\", av, &optarg))) {\n+\t\toptions->line_prefix = optarg;\n+\t\tgraph_setup_line_prefix(options);\n+\t\treturn argcount;\n+\t}\n \telse if ((argcount = parse_long_opt(\"dst-prefix\", av, &optarg))) {\n \t\toptions->b_prefix = optarg;\n \t\treturn argcount;\ndiff --git a/diff.h b/diff.h\nindex 49e4aaafb2da..83d0b1ae8580 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -115,6 +115,7 @@ struct diff_options {\n \tconst char *pickaxe;\n \tconst char *single_follow;\n \tconst char *a_prefix, *b_prefix;\n+\tconst char *line_prefix;\n \tunsigned flags;\n \tunsigned touched_flags;\n \ndiff --git a/graph.c b/graph.c\nindex a46803840511..3ab71456262c 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -2,7 +2,6 @@\n #include \"commit.h\"\n #include \"color.h\"\n #include \"graph.h\"\n-#include \"diff.h\"\n #include \"revision.h\"\n \n /* Internal API */\n@@ -28,8 +27,15 @@ static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);\n * responsible for printing this line's graph (perhaps via\n * graph_show_commit() or graph_show_oneline()) before calling\n * graph_show_strbuf().\n+ *\n+ * Note that unlike some other graph display functions, you must pass the file\n+ * handle directly. It is assumed that this is the same file handle as the\n+ * file specified by the graph diff options. This is necessary so that\n+ * graph_show_strbuf can be called even with a NULL graph.\n */\n-static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);\n+static void graph_show_strbuf(struct git_graph *graph,\n+\t\t\t FILE *file,\n+\t\t\t struct strbuf const *sb);\n \n /*\n * TODO:\n@@ -59,6 +65,17 @@ enum graph_state {\n \tGRAPH_COLLAPSING\n };\n \n+static void graph_show_line_prefix(const struct diff_options *diffopt)\n+{\n+\tif (!diffopt || !diffopt->line_prefix)\n+\t\treturn;\n+\n+\tfwrite(diffopt->line_prefix,\n+\t sizeof(char),\n+\t strlen(diffopt->line_prefix),\n+\t diffopt->file);\n+}\n+\n static const char **column_colors;\n static unsigned short column_colors_max;\n \n@@ -195,13 +212,27 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void\n \tstatic struct strbuf msgbuf = STRBUF_INIT;\n \n \tassert(opt);\n-\tassert(graph);\n \n \tstrbuf_reset(&msgbuf);\n-\tgraph_padding_line(graph, &msgbuf);\n+\tif (opt->line_prefix)\n+\t\tstrbuf_addstr(&msgbuf, opt->line_prefix);\n+\tif (graph)\n+\t\tgraph_padding_line(graph, &msgbuf);\n \treturn &msgbuf;\n }\n \n+static const struct diff_options *default_diffopt = NULL;\n+\n+void graph_setup_line_prefix(struct diff_options *diffopt)\n+{\n+\tdefault_diffopt = diffopt;\n+\n+\t/* setup an output prefix callback if necessary */\n+\tif (diffopt && !diffopt->output_prefix)\n+\t\tdiffopt->output_prefix = diff_output_prefix_callback;\n+}\n+\n+\n struct git_graph *graph_init(struct rev_info *opt)\n {\n \tstruct git_graph *graph = xmalloc(sizeof(struct git_graph));\n@@ -1183,8 +1214,10 @@ void graph_show_commit(struct git_graph *graph)\n \tstruct strbuf msgbuf = STRBUF_INIT;\n \tint shown_commit_line = 0;\n \n-\tif (!graph)\n+\tif (!graph) {\n+\t\tgraph_show_line_prefix(default_diffopt);\n \t\treturn;\n+\t}\n \n \t/*\n \t * When showing a diff of a merge against each of its parents, we\n@@ -1198,6 +1231,7 @@ void graph_show_commit(struct git_graph *graph)\n \n \twhile (!shown_commit_line && !graph_is_commit_finished(graph)) {\n \t\tshown_commit_line = graph_next_line(graph, &msgbuf);\n+\t\tgraph_show_line_prefix(&graph->revs->diffopt);\n \t\tfwrite(msgbuf.buf, sizeof(char), msgbuf.len,\n \t\t\tgraph->revs->diffopt.file);\n \t\tif (!shown_commit_line)\n@@ -1212,10 +1246,13 @@ void graph_show_oneline(struct git_graph *graph)\n {\n \tstruct strbuf msgbuf = STRBUF_INIT;\n \n-\tif (!graph)\n+\tif (!graph) {\n+\t\tgraph_show_line_prefix(default_diffopt);\n \t\treturn;\n+\t}\n \n \tgraph_next_line(graph, &msgbuf);\n+\tgraph_show_line_prefix(&graph->revs->diffopt);\n \tfwrite(msgbuf.buf, sizeof(char), msgbuf.len, graph->revs->diffopt.file);\n \tstrbuf_release(&msgbuf);\n }\n@@ -1224,10 +1261,13 @@ void graph_show_padding(struct git_graph *graph)\n {\n \tstruct strbuf msgbuf = STRBUF_INIT;\n \n-\tif (!graph)\n+\tif (!graph) {\n+\t\tgraph_show_line_prefix(default_diffopt);\n \t\treturn;\n+\t}\n \n \tgraph_padding_line(graph, &msgbuf);\n+\tgraph_show_line_prefix(&graph->revs->diffopt);\n \tfwrite(msgbuf.buf, sizeof(char), msgbuf.len, graph->revs->diffopt.file);\n \tstrbuf_release(&msgbuf);\n }\n@@ -1237,14 +1277,19 @@ int graph_show_remainder(struct git_graph *graph)\n \tstruct strbuf msgbuf = STRBUF_INIT;\n \tint shown = 0;\n \n-\tif (!graph)\n+\tif (!graph) {\n+\t\tgraph_show_line_prefix(default_diffopt);\n \t\treturn 0;\n+\t}\n \n-\tif (graph_is_commit_finished(graph))\n+\tif (graph_is_commit_finished(graph)) {\n+\t\tgraph_show_line_prefix(&graph->revs->diffopt);\n \t\treturn 0;\n+\t}\n \n \tfor (;;) {\n \t\tgraph_next_line(graph, &msgbuf);\n+\t\tgraph_show_line_prefix(&graph->revs->diffopt);\n \t\tfwrite(msgbuf.buf, sizeof(char), msgbuf.len,\n \t\t\tgraph->revs->diffopt.file);\n \t\tstrbuf_setlen(&msgbuf, 0);\n@@ -1260,17 +1305,12 @@ int graph_show_remainder(struct git_graph *graph)\n \treturn shown;\n }\n \n-\n-static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)\n+static void graph_show_strbuf(struct git_graph *graph,\n+\t\t\t FILE *file,\n+\t\t\t struct strbuf const *sb)\n {\n \tchar *p;\n \n-\tif (!graph) {\n-\t\tfwrite(sb->buf, sizeof(char), sb->len,\n-\t\t\tgraph->revs->diffopt.file);\n-\t\treturn;\n-\t}\n-\n \t/*\n \t * Print the strbuf line by line,\n \t * and display the graph info before each line but the first.\n@@ -1285,7 +1325,7 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)\n \t\t} else {\n \t\t\tlen = (sb->buf + sb->len) - p;\n \t\t}\n-\t\tfwrite(p, sizeof(char), len, graph->revs->diffopt.file);\n+\t\tfwrite(p, sizeof(char), len, file);\n \t\tif (next_p && *next_p != '\\0')\n \t\t\tgraph_show_oneline(graph);\n \t\tp = next_p;\n@@ -1293,29 +1333,20 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)\n }\n \n void graph_show_commit_msg(struct git_graph *graph,\n+\t\t\t FILE *file,\n \t\t\t struct strbuf const *sb)\n {\n \tint newline_terminated;\n \n-\tif (!graph) {\n-\t\t/*\n-\t\t * If there's no graph, just print the message buffer.\n-\t\t *\n-\t\t * The message buffer for CMIT_FMT_ONELINE and\n-\t\t * CMIT_FMT_USERFORMAT are already missing a terminating\n-\t\t * newline. All of the other formats should have it.\n-\t\t */\n-\t\tfwrite(sb->buf, sizeof(char), sb->len,\n-\t\t\tgraph->revs->diffopt.file);\n-\t\treturn;\n-\t}\n-\n-\tnewline_terminated = (sb->len && sb->buf[sb->len - 1] == '\\n');\n-\n \t/*\n \t * Show the commit message\n \t */\n-\tgraph_show_strbuf(graph, sb);\n+\tgraph_show_strbuf(graph, file, sb);\n+\n+\tif (!graph)\n+\t\treturn;\n+\n+\tnewline_terminated = (sb->len && sb->buf[sb->len - 1] == '\\n');\n \n \t/*\n \t * If there is more output needed for this commit, show it now\n@@ -1327,7 +1358,7 @@ void graph_show_commit_msg(struct git_graph *graph,\n \t\t * new line.\n \t\t */\n \t\tif (!newline_terminated)\n-\t\t\tputc('\\n', graph->revs->diffopt.file);\n+\t\t\tputc('\\n', file);\n \n \t\tgraph_show_remainder(graph);\n \n@@ -1335,6 +1366,6 @@ void graph_show_commit_msg(struct git_graph *graph,\n \t\t * If sb ends with a newline, our output should too.\n \t\t */\n \t\tif (newline_terminated)\n-\t\t\tputc('\\n', graph->revs->diffopt.file);\n+\t\t\tputc('\\n', file);\n \t}\n }\ndiff --git a/graph.h b/graph.h\nindex 3f48c19b6208..af623390b605 100644\n--- a/graph.h\n+++ b/graph.h\n@@ -1,9 +1,22 @@\n #ifndef GRAPH_H\n #define GRAPH_H\n+#include \"diff.h\"\n \n /* A graph is a pointer to this opaque structure */\n struct git_graph;\n \n+/*\n+ * Called to setup global display of line_prefix diff option.\n+ *\n+ * Passed a diff_options structure which indicates the line_prefix and the\n+ * file to output the prefix to. This is sort of a hack used so that the\n+ * line_prefix will be honored by all flows which also honor \"--graph\"\n+ * regardless of whether a graph has actually been setup. The normal graph\n+ * flow will honor the exact diff_options passed, but a NULL graph will cause\n+ * display of a line_prefix to stdout.\n+ */\n+void graph_setup_line_prefix(struct diff_options *diffopt);\n+\n /*\n * Set up a custom scheme for column colors.\n *\n@@ -113,7 +126,14 @@ int graph_show_remainder(struct git_graph *graph);\n * missing a terminating newline (including if it is empty), the output\n * printed by graph_show_commit_msg() will also be missing a terminating\n * newline.\n+ *\n+ * Note that unlike some other graph display functions, you must pass the file\n+ * handle directly. It is assumed that this is the same file handle as the\n+ * file specified by the graph diff options. This is necessary so that\n+ * graph_show_commit_msg can be called even with a NULL graph.\n */\n-void graph_show_commit_msg(struct git_graph *graph, struct strbuf const *sb);\n+void graph_show_commit_msg(struct git_graph *graph,\n+\t\t\t FILE *file,\n+\t\t\t struct strbuf const *sb);\n \n #endif /* GRAPH_H */\ndiff --git a/log-tree.c b/log-tree.c\nindex bfb735c84556..8c2415747a26 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -715,10 +715,7 @@ void show_log(struct rev_info *opt)\n \telse\n \t\topt->missing_newline = 0;\n \n-\tif (opt->graph)\n-\t\tgraph_show_commit_msg(opt->graph, &msgbuf);\n-\telse\n-\t\tfwrite(msgbuf.buf, sizeof(char), msgbuf.len, opt->diffopt.file);\n+\tgraph_show_commit_msg(opt->graph, opt->diffopt.file, &msgbuf);\n \tif (opt->use_terminator && !commit_format_is_empty(opt->commit_format)) {\n \t\tif (!opt->missing_newline)\n \t\t\tgraph_show_padding(opt->graph);\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 94ef5000e787..566817e2efdc 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -306,6 +306,8 @@ diff --no-index --name-status dir2 dir\n diff --no-index --name-status -- dir2 dir\n diff --no-index dir dir3\n diff master master^ side\n+# Can't use spaces...\n+diff --line-prefix=abc master master^ side\n diff --dirstat master~1 master~2\n diff --dirstat initial rearrange\n diff --dirstat-by-file initial rearrange\n@@ -325,6 +327,10 @@ test_expect_success 'diff --cached -- file on unborn branch' '\n \tgit diff --cached -- file0 >result &&\n \ttest_cmp \"$TEST_DIRECTORY/t4013/diff.diff_--cached_--_file0\" result\n '\n+test_expect_success 'diff --line-prefix with spaces' '\n+\tgit diff --line-prefix=\"| | | \" --cached -- file0 >result &&\n+\ttest_cmp \"$TEST_DIRECTORY/t4013/diff.diff_--line-prefix_--cached_--_file0\" result\n+'\n \n test_expect_success 'diff-tree --stdin with log formatting' '\n \tcat >expect <<-\\EOF &&\ndiff --git a/t/t4013/diff.diff_--line-prefix=abc_master_master^_side b/t/t4013/diff.diff_--line-prefix=abc_master_master^_side\nnew file mode 100644\nindex 000000000000..99f91e7f0e32\n--- /dev/null\n+++ b/t/t4013/diff.diff_--line-prefix=abc_master_master^_side\n@@ -0,0 +1,29 @@\n+$ git diff --line-prefix=abc master master^ side\n+abcdiff --cc dir/sub\n+abcindex cead32e,7289e35..992913c\n+abc--- a/dir/sub\n+abc+++ b/dir/sub\n+abc@@@ -1,6 -1,4 +1,8 @@@\n+abc A\n+abc B\n+abc +C\n+abc +D\n+abc +E\n+abc +F\n+abc+ 1\n+abc+ 2\n+abcdiff --cc file0\n+abcindex b414108,f4615da..10a8a9f\n+abc--- a/file0\n+abc+++ b/file0\n+abc@@@ -1,6 -1,6 +1,9 @@@\n+abc 1\n+abc 2\n+abc 3\n+abc +4\n+abc +5\n+abc +6\n+abc+ A\n+abc+ B\n+abc+ C\n+$\ndiff --git a/t/t4013/diff.diff_--line-prefix_--cached_--_file0 b/t/t4013/diff.diff_--line-prefix_--cached_--_file0\nnew file mode 100644\nindex 000000000000..f41ba4d36aa1\n--- /dev/null\n+++ b/t/t4013/diff.diff_--line-prefix_--cached_--_file0\n@@ -0,0 +1,15 @@\n+| | | diff --git a/file0 b/file0\n+| | | new file mode 100644\n+| | | index 0000000..10a8a9f\n+| | | --- /dev/null\n+| | | +++ b/file0\n+| | | @@ -0,0 +1,9 @@\n+| | | +1\n+| | | +2\n+| | | +3\n+| | | +4\n+| | | +5\n+| | | +6\n+| | | +A\n+| | | +B\n+| | | +C\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex e2db47c36e09..1ccbd5948a73 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -187,6 +187,16 @@ test_expect_success 'git log --no-walk=sorted sorts by commit time' '\n \ttest_cmp expect actual\n '\n \n+cat > expect << EOF\n+=== 804a787 sixth\n+=== 394ef78 fifth\n+=== 5d31159 fourth\n+EOF\n+test_expect_success 'git log --line-prefix=\"=== \" --no-walk sorts by commit time' '\n+\tgit log --line-prefix=\"=== \" --no-walk --oneline 5d31159 804a787 394ef78 > actual &&\n+\ttest_cmp expect actual\n+'\n+\n cat > expect << EOF\n 5d31159 fourth\n 804a787 sixth\n@@ -284,6 +294,21 @@ test_expect_success 'simple log --graph' '\n \ttest_cmp expect actual\n '\n \n+cat > expect <actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'set up merge history' '\n \tgit checkout -b side HEAD~4 &&\n \ttest_commit side-1 1 1 &&\n@@ -313,6 +338,27 @@ test_expect_success 'log --graph with merge' '\n \ttest_cmp expect actual\n '\n \n+cat > expect <<\\EOF\n+| | | * Merge branch 'side'\n+| | | |\\\n+| | | | * side-2\n+| | | | * side-1\n+| | | * | Second\n+| | | * | sixth\n+| | | * | fifth\n+| | | * | fourth\n+| | | |/\n+| | | * third\n+| | | * second\n+| | | * initial\n+EOF\n+\n+test_expect_success 'log --graph --line-prefix=\"| | | \" with merge' '\n+\tgit log --line-prefix=\"| | | \" --graph --date-order --pretty=tformat:%s |\n+\t\tsed \"s/ *\\$//\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'log --raw --graph -m with merge' '\n \tgit log --raw --graph --oneline -m master | head -n 500 >actual &&\n \tgrep \"initial\" actual\n@@ -867,6 +913,283 @@ test_expect_success 'log --graph with diff and stats' '\n \ttest_i18ncmp expect actual.sanitized\n '\n \n+cat >expect <<\\EOF\n+*** * commit COMMIT_OBJECT_NAME\n+*** |\\ Merge: MERGE_PARENTS\n+*** | | Author: A U Thor \n+*** | |\n+*** | | Merge HEADS DESCRIPTION\n+*** | |\n+*** | * commit COMMIT_OBJECT_NAME\n+*** | | Author: A U Thor \n+*** | |\n+*** | | reach\n+*** | | ---\n+*** | | reach.t | 1 +\n+*** | | 1 file changed, 1 insertion(+)\n+*** | |\n+*** | | diff --git a/reach.t b/reach.t\n+*** | | new file mode 100644\n+*** | | index 0000000..10c9591\n+*** | | --- /dev/null\n+*** | | +++ b/reach.t\n+*** | | @@ -0,0 +1 @@\n+*** | | +reach\n+*** | |\n+*** | \\\n+*** *-. \\ commit COMMIT_OBJECT_NAME\n+*** |\\ \\ \\ Merge: MERGE_PARENTS\n+*** | | | | Author: A U Thor \n+*** | | | |\n+*** | | | | Merge HEADS DESCRIPTION\n+*** | | | |\n+*** | | * | commit COMMIT_OBJECT_NAME\n+*** | | |/ Author: A U Thor \n+*** | | |\n+*** | | | octopus-b\n+*** | | | ---\n+*** | | | octopus-b.t | 1 +\n+*** | | | 1 file changed, 1 insertion(+)\n+*** | | |\n+*** | | | diff --git a/octopus-b.t b/octopus-b.t\n+*** | | | new file mode 100644\n+*** | | | index 0000000..d5fcad0\n+*** | | | --- /dev/null\n+*** | | | +++ b/octopus-b.t\n+*** | | | @@ -0,0 +1 @@\n+*** | | | +octopus-b\n+*** | | |\n+*** | * | commit COMMIT_OBJECT_NAME\n+*** | |/ Author: A U Thor \n+*** | |\n+*** | | octopus-a\n+*** | | ---\n+*** | | octopus-a.t | 1 +\n+*** | | 1 file changed, 1 insertion(+)\n+*** | |\n+*** | | diff --git a/octopus-a.t b/octopus-a.t\n+*** | | new file mode 100644\n+*** | | index 0000000..11ee015\n+*** | | --- /dev/null\n+*** | | +++ b/octopus-a.t\n+*** | | @@ -0,0 +1 @@\n+*** | | +octopus-a\n+*** | |\n+*** * | commit COMMIT_OBJECT_NAME\n+*** |/ Author: A U Thor \n+*** |\n+*** | seventh\n+*** | ---\n+*** | seventh.t | 1 +\n+*** | 1 file changed, 1 insertion(+)\n+*** |\n+*** | diff --git a/seventh.t b/seventh.t\n+*** | new file mode 100644\n+*** | index 0000000..9744ffc\n+*** | --- /dev/null\n+*** | +++ b/seventh.t\n+*** | @@ -0,0 +1 @@\n+*** | +seventh\n+*** |\n+*** * commit COMMIT_OBJECT_NAME\n+*** |\\ Merge: MERGE_PARENTS\n+*** | | Author: A U Thor \n+*** | |\n+*** | | Merge branch 'tangle'\n+*** | |\n+*** | * commit COMMIT_OBJECT_NAME\n+*** | |\\ Merge: MERGE_PARENTS\n+*** | | | Author: A U Thor \n+*** | | |\n+*** | | | Merge branch 'side' (early part) into tangle\n+*** | | |\n+*** | * | commit COMMIT_OBJECT_NAME\n+*** | |\\ \\ Merge: MERGE_PARENTS\n+*** | | | | Author: A U Thor \n+*** | | | |\n+*** | | | | Merge branch 'master' (early part) into tangle\n+*** | | | |\n+*** | * | | commit COMMIT_OBJECT_NAME\n+*** | | | | Author: A U Thor \n+*** | | | |\n+*** | | | | tangle-a\n+*** | | | | ---\n+*** | | | | tangle-a | 1 +\n+*** | | | | 1 file changed, 1 insertion(+)\n+*** | | | |\n+*** | | | | diff --git a/tangle-a b/tangle-a\n+*** | | | | new file mode 100644\n+*** | | | | index 0000000..7898192\n+*** | | | | --- /dev/null\n+*** | | | | +++ b/tangle-a\n+*** | | | | @@ -0,0 +1 @@\n+*** | | | | +a\n+*** | | | |\n+*** * | | | commit COMMIT_OBJECT_NAME\n+*** |\\ \\ \\ \\ Merge: MERGE_PARENTS\n+*** | | | | | Author: A U Thor \n+*** | | | | |\n+*** | | | | | Merge branch 'side'\n+*** | | | | |\n+*** | * | | | commit COMMIT_OBJECT_NAME\n+*** | | |_|/ Author: A U Thor \n+*** | |/| |\n+*** | | | | side-2\n+*** | | | | ---\n+*** | | | | 2 | 1 +\n+*** | | | | 1 file changed, 1 insertion(+)\n+*** | | | |\n+*** | | | | diff --git a/2 b/2\n+*** | | | | new file mode 100644\n+*** | | | | index 0000000..0cfbf08\n+*** | | | | --- /dev/null\n+*** | | | | +++ b/2\n+*** | | | | @@ -0,0 +1 @@\n+*** | | | | +2\n+*** | | | |\n+*** | * | | commit COMMIT_OBJECT_NAME\n+*** | | | | Author: A U Thor \n+*** | | | |\n+*** | | | | side-1\n+*** | | | | ---\n+*** | | | | 1 | 1 +\n+*** | | | | 1 file changed, 1 insertion(+)\n+*** | | | |\n+*** | | | | diff --git a/1 b/1\n+*** | | | | new file mode 100644\n+*** | | | | index 0000000..d00491f\n+*** | | | | --- /dev/null\n+*** | | | | +++ b/1\n+*** | | | | @@ -0,0 +1 @@\n+*** | | | | +1\n+*** | | | |\n+*** * | | | commit COMMIT_OBJECT_NAME\n+*** | | | | Author: A U Thor \n+*** | | | |\n+*** | | | | Second\n+*** | | | | ---\n+*** | | | | one | 1 +\n+*** | | | | 1 file changed, 1 insertion(+)\n+*** | | | |\n+*** | | | | diff --git a/one b/one\n+*** | | | | new file mode 100644\n+*** | | | | index 0000000..9a33383\n+*** | | | | --- /dev/null\n+*** | | | | +++ b/one\n+*** | | | | @@ -0,0 +1 @@\n+*** | | | | +case\n+*** | | | |\n+*** * | | | commit COMMIT_OBJECT_NAME\n+*** | |_|/ Author: A U Thor \n+*** |/| |\n+*** | | | sixth\n+*** | | | ---\n+*** | | | a/two | 1 -\n+*** | | | 1 file changed, 1 deletion(-)\n+*** | | |\n+*** | | | diff --git a/a/two b/a/two\n+*** | | | deleted file mode 100644\n+*** | | | index 9245af5..0000000\n+*** | | | --- a/a/two\n+*** | | | +++ /dev/null\n+*** | | | @@ -1 +0,0 @@\n+*** | | | -ni\n+*** | | |\n+*** * | | commit COMMIT_OBJECT_NAME\n+*** | | | Author: A U Thor \n+*** | | |\n+*** | | | fifth\n+*** | | | ---\n+*** | | | a/two | 1 +\n+*** | | | 1 file changed, 1 insertion(+)\n+*** | | |\n+*** | | | diff --git a/a/two b/a/two\n+*** | | | new file mode 100644\n+*** | | | index 0000000..9245af5\n+*** | | | --- /dev/null\n+*** | | | +++ b/a/two\n+*** | | | @@ -0,0 +1 @@\n+*** | | | +ni\n+*** | | |\n+*** * | | commit COMMIT_OBJECT_NAME\n+*** |/ / Author: A U Thor \n+*** | |\n+*** | | fourth\n+*** | | ---\n+*** | | ein | 1 +\n+*** | | 1 file changed, 1 insertion(+)\n+*** | |\n+*** | | diff --git a/ein b/ein\n+*** | | new file mode 100644\n+*** | | index 0000000..9d7e69f\n+*** | | --- /dev/null\n+*** | | +++ b/ein\n+*** | | @@ -0,0 +1 @@\n+*** | | +ichi\n+*** | |\n+*** * | commit COMMIT_OBJECT_NAME\n+*** |/ Author: A U Thor \n+*** |\n+*** | third\n+*** | ---\n+*** | ichi | 1 +\n+*** | one | 1 -\n+*** | 2 files changed, 1 insertion(+), 1 deletion(-)\n+*** |\n+*** | diff --git a/ichi b/ichi\n+*** | new file mode 100644\n+*** | index 0000000..9d7e69f\n+*** | --- /dev/null\n+*** | +++ b/ichi\n+*** | @@ -0,0 +1 @@\n+*** | +ichi\n+*** | diff --git a/one b/one\n+*** | deleted file mode 100644\n+*** | index 9d7e69f..0000000\n+*** | --- a/one\n+*** | +++ /dev/null\n+*** | @@ -1 +0,0 @@\n+*** | -ichi\n+*** |\n+*** * commit COMMIT_OBJECT_NAME\n+*** | Author: A U Thor \n+*** |\n+*** | second\n+*** | ---\n+*** | one | 2 +-\n+*** | 1 file changed, 1 insertion(+), 1 deletion(-)\n+*** |\n+*** | diff --git a/one b/one\n+*** | index 5626abf..9d7e69f 100644\n+*** | --- a/one\n+*** | +++ b/one\n+*** | @@ -1 +1 @@\n+*** | -one\n+*** | +ichi\n+*** |\n+*** * commit COMMIT_OBJECT_NAME\n+*** Author: A U Thor \n+***\n+*** initial\n+*** ---\n+*** one | 1 +\n+*** 1 file changed, 1 insertion(+)\n+***\n+*** diff --git a/one b/one\n+*** new file mode 100644\n+*** index 0000000..5626abf\n+*** --- /dev/null\n+*** +++ b/one\n+*** @@ -0,0 +1 @@\n+*** +one\n+EOF\n+\n+test_expect_success 'log --line-prefix=\"*** \" --graph with diff and stats' '\n+\tgit log --line-prefix=\"*** \" --no-renames --graph --pretty=short --stat -p >actual &&\n+\tsanitize_output >actual.sanitized expect &&\n-- \n2.9.2.873.g47c31b4\n\n"},{"id":"299424","messageId":"20160815230702.30817-4-jacob.e.keller@intel.com","threadId":"43859","inReplyTo":"20160815230702.30817-1-jacob.e.keller@intel.com","subject":"[PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-08-15T23:07:02Z","receivedAt":"2016-08-15T23:07:29Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nTeach git-diff and friends a new format for displaying the difference of\na submodule using git-diff inside the submodule project. This allows\nusers to easily see exactly what source changed in a given commit that\nupdates the submodule pointer. To do this, remove DIFF_SUBMODULE_LOG bit\nfrom the diff options, and instead add a new enum type for these\nformats.\n\nAdd some tests for the new format and option.\n\nSigned-off-by: Jacob Keller \n---\n Documentation/diff-config.txt | 3 +-\n Documentation/diff-options.txt | 7 +-\n diff.c | 41 +-\n diff.h | 9 +-\n submodule.c | 130 +++++\n submodule.h | 6 +\n t/t4059-diff-submodule-option-diff-format.sh | 738 +++++++++++++++++++++++++++\n 7 files changed, 915 insertions(+), 19 deletions(-)\n create mode 100755 t/t4059-diff-submodule-option-diff-format.sh\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex d5a5b17d5088..f5d693afad6c 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -123,7 +123,8 @@ diff.suppressBlankEmpty::\n diff.submodule::\n \tSpecify the format in which differences in submodules are\n \tshown. The \"log\" format lists the commits in the range like\n-\tlinkgit:git-submodule[1] `summary` does. The \"short\" format\n+\tlinkgit:git-submodule[1] `summary` does. The \"diff\" format shows the\n+\tdiff between the beginning and end of the range. The \"short\" format\n \tformat just shows the names of the commits at the beginning\n \tand end of the range. Defaults to short.\n \ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex cc4342e2034d..d3ca4ad2c2c5 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -215,8 +215,11 @@ any of those replacements occurred.\n \tthe commits in the range like linkgit:git-submodule[1] `summary` does.\n \tOmitting the `--submodule` option or specifying `--submodule=short`,\n \tuses the 'short' format. This format just shows the names of the commits\n-\tat the beginning and end of the range. Can be tweaked via the\n-\t`diff.submodule` configuration variable.\n+\tat the beginning and end of the range. When `--submodule=diff` is\n+\tgiven, the 'diff' format is used. This format shows the diff between\n+\tthe old and new submodule commmit from the perspective of the\n+\tsubmodule. Defaults to `diff.submodule` or 'short' if the config\n+\toption is unset.\n \n --color[=]::\n \tShow colored diff.\ndiff --git a/diff.c b/diff.c\nindex e286897b51e6..232fbe17680f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -132,9 +132,11 @@ static int parse_dirstat_params(struct diff_options *options, const char *params\n static int parse_submodule_params(struct diff_options *options, const char *value)\n {\n \tif (!strcmp(value, \"log\"))\n-\t\tDIFF_OPT_SET(options, SUBMODULE_LOG);\n+\t\toptions->submodule_format = DIFF_SUBMODULE_LOG;\n+\telse if (!strcmp(value, \"diff\"))\n+\t\toptions->submodule_format = DIFF_SUBMODULE_DIFF;\n \telse if (!strcmp(value, \"short\"))\n-\t\tDIFF_OPT_CLR(options, SUBMODULE_LOG);\n+\t\toptions->submodule_format = DIFF_SUBMODULE_SHORT;\n \telse\n \t\treturn -1;\n \treturn 0;\n@@ -2300,9 +2302,18 @@ static void builtin_diff(const char *name_a,\n \tstruct strbuf header = STRBUF_INIT;\n \tconst char *line_prefix = diff_line_prefix(o);\n \n-\tif (DIFF_OPT_TST(o, SUBMODULE_LOG) &&\n-\t\t\t(!one->mode || S_ISGITLINK(one->mode)) &&\n-\t\t\t(!two->mode || S_ISGITLINK(two->mode))) {\n+\tdiff_set_mnemonic_prefix(o, \"a/\", \"b/\");\n+\tif (DIFF_OPT_TST(o, REVERSE_DIFF)) {\n+\t\ta_prefix = o->b_prefix;\n+\t\tb_prefix = o->a_prefix;\n+\t} else {\n+\t\ta_prefix = o->a_prefix;\n+\t\tb_prefix = o->b_prefix;\n+\t}\n+\n+\tif (o->submodule_format == DIFF_SUBMODULE_LOG &&\n+\t (!one->mode || S_ISGITLINK(one->mode)) &&\n+\t (!two->mode || S_ISGITLINK(two->mode))) {\n \t\tconst char *del = diff_get_color_opt(o, DIFF_FILE_OLD);\n \t\tconst char *add = diff_get_color_opt(o, DIFF_FILE_NEW);\n \t\tshow_submodule_summary(o->file, one->path ? one->path : two->path,\n@@ -2311,6 +2322,15 @@ static void builtin_diff(const char *name_a,\n \t\t\t\ttwo->dirty_submodule,\n \t\t\t\tmeta, del, add, reset);\n \t\treturn;\n+\t} else if (o->submodule_format == DIFF_SUBMODULE_DIFF &&\n+\t\t (!one->mode || S_ISGITLINK(one->mode)) &&\n+\t\t (!two->mode || S_ISGITLINK(two->mode))) {\n+\t\tshow_submodule_diff(o->file, one->path ? one->path : two->path,\n+\t\t\t\tline_prefix,\n+\t\t\t\tone->oid.hash, two->oid.hash,\n+\t\t\t\ttwo->dirty_submodule,\n+\t\t\t\tmeta, a_prefix, b_prefix, reset);\n+\t\treturn;\n \t}\n \n \tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n@@ -2318,15 +2338,6 @@ static void builtin_diff(const char *name_a,\n \t\ttextconv_two = get_textconv(two);\n \t}\n \n-\tdiff_set_mnemonic_prefix(o, \"a/\", \"b/\");\n-\tif (DIFF_OPT_TST(o, REVERSE_DIFF)) {\n-\t\ta_prefix = o->b_prefix;\n-\t\tb_prefix = o->a_prefix;\n-\t} else {\n-\t\ta_prefix = o->a_prefix;\n-\t\tb_prefix = o->b_prefix;\n-\t}\n-\n \t/* Never use a non-valid filename anywhere if at all possible */\n \tname_a = DIFF_FILE_VALID(one) ? name_a : name_b;\n \tname_b = DIFF_FILE_VALID(two) ? name_b : name_a;\n@@ -3916,7 +3927,7 @@ int diff_opt_parse(struct diff_options *options,\n \t\tDIFF_OPT_SET(options, OVERRIDE_SUBMODULE_CONFIG);\n \t\thandle_ignore_submodules_arg(options, arg);\n \t} else if (!strcmp(arg, \"--submodule\"))\n-\t\tDIFF_OPT_SET(options, SUBMODULE_LOG);\n+\t\toptions->submodule_format = DIFF_SUBMODULE_LOG;\n \telse if (skip_prefix(arg, \"--submodule=\", &arg))\n \t\treturn parse_submodule_opt(options, arg);\n \telse if (skip_prefix(arg, \"--ws-error-highlight=\", &arg))\ndiff --git a/diff.h b/diff.h\nindex 83d0b1ae8580..abaa9a49c036 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -83,7 +83,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n #define DIFF_OPT_DIRSTAT_BY_FILE (1 << 20)\n #define DIFF_OPT_ALLOW_TEXTCONV (1 << 21)\n #define DIFF_OPT_DIFF_FROM_CONTENTS (1 << 22)\n-#define DIFF_OPT_SUBMODULE_LOG (1 << 23)\n+/* (1 << 23) unused */\n #define DIFF_OPT_DIRTY_SUBMODULES (1 << 24)\n #define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 25)\n #define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 26)\n@@ -110,6 +110,12 @@ enum diff_words_type {\n \tDIFF_WORDS_COLOR\n };\n \n+enum diff_submodule_format {\n+\tDIFF_SUBMODULE_SHORT = 0,\n+\tDIFF_SUBMODULE_LOG,\n+\tDIFF_SUBMODULE_DIFF\n+};\n+\n struct diff_options {\n \tconst char *orderfile;\n \tconst char *pickaxe;\n@@ -156,6 +162,7 @@ struct diff_options {\n \tint stat_count;\n \tconst char *word_regex;\n \tenum diff_words_type word_diff;\n+\tenum diff_submodule_format submodule_format;\n \n \t/* this is set by diffcore for DIFF_FORMAT_PATCH */\n \tint found_changes;\ndiff --git a/submodule.c b/submodule.c\nindex 1b5cdfb7e784..b1da68dd49c9 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -333,6 +333,136 @@ static void print_submodule_summary(struct rev_info *rev, FILE *f,\n \tstrbuf_release(&sb);\n }\n \n+void show_submodule_diff(FILE *f, const char *path,\n+\t\tconst char *line_prefix,\n+\t\tunsigned char one[20], unsigned char two[20],\n+\t\tunsigned dirty_submodule, const char *meta,\n+\t\tconst char *a_prefix, const char *b_prefix,\n+\t\tconst char *reset)\n+{\n+\tstruct strbuf submodule_git_dir = STRBUF_INIT, sb = STRBUF_INIT;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tconst char *git_dir;\n+\n+\tif (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) {\n+\t\tfprintf(f, \"%sSubmodule %s contains untracked content\\n\",\n+\t\t\tline_prefix, path);\n+\t}\n+\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED) {\n+\t\tfprintf(f, \"%sSubmodule %s contains modified content\\n\",\n+\t\t\tline_prefix, path);\n+\t}\n+\n+\tstrbuf_addf(&sb, \"%s%sSubmodule %s %s..\",\n+\t\t line_prefix, meta, path,\n+\t\t find_unique_abbrev(one, DEFAULT_ABBREV));\n+\tstrbuf_addf(&sb, \"%s:%s\",\n+\t\t find_unique_abbrev(two, DEFAULT_ABBREV),\n+\t\t reset);\n+\tfwrite(sb.buf, sb.len, 1, f);\n+\n+\tif (is_null_sha1(one))\n+\t\tfprintf(f, \" (new submodule)\");\n+\tif (is_null_sha1(two))\n+\t\tfprintf(f, \" (submodule deleted)\");\n+\n+\t/*\n+\t * We need to determine the most accurate location to call the sub\n+\t * command, and handle the various corner cases involved. We'll first\n+\t * attempt to use the path directly if the submodule is checked out.\n+\t * Then, if that fails, we'll check the standard module location in\n+\t * the git directory. If even this fails, it means we can't do the\n+\t * lookup because the module has not been initialized.\n+\t */\n+\tstrbuf_addf(&submodule_git_dir, \"%s/.git\", path);\n+\tgit_dir = resolve_gitdir(submodule_git_dir.buf);\n+\tif (git_dir) {\n+\t\t/*\n+\t\t * If we can resolve a git dir, this means that the submodule\n+\t\t * has been checked out. In this case, just use the original\n+\t\t * path since we want access to the work tree\n+\t\t */\n+\t\tgit_dir = path;\n+\t} else {\n+\t\t/*\n+\t\t * If we can't resolve a git dir, this means that the\n+\t\t * submodule has not been checked out. In this case, try the\n+\t\t * standard location for modules\n+\t\t */\n+\t\tstrbuf_reset(&submodule_git_dir);\n+\t\tstrbuf_addf(&submodule_git_dir, \"%s/modules/%s\", get_git_dir(), path);\n+\t\tgit_dir = resolve_gitdir(submodule_git_dir.buf);\n+\t\tif (!git_dir) {\n+\t\t\t/*\n+\t\t\t * If we failed to find a git directory here, then the\n+\t\t\t * submodule must not have been initialized. Without\n+\t\t\t * the initialized contents of the submodule, we won't\n+\t\t\t * be able to perform the difference.\n+\t\t\t */\n+\t\t\tfprintf(f, \" (submodule not initialized)\\n\");\n+\t\t\tgoto out;\n+\t\t}\n+\t}\n+\n+\t/*\n+\t * print a newline and flush the file so that the diff output appears\n+\t * starting on its own line\n+\t */\n+\tfprintf(f, \"\\n\");\n+\tfflush(f);\n+\n+\tcp.git_cmd = 1;\n+\tcp.dir = git_dir;\n+\tcp.out = dup(fileno(f));\n+\tcp.no_stdin = 1;\n+\n+\targv_array_push(&cp.args, \"diff\");\n+\targv_array_pushf(&cp.args, \"--line-prefix=%s\", line_prefix);\n+\targv_array_pushf(&cp.args, \"--src-prefix=%s%s/\", a_prefix, path);\n+\targv_array_pushf(&cp.args, \"--dst-prefix=%s%s/\", b_prefix, path);\n+\n+\tif (is_null_sha1(one)) {\n+\t\t/*\n+\t\t * If the origin commit is null, we want to use the empty tree\n+\t\t * so that we see a diff of all the new contents added.\n+\t\t */\n+\t\targv_array_push(&cp.args, EMPTY_TREE_SHA1_HEX);\n+\t} else {\n+\t\t/* Use the old commit as the diff base */\n+\t\targv_array_push(&cp.args, sha1_to_hex(one));\n+\t}\n+\n+\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED) {\n+\t\t/*\n+\t\t * If the submodule has modified contents we want to diff\n+\t\t * against the work tree, so don't add a second parameter.\n+\t\t * This is essentially equivalent of using diff-index instead.\n+\t\t * Note that we can't (easily) show the diff of any untracked\n+\t\t * files.\n+\t\t */\n+\t} else if (is_null_sha1(two)) {\n+\t\t/*\n+\t\t * If new commit is null, we should diff against the empty\n+\t\t * tree, so that we see a diff of all submodule contents\n+\t\t * removed.\n+\t\t */\n+\t\targv_array_push(&cp.args, EMPTY_TREE_SHA1_HEX);\n+\t} else {\n+\t\t/*\n+\t\t * Finally, in other cases use the new commit as the end\n+\t\t * result.\n+\t\t */\n+\t\targv_array_push(&cp.args, sha1_to_hex(two));\n+\t}\n+\n+\tif (run_command(&cp))\n+\t\tfprintf(f, \"(diff failed)\\n\");\n+\n+out:\n+\tstrbuf_release(&submodule_git_dir);\n+\tstrbuf_release(&sb);\n+}\n+\n void show_submodule_summary(FILE *f, const char *path,\n \t\tconst char *line_prefix,\n \t\tunsigned char one[20], unsigned char two[20],\ndiff --git a/submodule.h b/submodule.h\nindex 2af939099819..f32a25c667ab 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -41,6 +41,12 @@ int parse_submodule_update_strategy(const char *value,\n \t\tstruct submodule_update_strategy *dst);\n const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);\n void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);\n+void show_submodule_diff(FILE *f, const char *path,\n+\t\tconst char *line_prefix,\n+\t\tunsigned char one[20], unsigned char two[20],\n+\t\tunsigned dirty_submodule, const char *meta,\n+\t\tconst char *a_prefix, const char *b_prefix,\n+\t\tconst char *reset);\n void show_submodule_summary(FILE *f, const char *path,\n \t\tconst char *line_prefix,\n \t\tunsigned char one[20], unsigned char two[20],\ndiff --git a/t/t4059-diff-submodule-option-diff-format.sh b/t/t4059-diff-submodule-option-diff-format.sh\nnew file mode 100755\nindex 000000000000..d1306b65986c\n--- /dev/null\n+++ b/t/t4059-diff-submodule-option-diff-format.sh\n@@ -0,0 +1,738 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2009 Jens Lehmann, based on t7401 by Ping Yin\n+# Copyright (c) 2011 Alexey Shumkin (+ non-UTF-8 commit encoding tests)\n+# Copyright (c) 2016 Jacob Keller (copy + convert to --submodule=diff)\n+#\n+\n+test_description='Support for diff format verbose submodule difference in git diff\n+\n+This test tries to verify the sanity of --submodule=diff option of git diff.\n+'\n+\n+. ./test-lib.sh\n+\n+# Tested non-UTF-8 encoding\n+test_encoding=\"ISO8859-1\"\n+\n+# String \"added\" in German (translated with Google Translate), encoded in UTF-8,\n+# used in sample commit log messages in add_file() function below.\n+added=$(printf \"hinzugef\\303\\274gt\")\n+add_file () {\n+\t(\n+\t\tcd \"$1\" &&\n+\t\tshift &&\n+\t\tfor name\n+\t\tdo\n+\t\t\techo \"$name\" >\"$name\" &&\n+\t\t\tgit add \"$name\" &&\n+\t\t\ttest_tick &&\n+\t\t\t# \"git commit -m\" would break MinGW, as Windows refuse to pass\n+\t\t\t# $test_encoding encoded parameter to git.\n+\t\t\techo \"Add $name ($added $name)\" | iconv -f utf-8 -t $test_encoding |\n+\t\t\tgit -c \"i18n.commitEncoding=$test_encoding\" commit -F -\n+\t\tdone >/dev/null &&\n+\t\tgit rev-parse --short --verify HEAD\n+\t)\n+}\n+commit_file () {\n+\ttest_tick &&\n+\tgit commit \"$@\" -m \"Commit $*\" >/dev/null\n+}\n+\n+test_create_repo sm1 &&\n+add_file . foo >/dev/null\n+\n+head1=$(add_file sm1 foo1 foo2)\n+fullhead1=$(cd sm1; git rev-parse --verify HEAD)\n+\n+test_expect_success 'added submodule' '\n+\tgit add sm1 &&\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 0000000..1beffeb: (new submodule)\n+\tdiff --git a/sm1/foo1 b/sm1/foo1\n+\tnew file mode 100644\n+\tindex 0000000..1715acd\n+\t--- /dev/null\n+\t+++ b/sm1/foo1\n+\t@@ -0,0 +1 @@\n+\t+foo1\n+\tdiff --git a/sm1/foo2 b/sm1/foo2\n+\tnew file mode 100644\n+\tindex 0000000..54b060e\n+\t--- /dev/null\n+\t+++ b/sm1/foo2\n+\t@@ -0,0 +1 @@\n+\t+foo2\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'added submodule, set diff.submodule' '\n+\tgit config diff.submodule log &&\n+\tgit add sm1 &&\n+\tgit diff --cached >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 0000000...$head1 (new submodule)\n+\tEOF\n+\tgit config --unset diff.submodule &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success '--submodule=short overrides diff.submodule' '\n+\ttest_config diff.submodule log &&\n+\tgit add sm1 &&\n+\tgit diff --submodule=short --cached >actual &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/sm1 b/sm1\n+\tnew file mode 160000\n+\tindex 0000000..$head1\n+\t--- /dev/null\n+\t+++ b/sm1\n+\t@@ -0,0 +1 @@\n+\t+Subproject commit $fullhead1\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'diff.submodule does not affect plumbing' '\n+\ttest_config diff.submodule log &&\n+\tgit diff-index -p HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/sm1 b/sm1\n+\tnew file mode 160000\n+\tindex 0000000..$head1\n+\t--- /dev/null\n+\t+++ b/sm1\n+\t@@ -0,0 +1 @@\n+\t+Subproject commit $fullhead1\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+commit_file sm1 &&\n+head2=$(add_file sm1 foo3)\n+\n+test_expect_success 'modified submodule(forward)' '\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 1beffeb..30b9670:\n+\tdiff --git a/sm1/foo3 b/sm1/foo3\n+\tnew file mode 100644\n+\tindex 0000000..c1ec6c6\n+\t--- /dev/null\n+\t+++ b/sm1/foo3\n+\t@@ -0,0 +1 @@\n+\t+foo3\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'modified submodule(forward)' '\n+\tgit diff --submodule=diff >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 1beffeb..30b9670:\n+\tdiff --git a/sm1/foo3 b/sm1/foo3\n+\tnew file mode 100644\n+\tindex 0000000..c1ec6c6\n+\t--- /dev/null\n+\t+++ b/sm1/foo3\n+\t@@ -0,0 +1 @@\n+\t+foo3\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'modified submodule(forward) --submodule' '\n+\tgit diff --submodule >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 $head1..$head2:\n+\t > Add foo3 ($added foo3)\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+fullhead2=$(cd sm1; git rev-parse --verify HEAD)\n+test_expect_success 'modified submodule(forward) --submodule=short' '\n+\tgit diff --submodule=short >actual &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/sm1 b/sm1\n+\tindex $head1..$head2 160000\n+\t--- a/sm1\n+\t+++ b/sm1\n+\t@@ -1 +1 @@\n+\t-Subproject commit $fullhead1\n+\t+Subproject commit $fullhead2\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+commit_file sm1 &&\n+head3=$(\n+\tcd sm1 &&\n+\tgit reset --hard HEAD~2 >/dev/null &&\n+\tgit rev-parse --short --verify HEAD\n+)\n+\n+test_expect_success 'modified submodule(backward)' '\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 30b9670..dafb207:\n+\tdiff --git a/sm1/foo2 b/sm1/foo2\n+\tdeleted file mode 100644\n+\tindex 54b060e..0000000\n+\t--- a/sm1/foo2\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-foo2\n+\tdiff --git a/sm1/foo3 b/sm1/foo3\n+\tdeleted file mode 100644\n+\tindex c1ec6c6..0000000\n+\t--- a/sm1/foo3\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-foo3\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+head4=$(add_file sm1 foo4 foo5)\n+test_expect_success 'modified submodule(backward and forward)' '\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 30b9670..d176589:\n+\tdiff --git a/sm1/foo2 b/sm1/foo2\n+\tdeleted file mode 100644\n+\tindex 54b060e..0000000\n+\t--- a/sm1/foo2\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-foo2\n+\tdiff --git a/sm1/foo3 b/sm1/foo3\n+\tdeleted file mode 100644\n+\tindex c1ec6c6..0000000\n+\t--- a/sm1/foo3\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-foo3\n+\tdiff --git a/sm1/foo4 b/sm1/foo4\n+\tnew file mode 100644\n+\tindex 0000000..a0016db\n+\t--- /dev/null\n+\t+++ b/sm1/foo4\n+\t@@ -0,0 +1 @@\n+\t+foo4\n+\tdiff --git a/sm1/foo5 b/sm1/foo5\n+\tnew file mode 100644\n+\tindex 0000000..d6f2413\n+\t--- /dev/null\n+\t+++ b/sm1/foo5\n+\t@@ -0,0 +1 @@\n+\t+foo5\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+commit_file sm1 &&\n+mv sm1 sm1-bak &&\n+echo sm1 >sm1 &&\n+head5=$(git hash-object sm1 | cut -c1-7) &&\n+git add sm1 &&\n+rm -f sm1 &&\n+mv sm1-bak sm1\n+\n+test_expect_success 'typechanged submodule(submodule->blob), --cached' '\n+\tgit diff --submodule=diff --cached >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 d176589..0000000: (submodule deleted)\n+\tdiff --git a/sm1/foo1 b/sm1/foo1\n+\tdeleted file mode 100644\n+\tindex 1715acd..0000000\n+\t--- a/sm1/foo1\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-foo1\n+\tdiff --git a/sm1/foo4 b/sm1/foo4\n+\tdeleted file mode 100644\n+\tindex a0016db..0000000\n+\t--- a/sm1/foo4\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-foo4\n+\tdiff --git a/sm1/foo5 b/sm1/foo5\n+\tdeleted file mode 100644\n+\tindex d6f2413..0000000\n+\t--- a/sm1/foo5\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-foo5\n+\tdiff --git a/sm1 b/sm1\n+\tnew file mode 100644\n+\tindex 0000000..9da5fb8\n+\t--- /dev/null\n+\t+++ b/sm1\n+\t@@ -0,0 +1 @@\n+\t+sm1\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'typechanged submodule(submodule->blob)' '\n+\tgit diff --submodule=diff >actual &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/sm1 b/sm1\n+\tdeleted file mode 100644\n+\tindex 9da5fb8..0000000\n+\t--- a/sm1\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-sm1\n+\tSubmodule sm1 0000000..d176589: (new submodule)\n+\tdiff --git a/sm1/foo1 b/sm1/foo1\n+\tnew file mode 100644\n+\tindex 0000000..1715acd\n+\t--- /dev/null\n+\t+++ b/sm1/foo1\n+\t@@ -0,0 +1 @@\n+\t+foo1\n+\tdiff --git a/sm1/foo4 b/sm1/foo4\n+\tnew file mode 100644\n+\tindex 0000000..a0016db\n+\t--- /dev/null\n+\t+++ b/sm1/foo4\n+\t@@ -0,0 +1 @@\n+\t+foo4\n+\tdiff --git a/sm1/foo5 b/sm1/foo5\n+\tnew file mode 100644\n+\tindex 0000000..d6f2413\n+\t--- /dev/null\n+\t+++ b/sm1/foo5\n+\t@@ -0,0 +1 @@\n+\t+foo5\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+rm -rf sm1 &&\n+git checkout-index sm1\n+test_expect_success 'typechanged submodule(submodule->blob)' '\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 $head4..0000000: (submodule deleted) (submodule not initialized)\n+\tdiff --git a/sm1 b/sm1\n+\tnew file mode 100644\n+\tindex 0000000..$head5\n+\t--- /dev/null\n+\t+++ b/sm1\n+\t@@ -0,0 +1 @@\n+\t+sm1\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+rm -f sm1 &&\n+test_create_repo sm1 &&\n+head6=$(add_file sm1 foo6 foo7)\n+fullhead6=$(cd sm1; git rev-parse --verify HEAD)\n+test_expect_success 'nonexistent commit' '\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 d176589..17243c9:\n+\t(diff failed)\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+commit_file\n+test_expect_success 'typechanged submodule(blob->submodule)' '\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/sm1 b/sm1\n+\tdeleted file mode 100644\n+\tindex 9da5fb8..0000000\n+\t--- a/sm1\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-sm1\n+\tSubmodule sm1 0000000..17243c9: (new submodule)\n+\tdiff --git a/sm1/foo6 b/sm1/foo6\n+\tnew file mode 100644\n+\tindex 0000000..462398b\n+\t--- /dev/null\n+\t+++ b/sm1/foo6\n+\t@@ -0,0 +1 @@\n+\t+foo6\n+\tdiff --git a/sm1/foo7 b/sm1/foo7\n+\tnew file mode 100644\n+\tindex 0000000..6e9262c\n+\t--- /dev/null\n+\t+++ b/sm1/foo7\n+\t@@ -0,0 +1 @@\n+\t+foo7\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+commit_file sm1 &&\n+test_expect_success 'submodule is up to date' '\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'submodule contains untracked content' '\n+\techo new > sm1/new-file &&\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 contains untracked content\n+\tSubmodule sm1 17243c9..17243c9:\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'submodule contains untracked content (untracked ignored)' '\n+\tgit diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&\n+\t! test -s actual\n+'\n+\n+test_expect_success 'submodule contains untracked content (dirty ignored)' '\n+\tgit diff-index -p --ignore-submodules=dirty --submodule=diff HEAD >actual &&\n+\t! test -s actual\n+'\n+\n+test_expect_success 'submodule contains untracked content (all ignored)' '\n+\tgit diff-index -p --ignore-submodules=all --submodule=diff HEAD >actual &&\n+\t! test -s actual\n+'\n+\n+test_expect_success 'submodule contains untracked and modified content' '\n+\techo new > sm1/foo6 &&\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 contains untracked content\n+\tSubmodule sm1 contains modified content\n+\tSubmodule sm1 17243c9..17243c9:\n+\tdiff --git a/sm1/foo6 b/sm1/foo6\n+\tindex 462398b..3e75765 100644\n+\t--- a/sm1/foo6\n+\t+++ b/sm1/foo6\n+\t@@ -1 +1 @@\n+\t-foo6\n+\t+new\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+# NOT OK\n+test_expect_success 'submodule contains untracked and modified content (untracked ignored)' '\n+\techo new > sm1/foo6 &&\n+\tgit diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 contains modified content\n+\tSubmodule sm1 17243c9..17243c9:\n+\tdiff --git a/sm1/foo6 b/sm1/foo6\n+\tindex 462398b..3e75765 100644\n+\t--- a/sm1/foo6\n+\t+++ b/sm1/foo6\n+\t@@ -1 +1 @@\n+\t-foo6\n+\t+new\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'submodule contains untracked and modified content (dirty ignored)' '\n+\techo new > sm1/foo6 &&\n+\tgit diff-index -p --ignore-submodules=dirty --submodule=diff HEAD >actual &&\n+\t! test -s actual\n+'\n+\n+test_expect_success 'submodule contains untracked and modified content (all ignored)' '\n+\techo new > sm1/foo6 &&\n+\tgit diff-index -p --ignore-submodules --submodule=diff HEAD >actual &&\n+\t! test -s actual\n+'\n+\n+test_expect_success 'submodule contains modified content' '\n+\trm -f sm1/new-file &&\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 contains modified content\n+\tSubmodule sm1 17243c9..17243c9:\n+\tdiff --git a/sm1/foo6 b/sm1/foo6\n+\tindex 462398b..3e75765 100644\n+\t--- a/sm1/foo6\n+\t+++ b/sm1/foo6\n+\t@@ -1 +1 @@\n+\t-foo6\n+\t+new\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+(cd sm1; git commit -mchange foo6 >/dev/null) &&\n+head8=$(cd sm1; git rev-parse --short --verify HEAD) &&\n+test_expect_success 'submodule is modified' '\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 17243c9..cfce562:\n+\tdiff --git a/sm1/foo6 b/sm1/foo6\n+\tindex 462398b..3e75765 100644\n+\t--- a/sm1/foo6\n+\t+++ b/sm1/foo6\n+\t@@ -1 +1 @@\n+\t-foo6\n+\t+new\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'modified submodule contains untracked content' '\n+\techo new > sm1/new-file &&\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 contains untracked content\n+\tSubmodule sm1 17243c9..cfce562:\n+\tdiff --git a/sm1/foo6 b/sm1/foo6\n+\tindex 462398b..3e75765 100644\n+\t--- a/sm1/foo6\n+\t+++ b/sm1/foo6\n+\t@@ -1 +1 @@\n+\t-foo6\n+\t+new\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'modified submodule contains untracked content (untracked ignored)' '\n+\tgit diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 17243c9..cfce562:\n+\tdiff --git a/sm1/foo6 b/sm1/foo6\n+\tindex 462398b..3e75765 100644\n+\t--- a/sm1/foo6\n+\t+++ b/sm1/foo6\n+\t@@ -1 +1 @@\n+\t-foo6\n+\t+new\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'modified submodule contains untracked content (dirty ignored)' '\n+\tgit diff-index -p --ignore-submodules=dirty --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 17243c9..cfce562:\n+\tdiff --git a/sm1/foo6 b/sm1/foo6\n+\tindex 462398b..3e75765 100644\n+\t--- a/sm1/foo6\n+\t+++ b/sm1/foo6\n+\t@@ -1 +1 @@\n+\t-foo6\n+\t+new\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'modified submodule contains untracked content (all ignored)' '\n+\tgit diff-index -p --ignore-submodules=all --submodule=diff HEAD >actual &&\n+\t! test -s actual\n+'\n+\n+test_expect_success 'modified submodule contains untracked and modified content' '\n+\techo modification >> sm1/foo6 &&\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 contains untracked content\n+\tSubmodule sm1 contains modified content\n+\tSubmodule sm1 17243c9..cfce562:\n+\tdiff --git a/sm1/foo6 b/sm1/foo6\n+\tindex 462398b..dfda541 100644\n+\t--- a/sm1/foo6\n+\t+++ b/sm1/foo6\n+\t@@ -1 +1,2 @@\n+\t-foo6\n+\t+new\n+\t+modification\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'modified submodule contains untracked and modified content (untracked ignored)' '\n+\techo modification >> sm1/foo6 &&\n+\tgit diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 contains modified content\n+\tSubmodule sm1 17243c9..cfce562:\n+\tdiff --git a/sm1/foo6 b/sm1/foo6\n+\tindex 462398b..e20e2d9 100644\n+\t--- a/sm1/foo6\n+\t+++ b/sm1/foo6\n+\t@@ -1 +1,3 @@\n+\t-foo6\n+\t+new\n+\t+modification\n+\t+modification\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'modified submodule contains untracked and modified content (dirty ignored)' '\n+\techo modification >> sm1/foo6 &&\n+\tgit diff-index -p --ignore-submodules=dirty --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 17243c9..cfce562:\n+\tdiff --git a/sm1/foo6 b/sm1/foo6\n+\tindex 462398b..3e75765 100644\n+\t--- a/sm1/foo6\n+\t+++ b/sm1/foo6\n+\t@@ -1 +1 @@\n+\t-foo6\n+\t+new\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'modified submodule contains untracked and modified content (all ignored)' '\n+\techo modification >> sm1/foo6 &&\n+\tgit diff-index -p --ignore-submodules --submodule=diff HEAD >actual &&\n+\t! test -s actual\n+'\n+\n+# NOT OK\n+test_expect_success 'modified submodule contains modified content' '\n+\trm -f sm1/new-file &&\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 contains modified content\n+\tSubmodule sm1 17243c9..cfce562:\n+\tdiff --git a/sm1/foo6 b/sm1/foo6\n+\tindex 462398b..ac466ca 100644\n+\t--- a/sm1/foo6\n+\t+++ b/sm1/foo6\n+\t@@ -1 +1,5 @@\n+\t-foo6\n+\t+new\n+\t+modification\n+\t+modification\n+\t+modification\n+\t+modification\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+rm -rf sm1\n+test_expect_success 'deleted submodule' '\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 17243c9..0000000: (submodule deleted) (submodule not initialized)\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_create_repo sm2 &&\n+head7=$(add_file sm2 foo8 foo9) &&\n+git add sm2\n+\n+test_expect_success 'multiple submodules' '\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 17243c9..0000000: (submodule deleted) (submodule not initialized)\n+\tSubmodule sm2 0000000..a5a65c9: (new submodule)\n+\tdiff --git a/sm2/foo8 b/sm2/foo8\n+\tnew file mode 100644\n+\tindex 0000000..db9916b\n+\t--- /dev/null\n+\t+++ b/sm2/foo8\n+\t@@ -0,0 +1 @@\n+\t+foo8\n+\tdiff --git a/sm2/foo9 b/sm2/foo9\n+\tnew file mode 100644\n+\tindex 0000000..9c3b4f6\n+\t--- /dev/null\n+\t+++ b/sm2/foo9\n+\t@@ -0,0 +1 @@\n+\t+foo9\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'path filter' '\n+\tgit diff-index -p --submodule=diff HEAD sm2 >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm2 0000000..a5a65c9: (new submodule)\n+\tdiff --git a/sm2/foo8 b/sm2/foo8\n+\tnew file mode 100644\n+\tindex 0000000..db9916b\n+\t--- /dev/null\n+\t+++ b/sm2/foo8\n+\t@@ -0,0 +1 @@\n+\t+foo8\n+\tdiff --git a/sm2/foo9 b/sm2/foo9\n+\tnew file mode 100644\n+\tindex 0000000..9c3b4f6\n+\t--- /dev/null\n+\t+++ b/sm2/foo9\n+\t@@ -0,0 +1 @@\n+\t+foo9\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+commit_file sm2\n+test_expect_success 'given commit' '\n+\tgit diff-index -p --submodule=diff HEAD^ >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 17243c9..0000000: (submodule deleted) (submodule not initialized)\n+\tSubmodule sm2 0000000..a5a65c9: (new submodule)\n+\tdiff --git a/sm2/foo8 b/sm2/foo8\n+\tnew file mode 100644\n+\tindex 0000000..db9916b\n+\t--- /dev/null\n+\t+++ b/sm2/foo8\n+\t@@ -0,0 +1 @@\n+\t+foo8\n+\tdiff --git a/sm2/foo9 b/sm2/foo9\n+\tnew file mode 100644\n+\tindex 0000000..9c3b4f6\n+\t--- /dev/null\n+\t+++ b/sm2/foo9\n+\t@@ -0,0 +1 @@\n+\t+foo9\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'setup .git file for sm2' '\n+\t(cd sm2 &&\n+\t REAL=\"$(pwd)/../.real\" &&\n+\t mv .git \"$REAL\"\n+\t echo \"gitdir: $REAL\" >.git)\n+'\n+\n+test_expect_success 'diff --submodule=diff with .git file' '\n+\tgit diff --submodule=diff HEAD^ >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 17243c9..0000000: (submodule deleted) (submodule not initialized)\n+\tSubmodule sm2 0000000..a5a65c9: (new submodule)\n+\tdiff --git a/sm2/foo8 b/sm2/foo8\n+\tnew file mode 100644\n+\tindex 0000000..db9916b\n+\t--- /dev/null\n+\t+++ b/sm2/foo8\n+\t@@ -0,0 +1 @@\n+\t+foo8\n+\tdiff --git a/sm2/foo9 b/sm2/foo9\n+\tnew file mode 100644\n+\tindex 0000000..9c3b4f6\n+\t--- /dev/null\n+\t+++ b/sm2/foo9\n+\t@@ -0,0 +1 @@\n+\t+foo9\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_done\n-- \n2.9.2.873.g47c31b4\n\n"},{"id":"299492","messageId":"xmqqpop8d21j.fsf@gitster.mtv.corp.google.com","threadId":"43859","inReplyTo":"20160815230702.30817-2-jacob.e.keller@intel.com","subject":"Re: [PATCH v6 1/3] diff.c: remove output_prefix_length field","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-16T18:03:20Z","receivedAt":"2016-08-16T18:03:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> From: Junio C Hamano \n\nThanks. I had quite a many typoes in this one.\n\n> \"diff/log --stat\" has a logic that determines the display columns\n> available for the diffstat part of the output and apportions it for\n> pathnames and diffstat graph automatically.\n>\n> 5e71a84a (Add output_prefix_length to diff_options, 2012-04-16)\n> added the output_prefix_length field to diff_options structure to\n> allow this logic subtract the display columns used for display the\n\ns/^.*$/allow this logic to subtract the display columns used for the/\n\n> history graph part from the total \"terminal width\"; this matters\n> when the \"git log --graph -p\" option is in use.\n>\n> The field be set to the number of display columns needed to show the\n\ns/field be/field must be/\n\n> output from the output_prefix() callback. Any new output_prefix()\n> callback must also update the field accordingly, which is error\n\n\"output from the output_prefix() callback, which is error\"\n\n> prone.\n\nNo need to resend; I had it with the above fix in 'pu' for a few\ndays already ;-)\n"},{"id":"299493","messageId":"CA+P7+xqG_3KS7tGS_DAuDx5aVTkfiO5k5biECcbbvA06PtXrhA@mail.gmail.com","threadId":"43859","inReplyTo":"xmqqpop8d21j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 1/3] diff.c: remove output_prefix_length field","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-16T18:08:37Z","receivedAt":"2016-08-16T18:09:03Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Aug 16, 2016 at 11:03 AM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>\n>> From: Junio C Hamano \n>\n> Thanks. I had quite a many typoes in this one.\n>\n>\n> No need to resend; I had it with the above fix in 'pu' for a few\n> days already ;-)\n\nThanks,\nJake\n"},{"id":"299494","messageId":"xmqqlgzwd14w.fsf@gitster.mtv.corp.google.com","threadId":"43859","inReplyTo":"20160815230702.30817-3-jacob.e.keller@intel.com","subject":"Re: [PATCH v6 2/3] graph: add support for --line-prefix on all graph-aware output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-16T18:22:55Z","receivedAt":"2016-08-16T18:23:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> To make this work, we have to fix a few bugs in the graph API that force\n> graph_show_commit_msg to be used only when you have a valid graph.\n> Additionally, we extend the default_diff_output_prefix handler to work\n> even when no graph is enabled.\n>\n> This is somewhat of a hack on top of the graph API, but I think it\n> should be acceptable here.\n\nUnlike the opt-prefix-length I removed in 1/3, the length of the\nline-prefix will never change during the lifetime of a single\ndiff_options struct, so it might turn out that repeated strlen()\non it for each and every line output is wasteful.\n\nOther than that, I didn't spot anything immediately questionable.\n\nThanks.\n"},{"id":"299497","messageId":"xmqqh9akczyp.fsf@gitster.mtv.corp.google.com","threadId":"43859","inReplyTo":"20160815230702.30817-4-jacob.e.keller@intel.com","subject":"Re: [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-16T18:48:14Z","receivedAt":"2016-08-16T18:55:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\n> index d5a5b17d5088..f5d693afad6c 100644\n> --- a/Documentation/diff-config.txt\n> +++ b/Documentation/diff-config.txt\n> @@ -123,7 +123,8 @@ diff.suppressBlankEmpty::\n> diff.submodule::\n> \tSpecify the format in which differences in submodules are\n> \tshown. The \"log\" format lists the commits in the range like\n> -\tlinkgit:git-submodule[1] `summary` does. The \"short\" format\n> +\tlinkgit:git-submodule[1] `summary` does. The \"diff\" format shows the\n> +\tdiff between the beginning and end of the range. The \"short\" format\n> \tformat just shows the names of the commits at the beginning\n> \tand end of the range. Defaults to short.\n\nIt would be much better to describe the default one first and then\nmore involved one next, no? That would also match what the change\nto \"diff-options\" in this patch does (can be seen below).\n\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index cc4342e2034d..d3ca4ad2c2c5 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -215,8 +215,11 @@ any of those replacements occurred.\n> \tthe commits in the range like linkgit:git-submodule[1] `summary` does.\n> \tOmitting the `--submodule` option or specifying `--submodule=short`,\n> \tuses the 'short' format. This format just shows the names of the commits\n> -\tat the beginning and end of the range. Can be tweaked via the\n> -\t`diff.submodule` configuration variable.\n> +\tat the beginning and end of the range. When `--submodule=diff` is\n> +\tgiven, the 'diff' format is used. This format shows the diff between\n> +\tthe old and new submodule commmit from the perspective of the\n> +\tsubmodule. Defaults to `diff.submodule` or 'short' if the config\n> +\toption is unset.\n\n> @@ -2311,6 +2322,15 @@ static void builtin_diff(const char *name_a,\n> \t\t\t\ttwo->dirty_submodule,\n> \t\t\t\tmeta, del, add, reset);\n> \t\treturn;\n> +\t} else if (o->submodule_format == DIFF_SUBMODULE_DIFF &&\n> +\t\t (!one->mode || S_ISGITLINK(one->mode)) &&\n> +\t\t (!two->mode || S_ISGITLINK(two->mode))) {\n> +\t\tshow_submodule_diff(o->file, one->path ? one->path : two->path,\n> +\t\t\t\tline_prefix,\n> +\t\t\t\tone->oid.hash, two->oid.hash,\n> +\t\t\t\ttwo->dirty_submodule,\n> +\t\t\t\tmeta, a_prefix, b_prefix, reset);\n> +\t\treturn;\n> \t}\n\nThe \"either missing or is submodule\" check used here is being\nconsistent with the existing \"submodule=log\" case. Good.\n\n> diff --git a/submodule.c b/submodule.c\n> index 1b5cdfb7e784..b1da68dd49c9 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -333,6 +333,136 @@ static void print_submodule_summary(struct rev_info *rev, FILE *f,\n> \tstrbuf_release(&sb);\n> }\n> \n> +void show_submodule_diff(FILE *f, const char *path,\n> +\t\tconst char *line_prefix,\n> +\t\tunsigned char one[20], unsigned char two[20],\n> +\t\tunsigned dirty_submodule, const char *meta,\n> +\t\tconst char *a_prefix, const char *b_prefix,\n> +\t\tconst char *reset)\n> +{\n> +\tstruct strbuf submodule_git_dir = STRBUF_INIT, sb = STRBUF_INIT;\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tconst char *git_dir;\n> +\n> +\tif (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) {\n> +\t\tfprintf(f, \"%sSubmodule %s contains untracked content\\n\",\n> +\t\t\tline_prefix, path);\n> +\t}\n> +\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED) {\n> +\t\tfprintf(f, \"%sSubmodule %s contains modified content\\n\",\n> +\t\t\tline_prefix, path);\n> +\t}\n> +\n> +\tstrbuf_addf(&sb, \"%s%sSubmodule %s %s..\",\n> +\t\t line_prefix, meta, path,\n> +\t\t find_unique_abbrev(one, DEFAULT_ABBREV));\n> +\tstrbuf_addf(&sb, \"%s:%s\",\n> +\t\t find_unique_abbrev(two, DEFAULT_ABBREV),\n> +\t\t reset);\n> +\tfwrite(sb.buf, sb.len, 1, f);\n> +\n> +\tif (is_null_sha1(one))\n> +\t\tfprintf(f, \" (new submodule)\");\n> +\tif (is_null_sha1(two))\n> +\t\tfprintf(f, \" (submodule deleted)\");\n\nThese messages are in sync with show_submodule_summary() that is\nused in --submodule=log codepath. Good.\n\n> +\t/*\n> +\t * We need to determine the most accurate location to call the sub\n> +\t * command, and handle the various corner cases involved. We'll first\n> +\t * attempt to use the path directly if the submodule is checked out.\n> +\t * Then, if that fails, we'll check the standard module location in\n> +\t * the git directory. If even this fails, it means we can't do the\n> +\t * lookup because the module has not been initialized.\n> +\t */\n\nThis is more elaborate than what show_submodule_summary() does,\nisn't it? Would it make the patch series (and the resulting code)\nmore understandable if you used the same code by refactoring these\ntwo? If so, I wonder if it makes sense to split 3/3 into a few\nseparate steps:\n\n * Update the internal \"--submodule=\" handling without adding\n the \"--submodule=diff\" and show_submodule_diff() function.\n\n * Refactor the determination of the submodule status (i.e. does it\n even have a clone? where is its repository? etc.) from existing\n show_submodule_summary() into a separate helper function.\n\n * Make that helper function more elaborate like what you do here,\n and update show_submodule_summary(). I think the state\n show_submodule_summary() calls \"not checked out\" corresponds to\n what you say \"not initialized\" below, and they should share the\n same logic to determine that the submodule is in that state, and\n share the same message, for example.\n\n * Introduce \"--submodule=diff\", and show_submodule_diff() function;\n the latter would use the helper function prepared in the previous\n step.\n\nperhaps?\n\n> +\tstrbuf_addf(&submodule_git_dir, \"%s/.git\", path);\n> +\tgit_dir = resolve_gitdir(submodule_git_dir.buf);\n> +\tif (git_dir) {\n> +\t\t/*\n> +\t\t * If we can resolve a git dir, this means that the submodule\n> +\t\t * has been checked out. In this case, just use the original\n> +\t\t * path since we want access to the work tree\n> +\t\t */\n> +\t\tgit_dir = path;\n> +\t} else {\n> +\t\t/*\n> +\t\t * If we can't resolve a git dir, this means that the\n> +\t\t * submodule has not been checked out. In this case, try the\n> +\t\t * standard location for modules\n> +\t\t */\n> +\t\tstrbuf_reset(&submodule_git_dir);\n> +\t\tstrbuf_addf(&submodule_git_dir, \"%s/modules/%s\", get_git_dir(), path);\n> +\t\tgit_dir = resolve_gitdir(submodule_git_dir.buf);\n> +\t\tif (!git_dir) {\n> +\t\t\t/*\n> +\t\t\t * If we failed to find a git directory here, then the\n> +\t\t\t * submodule must not have been initialized. Without\n> +\t\t\t * the initialized contents of the submodule, we won't\n> +\t\t\t * be able to perform the difference.\n> +\t\t\t */\n> +\t\t\tfprintf(f, \" (submodule not initialized)\\n\");\n> +\t\t\tgoto out;\n> +\t\t}\n> +\t}\n> +\n> +\t/*\n> +\t * print a newline and flush the file so that the diff output appears\n> +\t * starting on its own line\n> +\t */\n> +\tfprintf(f, \"\\n\");\n> +\tfflush(f);\n> +\n> +\tcp.git_cmd = 1;\n> +\tcp.dir = git_dir;\n> +\tcp.out = dup(fileno(f));\n> +\tcp.no_stdin = 1;\n> +\n> +\targv_array_push(&cp.args, \"diff\");\n> +\targv_array_pushf(&cp.args, \"--line-prefix=%s\", line_prefix);\n> +\targv_array_pushf(&cp.args, \"--src-prefix=%s%s/\", a_prefix, path);\n> +\targv_array_pushf(&cp.args, \"--dst-prefix=%s%s/\", b_prefix, path);\n> +\n> +\tif (is_null_sha1(one)) {\n> +\t\t/*\n> +\t\t * If the origin commit is null, we want to use the empty tree\n> +\t\t * so that we see a diff of all the new contents added.\n> +\t\t */\n> +\t\targv_array_push(&cp.args, EMPTY_TREE_SHA1_HEX);\n> +\t} else {\n> +\t\t/* Use the old commit as the diff base */\n> +\t\targv_array_push(&cp.args, sha1_to_hex(one));\n> +\t}\n> +\n> +\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED) {\n> +\t\t/*\n> +\t\t * If the submodule has modified contents we want to diff\n> +\t\t * against the work tree, so don't add a second parameter.\n> +\t\t * This is essentially equivalent of using diff-index instead.\n> +\t\t * Note that we can't (easily) show the diff of any untracked\n> +\t\t * files.\n> +\t\t */\n> +\t} else if (is_null_sha1(two)) {\n\nIt is safer to have ';' inside the empty if(){} block to make sure\nthat one empty statement exists there. It makes the intention of\nthe code clearer, too.\n\nI am debating myself if this is a good thing to do, though. The\nsubmodule is a separate project for a reason, and there is a reason\nwhy the changes haven't been committed. When asking \"what's different\nbetween these two superproject states?\", should the user really see\nthese uncommitted changes?\n\nThanks.\n"},{"id":"299501","messageId":"CA+P7+xqc_WwzjUnF5P4arBhBqgRbtXyKC9QWtRJ3+fmx0Q2+oA@mail.gmail.com","threadId":"43859","inReplyTo":"xmqqh9akczyp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-16T20:25:46Z","receivedAt":"2016-08-16T20:27:23Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Aug 16, 2016 at 11:48 AM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>\n>> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\n>> index d5a5b17d5088..f5d693afad6c 100644\n>> --- a/Documentation/diff-config.txt\n>> +++ b/Documentation/diff-config.txt\n>> @@ -123,7 +123,8 @@ diff.suppressBlankEmpty::\n>> diff.submodule::\n>> Specify the format in which differences in submodules are\n>> shown. The \"log\" format lists the commits in the range like\n>> - linkgit:git-submodule[1] `summary` does. The \"short\" format\n>> + linkgit:git-submodule[1] `summary` does. The \"diff\" format shows the\n>> + diff between the beginning and end of the range. The \"short\" format\n>> format just shows the names of the commits at the beginning\n>> and end of the range. Defaults to short.\n>\n> It would be much better to describe the default one first and then\n> more involved one next, no? That would also match what the change\n> to \"diff-options\" in this patch does (can be seen below).\n>\n\nThe main thing is that \"--submodule\" alone means \"use the log format\"\nso I think that's why it went first. I can reword this to make it more\nclear how this works.\n\nThanks,\nJake\n\n>> @@ -2311,6 +2322,15 @@ static void builtin_diff(const char *name_a,\n>> two->dirty_submodule,\n>> meta, del, add, reset);\n>> return;\n>> + } else if (o->submodule_format == DIFF_SUBMODULE_DIFF &&\n>> + (!one->mode || S_ISGITLINK(one->mode)) &&\n>> + (!two->mode || S_ISGITLINK(two->mode))) {\n>> + show_submodule_diff(o->file, one->path ? one->path : two->path,\n>> + line_prefix,\n>> + one->oid.hash, two->oid.hash,\n>> + two->dirty_submodule,\n>> + meta, a_prefix, b_prefix, reset);\n>> + return;\n>> }\n>\n> The \"either missing or is submodule\" check used here is being\n> consistent with the existing \"submodule=log\" case. Good.\n>\n>> diff --git a/submodule.c b/submodule.c\n>> index 1b5cdfb7e784..b1da68dd49c9 100644\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -333,6 +333,136 @@ static void print_submodule_summary(struct rev_info *rev, FILE *f,\n>> strbuf_release(&sb);\n>> }\n>>\n>> +void show_submodule_diff(FILE *f, const char *path,\n>> + const char *line_prefix,\n>> + unsigned char one[20], unsigned char two[20],\n>> + unsigned dirty_submodule, const char *meta,\n>> + const char *a_prefix, const char *b_prefix,\n>> + const char *reset)\n>> +{\n>> + struct strbuf submodule_git_dir = STRBUF_INIT, sb = STRBUF_INIT;\n>> + struct child_process cp = CHILD_PROCESS_INIT;\n>> + const char *git_dir;\n>> +\n>> + if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) {\n>> + fprintf(f, \"%sSubmodule %s contains untracked content\\n\",\n>> + line_prefix, path);\n>> + }\n>> + if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED) {\n>> + fprintf(f, \"%sSubmodule %s contains modified content\\n\",\n>> + line_prefix, path);\n>> + }\n>> +\n>> + strbuf_addf(&sb, \"%s%sSubmodule %s %s..\",\n>> + line_prefix, meta, path,\n>> + find_unique_abbrev(one, DEFAULT_ABBREV));\n>> + strbuf_addf(&sb, \"%s:%s\",\n>> + find_unique_abbrev(two, DEFAULT_ABBREV),\n>> + reset);\n>> + fwrite(sb.buf, sb.len, 1, f);\n>> +\n>> + if (is_null_sha1(one))\n>> + fprintf(f, \" (new submodule)\");\n>> + if (is_null_sha1(two))\n>> + fprintf(f, \" (submodule deleted)\");\n>\n> These messages are in sync with show_submodule_summary() that is\n> used in --submodule=log codepath. Good.\n>\n\nThey're not exactly the same due to some ways of splitting up new lines.\n\n>> + /*\n>> + * We need to determine the most accurate location to call the sub\n>> + * command, and handle the various corner cases involved. We'll first\n>> + * attempt to use the path directly if the submodule is checked out.\n>> + * Then, if that fails, we'll check the standard module location in\n>> + * the git directory. If even this fails, it means we can't do the\n>> + * lookup because the module has not been initialized.\n>> + */\n>\n> This is more elaborate than what show_submodule_summary() does,\n> isn't it? Would it make the patch series (and the resulting code)\n> more understandable if you used the same code by refactoring these\n> two? If so, I wonder if it makes sense to split 3/3 into a few\n> separate steps:\n\nThe show_submodule_summary just uses \"add_submodule_odb\" which adds\nthe submodule as an alternate source of objects, if I understand\ncorrectly.\n\n>\n> * Update the internal \"--submodule=\" handling without adding\n> the \"--submodule=diff\" and show_submodule_diff() function.\n>\n> * Refactor the determination of the submodule status (i.e. does it\n> even have a clone? where is its repository? etc.) from existing\n> show_submodule_summary() into a separate helper function.\n>\n> * Make that helper function more elaborate like what you do here,\n> and update show_submodule_summary(). I think the state\n> show_submodule_summary() calls \"not checked out\" corresponds to\n> what you say \"not initialized\" below, and they should share the\n> same logic to determine that the submodule is in that state, and\n> share the same message, for example.\n\nMakes sense, I might squash that with the above if it's easier.\n\n>\n> * Introduce \"--submodule=diff\", and show_submodule_diff() function;\n> the latter would use the helper function prepared in the previous\n> step.\n>\n> perhaps?\n>\n\nThat makes more sense. I'll rework the series some more.\n\n>> +\n>> + if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED) {\n>> + /*\n>> + * If the submodule has modified contents we want to diff\n>> + * against the work tree, so don't add a second parameter.\n>> + * This is essentially equivalent of using diff-index instead.\n>> + * Note that we can't (easily) show the diff of any untracked\n>> + * files.\n>> + */\n>> + } else if (is_null_sha1(two)) {\n>\n> It is safer to have ';' inside the empty if(){} block to make sure\n> that one empty statement exists there. It makes the intention of\n> the code clearer, too.\n>\n\nWill do.\n\n> I am debating myself if this is a good thing to do, though. The\n> submodule is a separate project for a reason, and there is a reason\n> why the changes haven't been committed. When asking \"what's different\n> between these two superproject states?\", should the user really see\n> these uncommitted changes?\n>\n> Thanks.\n\nWell, the previous submodule code for \"log\" prints out \"submodule has\nuntracked content\" and \"submodule has modified content\" so I figured\nthe diff might want to show that as a diff too? Or maybe we just stick\nwith the messages and only show the difference of what's actually been\ncommitted.. I think that's probably ok too.\n\nRegards,\nJake\n"},{"id":"299506","messageId":"CA+P7+xrsbNpCjm_bQ6kBK=FGcWOr0WkuVnWyV6p_0FFEMKTxhw@mail.gmail.com","threadId":"43859","inReplyTo":"xmqqlgzwd14w.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 2/3] graph: add support for --line-prefix on all graph-aware output","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-16T20:19:32Z","receivedAt":"2016-08-16T20:37:33Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Aug 16, 2016 at 11:22 AM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>\n>> To make this work, we have to fix a few bugs in the graph API that force\n>> graph_show_commit_msg to be used only when you have a valid graph.\n>> Additionally, we extend the default_diff_output_prefix handler to work\n>> even when no graph is enabled.\n>>\n>> This is somewhat of a hack on top of the graph API, but I think it\n>> should be acceptable here.\n>\n> Unlike the opt-prefix-length I removed in 1/3, the length of the\n> line-prefix will never change during the lifetime of a single\n> diff_options struct, so it might turn out that repeated strlen()\n> on it for each and every line output is wasteful.\n>\n\nI could change this to store the length at option initialization,\nprobably a good idea. Would it be better to just store it as a strbuf,\nsince these know their length already? Or just add a\nline_prefix_length field? I agree that calling strlen a lot is\nprobably wasteful.\n\n> Other than that, I didn't spot anything immediately questionable.\n>\n\nThanks. I definitely feel like this is somewhat abusing the graph API\nbecause we're adding something not exactly graph-related. But in some\nways it really is graph related as well, so I think it makes some\nsense.\n\nRegards,\nJake\n\n> Thanks.\n"},{"id":"299513","messageId":"xmqqvaz0bemk.fsf@gitster.mtv.corp.google.com","threadId":"43859","inReplyTo":"CA+P7+xqc_WwzjUnF5P4arBhBqgRbtXyKC9QWtRJ3+fmx0Q2+oA@mail.gmail.com","subject":"Re: [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-16T21:14:27Z","receivedAt":"2016-08-16T21:15:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n>>> +\n>>> + if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED) {\n>>> + /*\n>>> + * If the submodule has modified contents we want to diff\n>>> + * against the work tree, so don't add a second parameter.\n>>> + * This is essentially equivalent of using diff-index instead.\n>>> + * Note that we can't (easily) show the diff of any untracked\n>>> + * files.\n>>> + */\n>> ...\n>> I am debating myself if this is a good thing to do, though. The\n>> submodule is a separate project for a reason, and there is a reason\n>> why the changes haven't been committed. When asking \"what's different\n>> between these two superproject states?\", should the user really see\n>> these uncommitted changes?\n>\n> Well, the previous submodule code for \"log\" prints out \"submodule has\n> untracked content\" and \"submodule has modified content\" so I figured\n> the diff might want to show that as a diff too? Or maybe we just stick\n> with the messages and only show the difference of what's actually been\n> committed.. I think that's probably ok too.\n\nI do not have a strong opinion. We only see DIRTY when we are\nlooking at the working tree at the top-level diff (i.e. \"git diff\nHEAD~ HEAD\" at the top-level, or \"git diff --cached\" will not show\nDIRTY_SUBMODULE_MODIFIED when a submodule has local modifications in\nits working tree), so in that sense, it probably makes sense to show\nthe more detailed picture of the working tree like your patch does.\nAfter all, choosing to use --submodule=diff is a strong sign that\nthe user who says top-level \"git diff []\" wants to see the\ndetails of her work-in-progress.\n\nDo you need to handle \"git diff -R []\" at the top-level a bit\ndifferently, by the way? If this function gets the full diff_options\nthat is driving the top-level diff, the DIFF_OPT_REVERSE_DIFF bit\nwould tell you that.\n\n"},{"id":"299516","messageId":"CA+P7+xrnUAN8M4UX0hZN213yUKndb_4d+AjohWV45KvCdJm9PQ@mail.gmail.com","threadId":"43859","inReplyTo":"xmqqvaz0bemk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-16T21:20:30Z","receivedAt":"2016-08-16T21:21:22Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Aug 16, 2016 at 2:14 PM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>\n>>>> +\n>>>> + if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED) {\n>>>> + /*\n>>>> + * If the submodule has modified contents we want to diff\n>>>> + * against the work tree, so don't add a second parameter.\n>>>> + * This is essentially equivalent of using diff-index instead.\n>>>> + * Note that we can't (easily) show the diff of any untracked\n>>>> + * files.\n>>>> + */\n>>> ...\n>>> I am debating myself if this is a good thing to do, though. The\n>>> submodule is a separate project for a reason, and there is a reason\n>>> why the changes haven't been committed. When asking \"what's different\n>>> between these two superproject states?\", should the user really see\n>>> these uncommitted changes?\n>>\n>> Well, the previous submodule code for \"log\" prints out \"submodule has\n>> untracked content\" and \"submodule has modified content\" so I figured\n>> the diff might want to show that as a diff too? Or maybe we just stick\n>> with the messages and only show the difference of what's actually been\n>> committed.. I think that's probably ok too.\n>\n> I do not have a strong opinion. We only see DIRTY when we are\n> looking at the working tree at the top-level diff (i.e. \"git diff\n> HEAD~ HEAD\" at the top-level, or \"git diff --cached\" will not show\n> DIRTY_SUBMODULE_MODIFIED when a submodule has local modifications in\n> its working tree), so in that sense, it probably makes sense to show\n> the more detailed picture of the working tree like your patch does.\n> After all, choosing to use --submodule=diff is a strong sign that\n> the user who says top-level \"git diff []\" wants to see the\n> details of her work-in-progress.\n>\n> Do you need to handle \"git diff -R []\" at the top-level a bit\n> differently, by the way? If this function gets the full diff_options\n> that is driving the top-level diff, the DIFF_OPT_REVERSE_DIFF bit\n> would tell you that.\n>\n\nProbably. Unfortunately what I really need is to be able to convert\n(almost) all diff options from the diff_options structure back into\ncommand line flags. This is the primary reason I would prefer to not\nuse a sub-command, but I'm not really sure what's the best way to\nactually DO that in a safe way.\n\nThe sub command brings nice separation between the superproject and\nits submodules... but it has problem because we can't use C calls\ndirectly, so I can't pass the options along myself.\n\nThoughts on that? Or should we just limit ourselves to only some\noptions get propagated to the submodule?\n\nThanks,\nJake\n"},{"id":"299519","messageId":"xmqqmvkcbdui.fsf@gitster.mtv.corp.google.com","threadId":"43859","inReplyTo":"CA+P7+xrnUAN8M4UX0hZN213yUKndb_4d+AjohWV45KvCdJm9PQ@mail.gmail.com","subject":"Re: [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-16T21:31:17Z","receivedAt":"2016-08-16T21:31:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> Thoughts on that? Or should we just limit ourselves to only some\n> options get propagated to the submodule?\n\nI think you have to be selective either way. You do not want\npathspecs used to limit the top-level paths propagated down when you\nrun a diff or a log in the submodule, for example, and that does not\nchange even if you start using the code in diff-lib.c (after adding\nthe submodule odb as an alternate).\n\n"}]}