{"thread":{"id":"43047","subject":"[PATCH v3 1/2] diff: add --line-prefix option for passing in a prefix","startedAt":"2016-08-10T21:17:23Z","lastAt":"2016-08-10T22:57:30Z","messageCount":7,"participants":["Jacob Keller","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"293648","messageId":"20160810211710.23173-1-jacob.e.keller@intel.com","threadId":"43047","inReplyTo":null,"subject":"[PATCH v3 1/2] diff: add --line-prefix option for passing in a prefix","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-08-10T21:17:09Z","receivedAt":"2016-08-10T21:17:23Z","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\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\nAs suggested by Junio, I implemented --line-prefix to enable the graph\ndisplay correctly. This works by a neat trick of adding to the msgbuf,\nso no code needs to be altered. I presumed that the line prefix should\ngo *after* the graphs own prefix.\n\n Documentation/diff-options.txt |  3 +++\n diff.c                         | 12 +++++++++++-\n diff.h                         |  1 +\n 3 files changed, 15 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..6fe5c6d084a3 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1167,10 +1167,16 @@ 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+\tif (opt->line_prefix)\n+\t\tstrbuf_addstr(msgbuf, opt->line_prefix);\n \treturn msgbuf->buf;\n }\n \n@@ -3966,6 +3972,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.g0b694e0.dirty\n\n"},{"id":"293649","messageId":"20160810211710.23173-2-jacob.e.keller@intel.com","threadId":"43047","inReplyTo":"20160810211710.23173-1-jacob.e.keller@intel.com","subject":"[PATCH v3 2/2] diff: add SUBMODULE_DIFF format to display submodule diff","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-08-10T21:17:10Z","receivedAt":"2016-08-10T21:17:26Z","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\nFor projects which have frequent updates to submodules it is often\nuseful to be able to see a submodule update commit as a difference.\nTeach diff's --submodule= a new \"diff\" format which will execute a diff\nfor the submodule between the old and new commit, and display it as\na standard diff. This allows users to easily see what changed in\na submodule without having to dig into the submodule themselves.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\nChanges since v2:\n* dropped the prepare_submodule_diff as it's not really needed\n* added --line-prefix to properly prefix every line\n* added correct use of a_prefix and b_prefix\n* use dup and fileno instead so that we don't have to copy output\n\nI still need to sort out some tests, and I ran into one interesting\nissue: I had to do make-install in order to get the sub command to\nprocess the new diff option... is that normal? I think that may cause\nissues during tests as well... Shouldn't we use the same binaries we're\nrunning from and not search in the install path when running a sub\ncommand?\n\n Documentation/diff-config.txt  |  3 +-\n Documentation/diff-options.txt |  6 ++--\n diff.c                         | 39 ++++++++++++++++--------\n diff.h                         |  2 +-\n submodule.c                    | 67 ++++++++++++++++++++++++++++++++++++++++++\n submodule.h                    |  6 ++++\n 6 files changed, 107 insertions(+), 16 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 6fe5c6d084a3..2d23767739a4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -130,12 +130,18 @@ static int parse_dirstat_params(struct diff_options *options, const char *params\n \n static int parse_submodule_params(struct diff_options *options, const char *value)\n {\n-\tif (!strcmp(value, \"log\"))\n+\tif (!strcmp(value, \"log\")) {\n \t\tDIFF_OPT_SET(options, SUBMODULE_LOG);\n-\telse if (!strcmp(value, \"short\"))\n+\t\tDIFF_OPT_CLR(options, SUBMODULE_DIFF);\n+\t} else if (!strcmp(value, \"diff\")) {\n+\t\tDIFF_OPT_SET(options, SUBMODULE_DIFF);\n \t\tDIFF_OPT_CLR(options, SUBMODULE_LOG);\n-\telse\n+\t} else if (!strcmp(value, \"short\")) {\n+\t\tDIFF_OPT_CLR(options, SUBMODULE_DIFF);\n+\t\tDIFF_OPT_CLR(options, SUBMODULE_LOG);\n+\t} else {\n \t\treturn -1;\n+\t}\n \treturn 0;\n }\n \n@@ -2305,6 +2311,15 @@ 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+\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 (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@@ -2316,6 +2331,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 (DIFF_OPT_TST(o, SUBMODULE_DIFF) &&\n+\t\t\t(!one->mode || S_ISGITLINK(one->mode)) &&\n+\t\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@@ -2323,15 +2347,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;\ndiff --git a/diff.h b/diff.h\nindex 6a91a1139686..65df44b1fcba 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -69,7 +69,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)\n #define DIFF_OPT_FOLLOW_RENAMES      (1 <<  7)\n #define DIFF_OPT_RENAME_EMPTY        (1 <<  8)\n-/* (1 <<  9) unused */\n+#define DIFF_OPT_SUBMODULE_DIFF      (1 <<  9)\n #define DIFF_OPT_HAS_CHANGES         (1 << 10)\n #define DIFF_OPT_QUICK               (1 << 11)\n #define DIFF_OPT_NO_INDEX            (1 << 12)\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.g0b694e0.dirty\n\n"},{"id":"293658","messageId":"xmqq4m6s476t.fsf@gitster.mtv.corp.google.com","threadId":"43047","inReplyTo":"20160810211710.23173-1-jacob.e.keller@intel.com","subject":"Re: [PATCH v3 1/2] diff: add --line-prefix option for passing in a prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-10T21:58:50Z","receivedAt":"2016-08-10T21:59:15Z","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> As suggested by Junio, I implemented --line-prefix to enable the graph\n> display correctly. This works by a neat trick of adding to the msgbuf,\n> so no code needs to be altered. I presumed that the line prefix should\n> go *after* the graphs own prefix.\n\nI do not understand the last sentence.\n\nThe motivation I suggested the --line-prefix for is for a scenario\nin which \"git log -p --graph\" that recurses into a submodule causes\n\"git diff A B\" between the two commits in a submodule to run; the\ninternal diff machinery driven by \"log -p --graph\" for the\nsuperproject knows what the graph lines that depict the lineages of\nhistory in the superproject should look like, but the \"git diff A B\"\nin the submodule of course does not, so while showing e.g. \"| | |\"\n(three lineages) for the history in the superproject, you would run\n\"git diff --line-prefix='| | | ' A B\" and by showing them before\nanything that \"git diff A B\" without the new option would have\nproduced, you could mimick and match the graph output in the\nsuperproject.\n\nIn that scenario, the line prefix _is_ the graph's prefix in the\nsuperproject.\n\nYou might be envisioning a future enhancement where the recursive\none uses not \"-Submodule commit A\"/\"-Submodule commit B\", and not\n\"diff A B\", but \"log -p A...B\" in the submodule, and in such a case,\nit might make sense to run \"log -p --graph A...B\" instead when the\ncommand the end user run in the superproject asked for \"--graph\".\nYou would use the same \"--line-prefix='| | | '\" when running the\n\"log -p --graph A...B\" command in the submodule, so that the output\nfor the submodule will go _after_ the graph of the superproject, but\nin that case, the output for the submodule would also include its\nown graph to show the relationship between A and B.  The line-prefix\nmust come before that graph part (and also if it is \"git log\" run in\nthe submodule, the same line-prefix must come before each line of\nthe log message output).\n\nWith that understanding/assumption, \"line prefix should go after the\ngraph's own prefix\" sounds like a wrong choice to me.  Shouldn't the\nprefix go before everything else?\n\n"},{"id":"293663","messageId":"xmqqziok2sbg.fsf@gitster.mtv.corp.google.com","threadId":"43047","inReplyTo":"20160810211710.23173-2-jacob.e.keller@intel.com","subject":"Re: [PATCH v3 2/2] diff: add SUBMODULE_DIFF format to display submodule diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-10T22:05:23Z","receivedAt":"2016-08-10T22:10:06Z","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> @@ -2305,6 +2311,15 @@ 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> +\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\nHmph, is it safe to raise this when SUBMODULE_DIFF is not in effect?\nNot objecting, just asking.\n\n>  \tif (DIFF_OPT_TST(o, SUBMODULE_LOG) &&\n> ...\n> +\t} else if (DIFF_OPT_TST(o, SUBMODULE_DIFF) &&\n\nThis makes clear that SUBMODULE_LOG and SUBMODULE_DIFF should not be\nindependent bits in the diff-opt flag word.  We used to run\nsomething like \"log --oneline --left-right A...B\" and your new code\nruns \"diff A B\", but the next month somebody else would want to do\n\"log -p --left-right A...B\" or something else, and they are clearly\nmutually exclusive.\n\n> diff --git a/diff.h b/diff.h\n> index 6a91a1139686..65df44b1fcba 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -69,7 +69,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n>  #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)\n>  #define DIFF_OPT_FOLLOW_RENAMES      (1 <<  7)\n>  #define DIFF_OPT_RENAME_EMPTY        (1 <<  8)\n> -/* (1 <<  9) unused */\n> +#define DIFF_OPT_SUBMODULE_DIFF      (1 <<  9)\n\nSo I'd really prefer not to see this change; instead, we should move\nin the direction where we _REMOVE_ DIFF_OPT_SUBMODULE_LOG from these,\nand introduce an enum to hold \"how would we show submodule changes\"\nin the diff_options structure.\n"},{"id":"293674","messageId":"CA+P7+xrcCDP4NZFb+hvV3yFE7VAbNcScL6S8ezBsjhRdcG4Xdw@mail.gmail.com","threadId":"43047","inReplyTo":"xmqqziok2sbg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/2] diff: add SUBMODULE_DIFF format to display submodule diff","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-10T22:45:37Z","receivedAt":"2016-08-10T22:46:02Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Aug 10, 2016 at 3:05 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jacob Keller <jacob.e.keller@intel.com> writes:\n>\n>> @@ -2305,6 +2311,15 @@ static void builtin_diff(const char *name_a,\n>>       struct strbuf header = STRBUF_INIT;\n>>       const char *line_prefix = diff_line_prefix(o);\n>>\n>> +     diff_set_mnemonic_prefix(o, \"a/\", \"b/\");\n>> +     if (DIFF_OPT_TST(o, REVERSE_DIFF)) {\n>> +             a_prefix = o->b_prefix;\n>> +             b_prefix = o->a_prefix;\n>> +     } else {\n>> +             a_prefix = o->a_prefix;\n>> +             b_prefix = o->b_prefix;\n>> +     }\n>> +\n>\n> Hmph, is it safe to raise this when SUBMODULE_DIFF is not in effect?\n> Not objecting, just asking.\n\nThe only other code was SUBMODULE_LOG which doesn't get passed the\na_prefix and b_prefix so it shouldn't make a difference.\n\n>\n>>       if (DIFF_OPT_TST(o, SUBMODULE_LOG) &&\n>> ...\n>> +     } else if (DIFF_OPT_TST(o, SUBMODULE_DIFF) &&\n>\n> This makes clear that SUBMODULE_LOG and SUBMODULE_DIFF should not be\n> independent bits in the diff-opt flag word.  We used to run\n> something like \"log --oneline --left-right A...B\" and your new code\n> runs \"diff A B\", but the next month somebody else would want to do\n> \"log -p --left-right A...B\" or something else, and they are clearly\n> mutually exclusive.\n\nThey are independent bits, but the set and clear make them mutually\nexclusive. How would you implement this instead? Maybe as a separate\nfield of the diff_options?\n\n>\n>> diff --git a/diff.h b/diff.h\n>> index 6a91a1139686..65df44b1fcba 100644\n>> --- a/diff.h\n>> +++ b/diff.h\n>> @@ -69,7 +69,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n>>  #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)\n>>  #define DIFF_OPT_FOLLOW_RENAMES      (1 <<  7)\n>>  #define DIFF_OPT_RENAME_EMPTY        (1 <<  8)\n>> -/* (1 <<  9) unused */\n>> +#define DIFF_OPT_SUBMODULE_DIFF      (1 <<  9)\n>\n> So I'd really prefer not to see this change; instead, we should move\n> in the direction where we _REMOVE_ DIFF_OPT_SUBMODULE_LOG from these,\n> and introduce an enum to hold \"how would we show submodule changes\"\n> in the diff_options structure.\n\nYes, I agree. I will rework that.\n\nThanks,\nJake\n"},{"id":"293678","messageId":"CA+P7+xpmWu6eeoPkenn=-2s=pW8GdCtBpRGcqDWkmASPmMsetA@mail.gmail.com","threadId":"43047","inReplyTo":"xmqq4m6s476t.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 1/2] diff: add --line-prefix option for passing in a prefix","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-10T22:53:54Z","receivedAt":"2016-08-10T22:54:20Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Aug 10, 2016 at 2:58 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> You might be envisioning a future enhancement where the recursive\n> one uses not \"-Submodule commit A\"/\"-Submodule commit B\", and not\n> \"diff A B\", but \"log -p A...B\" in the submodule, and in such a case,\n> it might make sense to run \"log -p --graph A...B\" instead when the\n> command the end user run in the superproject asked for \"--graph\".\n> You would use the same \"--line-prefix='| | | '\" when running the\n> \"log -p --graph A...B\" command in the submodule, so that the output\n> for the submodule will go _after_ the graph of the superproject, but\n> in that case, the output for the submodule would also include its\n> own graph to show the relationship between A and B.  The line-prefix\n> must come before that graph part (and also if it is \"git log\" run in\n> the submodule, the same line-prefix must come before each line of\n> the log message output).\n>\n\nOops, you are correct. In this case, the line-prefix should come\nfirst, and I will reverse this.\n\nThanks,\nJake\n"},{"id":"293679","messageId":"CA+P7+xr1RWsOREue_D75z_Qd8pC1aNcPAejTXAjo=S6nP3_fNQ@mail.gmail.com","threadId":"43047","inReplyTo":"xmqq4m6s476t.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 1/2] diff: add --line-prefix option for passing in a prefix","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-10T22:42:57Z","receivedAt":"2016-08-10T22:57:30Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Aug 10, 2016 at 2:58 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jacob Keller <jacob.e.keller@intel.com> writes:\n>\n>> As suggested by Junio, I implemented --line-prefix to enable the graph\n>> display correctly. This works by a neat trick of adding to the msgbuf,\n>> so no code needs to be altered. I presumed that the line prefix should\n>> go *after* the graphs own prefix.\n>\n> I do not understand the last sentence.\n>\n> The motivation I suggested the --line-prefix for is for a scenario\n> in which \"git log -p --graph\" that recurses into a submodule causes\n> \"git diff A B\" between the two commits in a submodule to run; the\n> internal diff machinery driven by \"log -p --graph\" for the\n> superproject knows what the graph lines that depict the lineages of\n> history in the superproject should look like, but the \"git diff A B\"\n> in the submodule of course does not, so while showing e.g. \"| | |\"\n> (three lineages) for the history in the superproject, you would run\n> \"git diff --line-prefix='| | | ' A B\" and by showing them before\n> anything that \"git diff A B\" without the new option would have\n> produced, you could mimick and match the graph output in the\n> superproject.\n>\n> In that scenario, the line prefix _is_ the graph's prefix in the\n> superproject.\n>\n> You might be envisioning a future enhancement where the recursive\n> one uses not \"-Submodule commit A\"/\"-Submodule commit B\", and not\n> \"diff A B\", but \"log -p A...B\" in the submodule, and in such a case,\n> it might make sense to run \"log -p --graph A...B\" instead when the\n> command the end user run in the superproject asked for \"--graph\".\n> You would use the same \"--line-prefix='| | | '\" when running the\n> \"log -p --graph A...B\" command in the submodule, so that the output\n> for the submodule will go _after_ the graph of the superproject, but\n> in that case, the output for the submodule would also include its\n> own graph to show the relationship between A and B.  The line-prefix\n> must come before that graph part (and also if it is \"git log\" run in\n> the submodule, the same line-prefix must come before each line of\n> the log message output).\n>\n> With that understanding/assumption, \"line prefix should go after the\n> graph's own prefix\" sounds like a wrong choice to me.  Shouldn't the\n> prefix go before everything else?\n>\n\nI don't think it matters, but if it did, I assumed that a users use of\n\"line-prefix\" goes \"closest to the edge with the diff content\" and\nthen any additional prefix from log --graph would go to the left of\nthat.\n\nThanks,\nJake\n"}]}