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 <lucian.poston@xxxxxxxxx>
> ---
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.)
> 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
--
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