Re: [PATCH v12 3/8] graph: add support for --line-prefix on all graph-aware output

[Jacob Keller]

Hi,

On Wed, Oct 19, 2016 at 1:24 PM, Dennis Kaarsemaker
 wrote:
> On Wed, 2016-08-31 at 16:27 -0700, Jacob Keller wrote:
>> From: Jacob Keller 
>>
>> Add an extension to git-diff and git-log (and any other graph-aware
>> displayable output) such that "--line-prefix=" will print the
>> additional line-prefix on every line of output.
>
> This patch breaks git rev-list --header, also breaking gitweb.
>

Oops! Is it possible you have a test case already?

> The NUL between commits has gone missing, causing gitweb to interpret
> the output of git rev-list as one commit.
>

That is obviously not what we want!

> Sorry for not catching this earlier, I actually encountered this early
> september but thought it was caused by us running an ancient gitweb
> with a modern git. Finally managed to upgrade gitweb today, and the bug
> didn't go away. git bisect says 660e113ce is the culprit. Checking out
> 'next' and reverting this single patch makes the problem disappear.
>

Ok.

> Haven't yet tried to fix the bug, but this hunk looks suspicious:



>
> -   if (revs->commit_format != CMIT_FMT_USERFORMAT ||
> -   buf.len) {
> -   fwrite(buf.buf, 1, buf.len, stdout);
> -   putchar(info->hdr_termination);
> -   }
> +   /*
> +* 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');

Most likely this should have been "putchar(info->hdr_termination);" I
think? Not entirely sure.

If we can get a test case in we can use that to help debug the issue.

Thanks,
Jake

> +   if (revs->commit_format == CMIT_FMT_ONELINE)
> +
>
> D.

Re: [PATCH v12 3/8] graph: add support for --line-prefix on all graph-aware output

[Dennis Kaarsemaker]

On Wed, 2016-08-31 at 16:27 -0700, Jacob Keller wrote:
> From: Jacob Keller 
> 
> Add an extension to git-diff and git-log (and any other graph-aware
> displayable output) such that "--line-prefix=" will print the
> additional line-prefix on every line of output.

This patch breaks git rev-list --header, also breaking gitweb.

The NUL between commits has gone missing, causing gitweb to interpret
the output of git rev-list as one commit.

Sorry for not catching this earlier, I actually encountered this early
september but thought it was caused by us running an ancient gitweb
with a modern git. Finally managed to upgrade gitweb today, and the bug
didn't go away. git bisect says 660e113ce is the culprit. Checking out
'next' and reverting this single patch makes the problem disappear.

Haven't yet tried to fix the bug, but this hunk looks suspicious:

-   if (revs->commit_format != CMIT_FMT_USERFORMAT ||
-   buf.len) {
-   fwrite(buf.buf, 1, buf.len, stdout);
-   putchar(info->hdr_termination);
-   }
+   /*
+* 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)
+  

D.

[PATCH v12 3/8] graph: add support for --line-prefix on all graph-aware output

[Jacob Keller]

From: Jacob Keller 

Add an extension to git-diff and git-log (and any other graph-aware
displayable output) such that "--line-prefix=" 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 
Signed-off-by: Junio C Hamano 
---
 Documentation/diff-options.txt |   3 +
 builtin/rev-list.c |  70 ++---
 diff.c |   7 +
 diff.h |   2 +
 graph.c|  98 ---
 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, 502 insertions(+), 78 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=::
+   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..8479f6ed28aa 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(, commit, );
-   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, );
+   graph_show_commit_msg(revs->graph, stdout, );
 
-   /*
-* 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');
-