Lucian Poston <lucian.poston@xxxxxxxxx> writes:
> Adjusted stat width calculations to take into consideration the diff output
> prefix e.g. the graph prefix generated by `git log --graph --stat`.
>
> This change fixes the line wrapping that occurs when diff stats are large
> enough to be scaled to fit within the terminal's columns. This issue only
> appears when using --stat and --graph together on large diffs.
>
> Adjusted stat output tests accordingly. The scaled output tests are closer to
> the target 5:3 ratio.
>
> Added test that verifies the output of --stat --graph is truncated to fit
> within the available terminal $COLUMNS
Thanks.
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. ...
> @@ -1392,6 +1394,18 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> if (options->output_prefix) {
> msg = options->output_prefix(options, options->output_prefix_data);
> line_prefix = msg->buf;
> +
> + /*
> + * line_prefix can contain color codes, so only pipes '|' and
> + * spaces ' ' are counted.
> + */
> + line_prefix_iter = line_prefix;
> + while (*line_prefix_iter != '\0') {
> + if (*line_prefix_iter == ' ' || *line_prefix_iter == '|') {
> + line_prefix_length++;
> + }
> + line_prefix_iter += 1;
> + }
Yikes.
This code relies on "Count only ' ' and '|', because these are the only
ones we currently happen to use", which is fragile. The next person who
will update graph.c can change the set of letters used in the graph to
improve the output without even knowing your code exists or the assumption
your code makes, so she is likely not going to update it.
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.
> 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?
> @@ -179,7 +179,7 @@ log -1 --stat
> EOF
>
> cat >expect80 <<'EOF'
> - ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++
> + ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++++++++
Likewise.
> @@ -198,6 +198,26 @@ respects expect200 show --stat
> respects expect200 log -1 --stat
> EOF
>
> +cat >expect80graphed <<'EOF'
> +| ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++
> +EOF
> +cat >expect80 <<'EOF'
> + ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++++++++
> +EOF
> +while read verb expect cmd args
> +do
> + test_expect_success "$cmd $verb 80 COLUMNS (long filename)" '
> + COLUMNS=80 git $cmd $args >output
> + grep " | " output >actual &&
> + test_cmp "$expect" actual
> + '
> +done <<\EOF
> +respects expect80 show --stat
> +respects expect80 log -1 --stat
> +respects expect80graphed show --stat --graph
> +respects expect80graphed log -1 --stat --graph
> +EOF
> +
> cat >expect <<'EOF'
> abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> EOF
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html