Re: [PATCH 3/3 v4] Update diff --stat output in tests and tutorial

Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:

Err, this is not what I meant when I said "split that part to a different patch".

If you didn't have this bit in [PATCH 1/3]

        -               fprintf(options->file, "%5"PRIuMAX"%s", added + deleted,
        -                               added + deleted ? " " : "");
        +               fprintf(options->file, " %*"PRIuMAX"%s",
        +                       number_width, added + deleted,

then I think most of the changes in [PATCH 3/3] is unnecessary when using the default 80-column output, which is how the test is run.

In other words, ideally, the [PATCH 1/3] should improve the diffstat generation code in such a way that it produces output that is identical to the existing test vectors when COLUMNS is set to 80 but takes advantage of wider terminal when available. So either the above change should not be part of [PATCH 1/3] at all, or you still chnage this fprintf(), but use number_width that is hardcoded to 4 instead of using the value computed with decimal_width(max_change). As to the test part, when a wider COLUMNS is given, the code would obviously take advantage of it, so it may:

 - contain a change to the test suite somewhere, probably t/test-lib.sh,
   to set COLUMNS=80 and export it, to make sure that the existing test
   won't be broken when the number of columns learned from ioctl(1) is
   different from 80; and

 - add a new test that explicitly sets wider COLUMNS and makes sure you
   get a wider diffstat graph.

and almost no other changes to the expected output.

If you do not arrange [PATCH 1/3] that way, we cannot verify that it does not introduce any regression to existing users. Just applying [1/3] would start failing tests, and we cannot tell which failure is a false alarm due to this change, and which ones is a real regression that you are producing wrong output when COLUMNS is set to 80.

And then as a separate patch [PATCH 3/3] at the very end of the series, you would either introduce the above change to fprintf(), or if you changed fprintf() in [PATCH 1/3] but used hardcoded number_width, then change it to use decimal_width().

That change in turn makes the bulk of this patch to update the expected output necessary.

If your series is organized that way, we can weigh pros-and-cons of the change to use decimal_width() a lot more easily.

Thanks.