Re: [PATCH 1/1] Fix --stat width calculations to handle --graph
From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:53:20
Hi Lucian, On Tue, 20 Mar 2012, Lucian Poston wrote:
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 Signed-off-by: Lucian Poston <redacted> ---
Good. Just a quick question before everything else: are the commit messages cut off/wrapped to the same number of columns? If so, where do they get the indent from? (Sorry for asking, but I figured that you're already deep in the code so you might know of the top of your head.)
quoted hunk ↗ jump to hunk
diff --git a/diff.c b/diff.c index 377ec1e..3a26561 100644 --- a/diff.c +++ b/diff.c@@ -1382,7 +1382,9 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) int total_files = data->nr; int width, name_width, graph_width, number_width = 4, count; const char *reset, *add_c, *del_c; - const char *line_prefix = ""; + const char *line_prefix = "", *line_prefix_iter; + unsigned int line_prefix_length = 0; + unsigned int reserved_character_count; int extra_shown = 0; struct strbuf *msg = NULL;@@ -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; + } }
My 1st reaction was: why is the current indent width not stored in the options?
But you're right, the indent is generated dynamically from output_prefix()
which is a method of diff_options, so there is little chance to do it
differently from your solution.
However, a little nit, since this list is so famous for "just a little
nit": I'd prefer to factor-out the indent width measuring, like so:
static int count_pipes_and_spaces(const char *string)
{
int count;
for (count = 0; *string; string++)
if (*string == '|' || *string == ' ')
count++;
return count;
}
It's not only that that new function cannot mess with the local variables
of show_stats(), it also documents a bit better what the code is supposed
to do (and all that without a single /* ... */! Isn't that fab? ;)
As for the complete patch: nicely done. I especially like that it is
minimally intrusive and that you took great care of updating the comments
-- not something everybody does!
My nits aside: this is good to go.
Ciao,
Dscho