Re: [PATCH 1/3 v4] diff --stat: use the full terminal width

[Junio C Hamano <gitster@pobox.com>]

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

> - comments are updated and the word "histogram" is banished

Heh, I still see at least three instances of them in this patch.

> - use decimal_width(max_change) to calculate number of columns

Please see comments for 3/3 I'll send separately.

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 6797512..a65ade4 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -894,4 +894,100 @@ test_expect_success 'format patch ignores color.ui' '
>  	test_cmp expect actual
>  '
>  
> +name=aaaaaaaaaa
> +name=$name$name$name$name$name$name$name$name$name$name$name$name

How long is this name?  120 columns?  I think that should be fine for any
filesystem we care about.

> +test_expect_success 'preparation' "
> +	> ${name} &&
> +	git add ${name} &&
> +	git commit -m message &&
> +	echo a > ${name} &&
> +	git commit -m message ${name}
> +"

Please write these (exactly -- paying close attention to SP) like this:

	>"$name" &&
	git add "$name" &&
        git commit -m message &&
        echo a >"$name" &&
        git commit -m message "$name"

Points to note:

 - Even though you (and the reader) may know that "$name" does not contain
   word-breaking spaces, writing double-quotes around it reduces the
   mental burden from the readers;

 - Strictly speaking, the target of I/O redirection (e.g. >"$name") does
   not have to have quotes around it, but some versions of bash are known
   to give misguided warnings against it;

 - We do not write SP between the redirection and filename, but we do have
   one SP before the redirection; and

 - Unless you want to express concatenation with string that can appear
   later in words (e.g. "${name}1"), avoid ${name} to lessen the mental
   burden to readers, as "${" often signals there is some magic coming
   (e.g. "${name#strip}").

> +cat >expect <<'EOF'
> + ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
> +EOF
> +test_expect_success 'format patch graph width is 80 columns' '

Please make sure this test (and all the other new tests) pass with or
without your the patch that updates diff.c, as we need to ensure that
COLUMNS=80 or vanilla format-patch produces result that is identical to
the old output without regression (again, see comments to [PATCH 3/3]).

> +	git format-patch --stat --stdout -1 |
> +		grep -m 1 aaaaa > actual &&

Do not use "grep -m $count"; it is not portable.

Literal translation of the above would be:

	sed -n -e "/aaaaa/{
        	p
                q
	}"

but you can perhaps rely on the fact that there is only one path and '|'
does not ppear in the payload or log message, and use this instead:

	grep "|" >actual

Also to catch errors in format-patch that unexpectedly dies (after all,
you are touching diff machinery with your patch), avoid using pipes, and
write it perhaps like this:

	git format-patch --stat --stdout -1 >output &&
        grep "|" >actual &&