{"thread":{"id":"43882","subject":"Re: [PATCH v7 2/7] graph: add support for --line-prefix on all graph-aware output","startedAt":"2016-08-19T01:14:56Z","lastAt":"2016-08-19T01:23:07Z","messageCount":3,"participants":["Jacob Keller","Stefan Beller"],"isPatch":true,"patchVersion":7,"patchTotal":7},"messages":[{"id":"299645","messageId":"20160818182603.10772-1-jacob.e.keller@intel.com","threadId":"43882","inReplyTo":"CAGZ79kZeSwteU84-=GZ0cZDCJUf6=VrGzAiNqC_gx7DgTQX-8Q@mail.gmail.com","subject":"Re: [PATCH v7 2/7] graph: add support for --line-prefix on all graph-aware output","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-08-18T18:26:03Z","receivedAt":"2016-08-19T01:14:56Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nOn Thu, Aug 18, 2016 at 10:56 AM, Stefan Beller wrote:\n> On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller wrote:\n>> From: Jacob Keller \n>> - /*\n>> - * Add a newline after the commit message.\n>> - *\n>> - * Usually, this newline produces a blank\n>> - * padding line between entries, in which case\n>> - * we need to add graph padding on this line.\n>> - *\n>> - * However, the commit message may not end in a\n>> - * newline. In this case the newline simply\n>> - * ends the last line of the commit message,\n>> - * and we don't need any graph output. (This\n>> - * always happens with CMIT_FMT_ONELINE, and it\n>> - * happens with CMIT_FMT_USERFORMAT when the\n>> - * format doesn't explicitly end in a newline.)\n>> - */\n>> - if (buf.len && buf.buf[buf.len - 1] == '\\n')\n>> - graph_show_padding(revs->graph);\n>> - putchar('\\n');\n>> - } else {\n>> - /*\n>> - * If the message buffer is empty, just show\n>> - * the rest of the graph output for this\n>> - * commit.\n>> - */\n>> - if (graph_show_remainder(revs->graph))\n>> - putchar('\\n');\n>> - if (revs->commit_format == CMIT_FMT_ONELINE)\n>> - putchar('\\n');\n>> - }\n>> + /*\n>> + * Add a newline after the commit message.\n>\n> I wondered if it is my webmailer displaying things wrongly here so I checked\n> it at public inbox, and fetched the mails and applied them.\n>\n> It seems to me as if this long comment is broken in indentation\n> (i.e. you removed a blank ' ' directly in front of the '*' instead of a '\\t' ?)\n>\n\nYea it is indeed broken. Something like the following squash should fix it.\nWe could also re-flow the text if desired too, but I don't really see the\nadvantage.\n\n8<-----\n\nFrom 345bbaa47cc14aba7049738e99c3649e2c06748c Mon Sep 17 00:00:00 2001\nFrom: Jacob Keller \nDate: Thu, 18 Aug 2016 11:13:04 -0700\nSubject: [PATCH] SQUASH: fix indentation of comment in builtin/rev-list.c\n\nSigned-off-by: Jacob Keller \n---\n builtin/rev-list.c | 28 ++++++++++++++--------------\n 1 file changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 1a75a83538f4..21cde8dd6b31 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -129,20 +129,20 @@ static void show_commit(struct commit *commit, void *data)\n \t\t\tgraph_show_commit_msg(revs->graph, stdout, &buf);\n \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\t* Add a newline after the commit message.\n+\t\t\t*\n+\t\t\t* Usually, this newline produces a blank\n+\t\t\t* padding line between entries, in which case\n+\t\t\t* we need to add graph padding on this line.\n+\t\t\t*\n+\t\t\t* However, the commit message may not end in a\n+\t\t\t* newline. In this case the newline simply\n+\t\t\t* ends the last line of the commit message,\n+\t\t\t* and we don't need any graph output. (This\n+\t\t\t* always happens with CMIT_FMT_ONELINE, and it\n+\t\t\t* happens with CMIT_FMT_USERFORMAT when the\n+\t\t\t* format doesn't explicitly end in a newline.)\n+\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-- \n2.10.0.rc0.217.g609f9e8.dirty\n\n"},{"id":"299647","messageId":"CA+P7+xpEDy5xqNp=tFGauE7+N2hnaGgzZKr2neM2p6G6kvchWQ@mail.gmail.com","threadId":"43882","inReplyTo":"CAGZ79kZeSwteU84-=GZ0cZDCJUf6=VrGzAiNqC_gx7DgTQX-8Q@mail.gmail.com","subject":"Re: [PATCH v7 2/7] graph: add support for --line-prefix on all graph-aware output","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-18T18:29:23Z","receivedAt":"2016-08-19T01:15:50Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Aug 18, 2016 at 10:56 AM, Stefan Beller wrote:\n> On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller wrote:\n>> - if (!graph)\n>> + if (!graph) {\n>> + graph_show_line_prefix(default_diffopt);\n>> return;\n>> + }\n>\n>> - if (graph_is_commit_finished(graph))\n>> + if (graph_is_commit_finished(graph)) {\n>> + graph_show_line_prefix(&graph->revs->diffopt);\n>> return 0;\n>> + }\n>\n> This seems to be a reoccurring pattern, i.e. we need to add a lot of\n> one off instructions before a return. Is it possible to make the call\n> earlier instead\n> of just before the returns? (or later for that matter) ?\n>\n\nNot sure what you mean by this...? Just move the call up above this? I\nthink that will work, let me give it a try.\n\nThanks,\nJake\n\n>\n> Thanks,\n> Stefan\n"},{"id":"299650","messageId":"CAGZ79kZeSwteU84-=GZ0cZDCJUf6=VrGzAiNqC_gx7DgTQX-8Q@mail.gmail.com","threadId":"43882","inReplyTo":"20160818005131.31600-3-jacob.e.keller@intel.com","subject":"Re: [PATCH v7 2/7] graph: add support for --line-prefix on all graph-aware output","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-18T17:56:09Z","receivedAt":"2016-08-19T01:23:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller wrote:\n> From: Jacob Keller \n>\n> Add an extension to git-diff and git-log (and any other graph-aware\n> displayable output) such that \"--line-prefix=\" will print the\n> additional line-prefix on every line of output.\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> This will be used by a future extension of submodule display which\n> displays the submodule diff as the actual diff between the pre and post\n> commit in the submodule project.\n>\n> Add some tests for both git-log and git-diff to ensure that the prefix\n> is honored correctly.\n>\n> Signed-off-by: Jacob Keller \n> ---\n> Documentation/diff-options.txt | 3 +\n> builtin/rev-list.c | 70 ++---\n> diff.c | 7 +\n> diff.h | 2 +\n> graph.c | 104 ++++---\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, 506 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>\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 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> Do not show any source or destination prefix.\n>\n> +--line-prefix=::\n> + Prepend an additional prefix to every line of output.\n> +\n> For more detailed explanation on these common options, see also\n> linkgit:gitdiffcore[7].\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index 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> ctx.fmt = revs->commit_format;\n> ctx.output_encoding = get_log_output_encoding();\n> pretty_print_commit(&ctx, commit, &buf);\n> - if (revs->graph) {\n> - if (buf.len) {\n> - if (revs->commit_format != CMIT_FMT_ONELINE)\n> - graph_show_oneline(revs->graph);\n> + if (buf.len) {\n> + if (revs->commit_format != CMIT_FMT_ONELINE)\n> + graph_show_oneline(revs->graph);\n>\n> - graph_show_commit_msg(revs->graph, &buf);\n> + graph_show_commit_msg(revs->graph, stdout, &buf);\n>\n> - /*\n> - * Add a newline after the commit message.\n> - *\n> - * Usually, this newline produces a blank\n> - * padding line between entries, in which case\n> - * we need to add graph padding on this line.\n> - *\n> - * However, the commit message may not end in a\n> - * newline. In this case the newline simply\n> - * ends the last line of the commit message,\n> - * and we don't need any graph output. (This\n> - * always happens with CMIT_FMT_ONELINE, and it\n> - * happens with CMIT_FMT_USERFORMAT when the\n> - * format doesn't explicitly end in a newline.)\n> - */\n> - if (buf.len && buf.buf[buf.len - 1] == '\\n')\n> - graph_show_padding(revs->graph);\n> - putchar('\\n');\n> - } else {\n> - /*\n> - * If the message buffer is empty, just show\n> - * the rest of the graph output for this\n> - * commit.\n> - */\n> - if (graph_show_remainder(revs->graph))\n> - putchar('\\n');\n> - if (revs->commit_format == CMIT_FMT_ONELINE)\n> - putchar('\\n');\n> - }\n> + /*\n> + * Add a newline after the commit message.\n\nI wondered if it is my webmailer displaying things wrongly here so I checked\nit at public inbox, and fetched the mails and applied them.\n\nIt seems to me as if this long comment is broken in indentation\n(i.e. you removed a blank ' ' directly in front of the '*' instead of a '\\t' ?)\n\n\n> + *\n> + * Usually, this newline produces a blank\n> + * padding line between entries, in which case\n> + * we need to add graph padding on this line.\n> + *\n> + * However, the commit message may not end in a\n> + * newline. In this case the newline simply\n> + * ends the last line of the commit message,\n> + * and we don't need any graph output. (This\n> + * always happens with CMIT_FMT_ONELINE, and it\n> + * happens with CMIT_FMT_USERFORMAT when the\n> + * format doesn't explicitly end in a newline.)\n> + */\n\n> - if (!graph)\n> + if (!graph) {\n> + graph_show_line_prefix(default_diffopt);\n> return;\n> + }\n\n> - if (graph_is_commit_finished(graph))\n> + if (graph_is_commit_finished(graph)) {\n> + graph_show_line_prefix(&graph->revs->diffopt);\n> return 0;\n> + }\n\nThis seems to be a reoccurring pattern, i.e. we need to add a lot of\none off instructions before a return. Is it possible to make the call\nearlier instead\nof just before the returns? (or later for that matter) ?\n\n\nThanks,\nStefan\n"}]}