Zbigniew Jędrzejewski-Szmek <zbyszek@xxxxxxxxx> writes:
> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@xxxxxxxxx>
> ---
> All tests pass for me after this change.
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.
--
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