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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 15, 2012, 01:18 UTC
Message-ID: <7vwr7o3nee.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1329263146-19215-1-git-send-email-zbyszek@in.waw.pl>

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.

Show 10 quoted lines

> 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.

Show 7 quoted lines

> +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 &&