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

From: Jacob Keller <jacob.keller@gmail.com>
Date: Aug 16, 2016, 20:19 UTC
Message-ID: <CA+P7+xrsbNpCjm_bQ6kBK=FGcWOr0WkuVnWyV6p_0FFEMKTxhw@mail.gmail.com>
In-Reply-To: <xmqqlgzwd14w.fsf@gitster.mtv.corp.google.com>

On Tue, Aug 16, 2016 at 11:22 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> 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.
>
> Unlike the opt-prefix-length I removed in 1/3, the length of the
> line-prefix will never change during the lifetime of a single
> diff_options struct, so it might turn out that repeated strlen()
> on it for each and every line output is wasteful.
>

I could change this to store the length at option initialization, probably a good idea. Would it be better to just store it as a strbuf, since these know their length already? Or just add a line_prefix_length field? I agree that calling strlen a lot is probably wasteful.

> Other than that, I didn't spot anything immediately questionable.
>

Thanks. I definitely feel like this is somewhat abusing the graph API because we're adding something not exactly graph-related. But in some ways it really is graph related as well, so I think it makes some sense.

Regards, Jake

> Thanks.