Zbigniew Jędrzejewski-Szmek <zbyszek@xxxxxxxxx> 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 &&
--
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