threads / patch / 43882

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

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

## tl;dr

3 messages between Aug 19, 2016 and Aug 19, 2016. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

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

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

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

Show 13 quoted lines
> +                               *
> +                               * Usually, this newline produces a blank
> +                               * padding line between entries, in which case
> +                               * we need to add graph padding on this line.
> +                               *
> +                               * However, the commit message may not end in a
> +                               * newline.  In this case the newline simply
> +                               * ends the last line of the commit message,
> +                               * and we don't need any graph output.  (This
> +                               * always happens with CMIT_FMT_ONELINE, and it
> +                               * happens with CMIT_FMT_USERFORMAT when the
> +                               * format doesn't explicitly end in a newline.)
> +                               */
Show 5 quoted lines
> - if (!graph)
> + if (!graph) {
> +         graph_show_line_prefix(default_diffopt);
>           return;
> + }
Show 5 quoted lines
> -       if (graph_is_commit_finished(graph))
> +       if (graph_is_commit_finished(graph)) {
> +               graph_show_line_prefix(&graph->revs->diffopt);
>                 return 0;
> +       }

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

Thanks, Stefan

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

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

8<-----
From 345bbaa47cc14aba7049738e99c3649e2c06748c Mon Sep 17 00:00:00 2001
From: Jacob Keller <jacob.keller@gmail.com>
Date: Thu, 18 Aug 2016 11:13:04 -0700
Subject: [PATCH] SQUASH: fix indentation of comment in builtin/rev-list.c
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 builtin/rev-list.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)
Show changes to builtin/rev-list.c +14 −14
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 1a75a83538f4..21cde8dd6b31 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -129,20 +129,20 @@ static void show_commit(struct commit *commit, void *data)
 			graph_show_commit_msg(revs->graph, stdout, &buf);
 
 			/*
-				* Add a newline after the commit message.
-				*
-				* Usually, this newline produces a blank
-				* padding line between entries, in which case
-				* we need to add graph padding on this line.
-				*
-				* However, the commit message may not end in a
-				* newline.  In this case the newline simply
-				* ends the last line of the commit message,
-				* and we don't need any graph output.  (This
-				* always happens with CMIT_FMT_ONELINE, and it
-				* happens with CMIT_FMT_USERFORMAT when the
-				* format doesn't explicitly end in a newline.)
-				*/
+			* Add a newline after the commit message.
+			*
+			* Usually, this newline produces a blank
+			* padding line between entries, in which case
+			* we need to add graph padding on this line.
+			*
+			* However, the commit message may not end in a
+			* newline.  In this case the newline simply
+			* ends the last line of the commit message,
+			* and we don't need any graph output.  (This
+			* always happens with CMIT_FMT_ONELINE, and it
+			* happens with CMIT_FMT_USERFORMAT when the
+			* format doesn't explicitly end in a newline.)
+			*/
 			if (buf.len && buf.buf[buf.len - 1] == '\n')
 				graph_show_padding(revs->graph);
 			putchar('\n');
-- 
2.10.0.rc0.217.g609f9e8.dirty
Jacob Keller· Aug 18, 2016, 18:29 UTC · re: Stefan Beller · lore
On Thu, Aug 18, 2016 at 10:56 AM, Stefan Beller <sbeller@google.com> wrote:
Show 18 quoted lines
> On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>> - if (!graph)
>> + if (!graph) {
>> +         graph_show_line_prefix(default_diffopt);
>>           return;
>> + }
>
>> -       if (graph_is_commit_finished(graph))
>> +       if (graph_is_commit_finished(graph)) {
>> +               graph_show_line_prefix(&graph->revs->diffopt);
>>                 return 0;
>> +       }
>
> This seems to be a reoccurring pattern, i.e. we need to add a lot of
> one off instructions before a return. Is it possible to make the call
> earlier instead
> of just before the returns? (or later for that matter) ?
>

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

Thanks, Jake

>
> Thanks,
> Stefan

← back to recent threads