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

From: Jacob Keller <jacob.keller@gmail.com>
Date: Oct 19, 2016, 22:13 UTC
Message-ID: <CA+P7+xo65Wg+jTkGTBW87Xv8O5-FO9EWqkpinfW48jBMG_oNrQ@mail.gmail.com>
In-Reply-To: <1476908699.26043.9.camel@kaarsemaker.net>

Hi,

On Wed, Oct 19, 2016 at 1:24 PM, Dennis Kaarsemaker <dennis@kaarsemaker.net> wrote:

Show 9 quoted lines

> On Wed, 2016-08-31 at 16:27 -0700, Jacob Keller wrote:
>> 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.
>
> 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!

Show 6 quoted lines

> 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:

Show 13 quoted lines

>
> -                       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.