From: Lucian Poston <lucian.poston@gmail.com>
Date: Thu, 22 Mar 2012 19:39:37 GMT
Subject: Re: [PATCH 1/1] Fix --stat width calculations to handle --graph
Message-ID: <CACz_eyeyni0EkM25neWdPXF7Nu8GnZv1am-UkRz3BOxBvvA1Xg@mail.gmail.com>
In-Reply-To: <7vehsn6vy1.fsf@alter.siamese.dyndns.org>

2012/3/20 Junio C Hamano <gitster@pobox.com>:
> Regarding the log message:
>
>  - Please start it with a problem description. Describe both what the
>   current code shows, and why you think it is wrong or suboptimal.
>   I.e. the observation of the problem in your second paragraph comes at
>   the beginning
>
>  - Our log message usually gives an order to the codebase or to the person
>   who is applying the patch in order to address the problem you described
>   in the earlier part of the log message, instead of tells a story of
>   what happened in the past.
>
> E.g.
>
>    The recent change to compute the width of diff --stat based on the
>    terminal width did not take the width needed to show the --graph
>    output into account, and makes lines in "log --graph --stat" too long.
>
>    Adjust stat width calculation to take the width of graph prefix into
>    account. ...

Thanks for letting me know. Patch v2 has updated log messages. Let me
know whether they meet the conventions.

> I think the caller should be taught to pass the exact width it carves out
> of the available width for use by the ancestry graph output, and if we are
> to do so, adding "int output_prefix_len" field (which usually is 0) to
> diff_options, and seting it in graph.c::diff_output_prefix_callback() (at
> that point, graph->width has the number you want, I think), may be the way
> to go.

Added outout_prefix_length to struct diff_options in patch v2.

>> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
>> index 328aa8f..84dd8bb 100755
>> --- a/t/t4052-stat-output.sh
>> +++ b/t/t4052-stat-output.sh
>> @@ -162,7 +162,7 @@ test_expect_success 'preparation for long filename tests' '
>>  '
>>
>>  cat >expect <<'EOF'
>> - ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++
>> + ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++
>>  EOF
>
> Isn't it a sign that the change is doing a lot more than justified that it
> has to change the test vector for cases where --graph is *NOT* involved at
> all?

It is, and I didn't make that clear in the log message. In patch v2,
the log message describes what has changed to in the calculation to
cause this.

Thanks!
Lucian