{"thread":{"id":"43049","subject":"[PATCH v4 1/2] diff: add --line-prefix option for passing in a prefix","startedAt":"2016-08-10T23:19:41Z","lastAt":"2016-08-11T20:55:51Z","messageCount":8,"participants":["Jacob Keller","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"293681","messageId":"20160810231908.26330-1-jacob.e.keller@intel.com","threadId":"43049","inReplyTo":null,"subject":"[PATCH v4 1/2] diff: add --line-prefix option for passing in a prefix","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-08-10T23:19:07Z","receivedAt":"2016-08-10T23:19:41Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nThis will be used by a future patch which implements a diff mode for\nsubmodule display. Without this, the diff output would incorrectly\ndisplay when using both -p and --graph during a git-log.\n\nNote that the --line-prefix will be displayed first prior to any other\noutput prefix.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n Documentation/diff-options.txt |  3 +++\n diff.c                         | 13 ++++++++++++-\n diff.h                         |  1 +\n 3 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 705a87394200..e7b729f3644f 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=<prefix>::\n+\tPrepend an additional prefix to every diff output line.\n+\n For more detailed explanation on these common options, see also\n linkgit:gitdiffcore[7].\ndiff --git a/diff.c b/diff.c\nindex b43d3dd2ecb7..6fa9668b19e5 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1167,10 +1167,17 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix)\n const char *diff_line_prefix(struct diff_options *opt)\n {\n \tstruct strbuf *msgbuf;\n+\n \tif (!opt->output_prefix)\n-\t\treturn \"\";\n+\t\tif (opt->line_prefix)\n+\t\t\treturn opt->line_prefix;\n+\t\telse\n+\t\t\treturn \"\";\n \n \tmsgbuf = opt->output_prefix(opt, opt->output_prefix_data);\n+\t/* line prefix must be printed before the output_prefix() */\n+\tif (opt->line_prefix)\n+\t\tstrbuf_insert(msgbuf, 0, opt->line_prefix, strlen(opt->line_prefix));\n \treturn msgbuf->buf;\n }\n \n@@ -3966,6 +3973,10 @@ 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\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 125447be09eb..6a91a1139686 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 \n-- \n2.9.2.872.g2dc3c84\n\n"},{"id":"293682","messageId":"20160810231908.26330-2-jacob.e.keller@intel.com","threadId":"43049","inReplyTo":"20160810231908.26330-1-jacob.e.keller@intel.com","subject":"[PATCH v4 2/2] diff: add SUBMODULE_DIFF format to display submodule diff","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-08-10T23:19:08Z","receivedAt":"2016-08-10T23:19:44Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\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\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n Documentation/diff-config.txt  |  3 +-\n Documentation/diff-options.txt |  6 ++--\n diff.c                         | 41 ++++++++++++++++----------\n diff.h                         |  9 +++++-\n submodule.c                    | 67 ++++++++++++++++++++++++++++++++++++++++++\n submodule.h                    |  6 ++++\n 6 files changed, 113 insertions(+), 19 deletions(-)\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 e7b729f3644f..988068225463 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -215,8 +215,10 @@ 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.  Can be tweaked via the `diff.submodule` configuration variable.\n \n --color[=<when>]::\n \tShow colored diff.\ndiff --git a/diff.c b/diff.c\nindex 6fa9668b19e5..8a591d23eb0f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -131,9 +131,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@@ -2306,9 +2308,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@@ -2317,6 +2328,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@@ -2324,15 +2344,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@@ -3922,7 +3933,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 6a91a1139686..3110a2afb3d6 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..36f8fd00c3ce 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -333,6 +333,73 @@ 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 child_process cp = CHILD_PROCESS_INIT;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf submodule_git_dir = STRBUF_INIT;\n+\tconst char *git_dir, *message = NULL;\n+\tint create_dirty_diff = 0;\n+\tFILE *diff;\n+\tchar c;\n+\n+\tif ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||\n+\t    (dirty_submodule & DIRTY_SUBMODULE_MODIFIED))\n+\t\tcreate_dirty_diff = 1;\n+\n+\tstrbuf_addf(&sb, \"%s%sSubmodule %s %s..\", line_prefix, meta, path,\n+\t\t\tfind_unique_abbrev(one, DEFAULT_ABBREV));\n+\tstrbuf_addf(&sb, \"%s:%s\\n\", !create_dirty_diff ?\n+\t\t    find_unique_abbrev(two, DEFAULT_ABBREV) : \"\", reset);\n+\tfwrite(sb.buf, sb.len, 1, f);\n+\n+\tstrbuf_addf(&submodule_git_dir, \"%s/.git\", path);\n+\tgit_dir = read_gitfile(submodule_git_dir.buf);\n+\tif (!git_dir)\n+\t\tgit_dir = submodule_git_dir.buf;\n+\tif (!is_directory(git_dir)) {\n+\t\tstrbuf_reset(&submodule_git_dir);\n+\t\tstrbuf_addf(&submodule_git_dir, \".git/modules/%s\", path);\n+\t\tgit_dir = submodule_git_dir.buf;\n+\t}\n+\n+\tfflush(f);\n+\n+\tcp.git_cmd = 1;\n+\tif (!create_dirty_diff)\n+\t\tcp.dir = git_dir;\n+\telse\n+\t\tcp.dir = path;\n+\tcp.out = dup(fileno(f));\n+\tcp.no_stdin = 1;\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+\targv_array_push(&cp.args, sha1_to_hex(one));\n+\n+\t/*\n+\t * If the submodule has untracked or modified contents don't add the\n+\t * current stored commit. Thus, we will diff between the previous\n+\t * value and the current tip of working tree. The result is a complete\n+\t * diff which shows all changes to the submodule, not just changes\n+\t * that have been committed to the super project.\n+\t */\n+\tif (!create_dirty_diff)\n+\t\targv_array_push(&cp.args, sha1_to_hex(two));\n+\n+\tif (run_command(&cp))\n+\t\tfprintf(f, \"(diff failed)\\n\");\n+\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],\n-- \n2.9.2.872.g2dc3c84\n\n"},{"id":"293725","messageId":"xmqqh9ar2pc7.fsf@gitster.mtv.corp.google.com","threadId":"43049","inReplyTo":"20160810231908.26330-1-jacob.e.keller@intel.com","subject":"Re: [PATCH v4 1/2] diff: add --line-prefix option for passing in a prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-11T17:22:00Z","receivedAt":"2016-08-11T17:22:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n>  const char *diff_line_prefix(struct diff_options *opt)\n>  {\n>  \tstruct strbuf *msgbuf;\n> +\n>  \tif (!opt->output_prefix)\n> -\t\treturn \"\";\n> +\t\tif (opt->line_prefix)\n> +\t\t\treturn opt->line_prefix;\n> +\t\telse\n> +\t\t\treturn \"\";\n>  \n>  \tmsgbuf = opt->output_prefix(opt, opt->output_prefix_data);\n> +\t/* line prefix must be printed before the output_prefix() */\n> +\tif (opt->line_prefix)\n> +\t\tstrbuf_insert(msgbuf, 0, opt->line_prefix, strlen(opt->line_prefix));\n>  \treturn msgbuf->buf;\n>  }\n\nThe result of applying this change leaves the semantics of the\nline_prefix field, the output_prefix() callback, and the\noutput_prefix_length field in the diff_options structure a bit\ntricky to reason about.\n\nThe code pretends as if the remainder of the system does not even\ncare about the presence of line_prefix (i.e. output_prefix_length is\nnot updated), but its only user of output_prefix_length cares, I\nthink.  It is in diff.c::show_stats() where it auto-computes the\nallowed display width for the stat portion:\n\n        if (options->stat_width == -1)\n                width = term_columns() - options->output_prefix_length;\n\nThe output_prefix_length is initialized to be 0, but when --graph is\nin effect, it is set to the width of the graph portion of the output\nin the output_prefix callback, diff_output_prefix_callback().\n\nSo the above change is clearly wrong in that it needs to add the\nnumber of display columns needed to show opt->line_prefix to the\noutput_prefix_length, but I wonder if a better \"fix\" to this is to\nget rid of output_prefix_length field from diff_options struct as a\npreparatory step.  That would make the bug in this patch disappear.\n\nPerhaps like this.  I do not know if Lucian is still interested in,\nor remembers what did for, Git in 2012, but this updates his code\nand replaces it with what I hope is an equivalent, so I added him\nto the Cc: line.\n\n-- >8 --\nSubject: diff.c: remove output_prefix_length field\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 <gitster@pobox.com>\n---\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 b43d3dd..ae069c3 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 125447b..49e4aaa 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 dd17201..a468038 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\n\n\n\n\n\n\n\n"},{"id":"293769","messageId":"xmqqd1lf2nvj.fsf@gitster.mtv.corp.google.com","threadId":"43049","inReplyTo":"20160810231908.26330-2-jacob.e.keller@intel.com","subject":"Re: [PATCH v4 2/2] diff: add SUBMODULE_DIFF format to display submodule diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-11T17:53:36Z","receivedAt":"2016-08-11T17:53:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n> From: Jacob Keller <jacob.keller@gmail.com>\n>\n> Teach git-diff and friends a new format for displaying the difference of\n> a submodule using git-diff inside the submodule project. This allows\n> users to easily see exactly what source changed in a given commit that\n> updates the submodule pointer. To do this, remove DIFF_SUBMODULE_LOG bit\n> from the diff options, and instead add a new enum type for these\n> formats.\n>\n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> ---\n>  Documentation/diff-config.txt  |  3 +-\n>  Documentation/diff-options.txt |  6 ++--\n>  diff.c                         | 41 ++++++++++++++++----------\n>  diff.h                         |  9 +++++-\n>  submodule.c                    | 67 ++++++++++++++++++++++++++++++++++++++++++\n>  submodule.h                    |  6 ++++\n>  6 files changed, 113 insertions(+), 19 deletions(-)\n\nThis looks good.\n\nYou'd want some tests to make sure that the \"--submodule\" and the\n\"--submodule=<format>\" command line options and the diff.submodule\nconfiguration variables are parsed correctly (and when combined, the\ncommand line option overrides the configured default), and of course\nthe machinery does the right thing, with and without \"--graph\" when\nused in \"git log\".\n\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index e7b729f3644f..988068225463 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -215,8 +215,10 @@ 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.  Can be tweaked via the `diff.submodule` configuration variable.\n\nThis is carried over from the existing text, but \"Can be tweaked\nvia\" sounds a bit wasteful (and strange); \"Defaults to\" (or \"The\ndefault is taken from\") is the phrase we seem to use more often.\n\nThanks.\n"},{"id":"293774","messageId":"CA+P7+xqodtUt9gPzZ5HxPurFy67niN5+-EDOTJpraNrvLJfZ=Q@mail.gmail.com","threadId":"43049","inReplyTo":"xmqqh9ar2pc7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 1/2] diff: add --line-prefix option for passing in a prefix","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-11T18:30:33Z","receivedAt":"2016-08-11T18:32:12Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Aug 11, 2016 at 10:22 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> -- >8 --\n> Subject: diff.c: remove output_prefix_length field\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> 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> output from the output_prefix() callback.  Any new output_prefix()\n> callback must also update the field accordingly, which is error\n> prone.  As there is only one user of the field, and the user has the\n> actual value of the prefix string, let's get rid of the field and\n> have the user count the display width itself.\n>\n\nSeems correct to me.\n\nThanks,\nJake\n"},{"id":"293775","messageId":"CA+P7+xpYrEa4Ocsny0Ca6zsRDJxax9FwiRtuxDGArgDiPbk9Cg@mail.gmail.com","threadId":"43049","inReplyTo":"xmqqd1lf2nvj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 2/2] diff: add SUBMODULE_DIFF format to display submodule diff","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-11T18:34:38Z","receivedAt":"2016-08-11T18:35:06Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Aug 11, 2016 at 10:53 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jacob Keller <jacob.e.keller@intel.com> writes:\n>\n>> From: Jacob Keller <jacob.keller@gmail.com>\n>>\n>> Teach git-diff and friends a new format for displaying the difference of\n>> a submodule using git-diff inside the submodule project. This allows\n>> users to easily see exactly what source changed in a given commit that\n>> updates the submodule pointer. To do this, remove DIFF_SUBMODULE_LOG bit\n>> from the diff options, and instead add a new enum type for these\n>> formats.\n>>\n>> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n>> ---\n>>  Documentation/diff-config.txt  |  3 +-\n>>  Documentation/diff-options.txt |  6 ++--\n>>  diff.c                         | 41 ++++++++++++++++----------\n>>  diff.h                         |  9 +++++-\n>>  submodule.c                    | 67 ++++++++++++++++++++++++++++++++++++++++++\n>>  submodule.h                    |  6 ++++\n>>  6 files changed, 113 insertions(+), 19 deletions(-)\n>\n> This looks good.\n>\n> You'd want some tests to make sure that the \"--submodule\" and the\n> \"--submodule=<format>\" command line options and the diff.submodule\n> configuration variables are parsed correctly (and when combined, the\n> command line option overrides the configured default), and of course\n> the machinery does the right thing, with and without \"--graph\" when\n> used in \"git log\".\n\nYes, I am adding tests now, but I ran into some interesting corner\ncases for this, that still need some work.\n\nThere's a bunch of issues with cases involving adding a submodule that\nisn't stored in .git/modules/etc, which the current tests for\n--submodule=log do.\n\nThere's also the case of empty trees, which I believe I have resolved\nnow. Hopefully I can sort these cases correctly.\n\n>\n>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n>> index e7b729f3644f..988068225463 100644\n>> --- a/Documentation/diff-options.txt\n>> +++ b/Documentation/diff-options.txt\n>> @@ -215,8 +215,10 @@ any of those replacements occurred.\n>>       the commits in the range like linkgit:git-submodule[1] `summary` does.\n>>       Omitting the `--submodule` option or specifying `--submodule=short`,\n>>       uses the 'short' format. This format just shows the names of the commits\n>> -     at the beginning and end of the range.  Can be tweaked via the\n>> -     `diff.submodule` configuration variable.\n>> +     at the beginning and end of the range. When `--submodule=diff` is\n>> +     given, the 'diff' format is used. This format shows the diff between\n>> +     the old and new submodule commmit from the perspective of the\n>> +     submodule.  Can be tweaked via the `diff.submodule` configuration variable.\n>\n> This is carried over from the existing text, but \"Can be tweaked\n> via\" sounds a bit wasteful (and strange); \"Defaults to\" (or \"The\n> default is taken from\") is the phrase we seem to use more often.\n\nProbably worth fixing here. WIll do.\n\nThanks,\nJake\n\n>\n> Thanks.\n"},{"id":"298776","messageId":"xmqqmvkjyqre.fsf@gitster.mtv.corp.google.com","threadId":"43049","inReplyTo":"20160810231908.26330-2-jacob.e.keller@intel.com","subject":"Re: [PATCH v4 2/2] diff: add SUBMODULE_DIFF format to display submodule diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-11T20:50:13Z","receivedAt":"2016-08-11T20:50:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n> diff --git a/submodule.c b/submodule.c\n> index 1b5cdfb7e784..36f8fd00c3ce 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -333,6 +333,73 @@ 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 child_process cp = CHILD_PROCESS_INIT;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tstruct strbuf submodule_git_dir = STRBUF_INIT;\n> +\tconst char *git_dir, *message = NULL;\n> +\tint create_dirty_diff = 0;\n> +\tFILE *diff;\n> +\tchar c;\n\nThe variables message, diff and c are not used, are they?\n"},{"id":"298785","messageId":"xmqqinv7yqio.fsf@gitster.mtv.corp.google.com","threadId":"43049","inReplyTo":"20160810231908.26330-1-jacob.e.keller@intel.com","subject":"Re: [PATCH v4 1/2] diff: add --line-prefix option for passing in a prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-11T20:55:27Z","receivedAt":"2016-08-11T20:55:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Subject: SQUASH??? clarify the if/{if/else} nesting\n\nOtherwise GCC helpfully warns you.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n diff.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex bfc0a6b..d128b9d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1170,11 +1170,12 @@ const char *diff_line_prefix(struct diff_options *opt)\n {\n \tstruct strbuf *msgbuf;\n \n-\tif (!opt->output_prefix)\n+\tif (!opt->output_prefix) {\n \t\tif (opt->line_prefix)\n \t\t\treturn opt->line_prefix;\n \t\telse\n \t\t\treturn \"\";\n+\t}\n \n \tmsgbuf = opt->output_prefix(opt, opt->output_prefix_data);\n \t/* line prefix must be printed before the output_prefix() */\n-- \n2.9.2-863-g71ed253\n\n"}]}