Re: [PATCH 04/20] abbrev tests: add tests for core.abbrev and --abbrev

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/t/t0014-abbrev.sh b/t/t0014-abbrev.sh
> new file mode 100755
> index 0000000000..1c60f5ff93
> --- /dev/null
> +++ b/t/t0014-abbrev.sh
> @@ -0,0 +1,118 @@
> +#!/bin/sh
> +
> +test_description='test core.abbrev and related features'
> +
> +. ./test-lib.sh
> +
> +tr_d_n() {
> +	tr -d '\n'
> +}

- I personally would prefer to see the reason for having a helper
  function like this to be "make it easier to reason about", rather
  "make it shorter to type".  tr_d_n feels more about the latter; if
  we aimed for the former, this would be called strip_LF or
  something.

- In the existing stcipts, it seems that we prefer to spell tr args
  with 3-octal, e.g.  \012 for LF, \015 for CR.

- Also let's have SP on both sides of (), i.e.

	funcname () {

  for shell function definitions.

> +cut_tr_d_n_field_n() {
> +	cut -d " " -f $1 | tr_d_n
> +}

Likewise.  Name this not after how it does what it does, but after
what it does and why it does so.  In other words, if your answer to
the question: "what does the caller want?" is "it wants to pick the
nth field", then name it "pick_nth_field" or something?

> +test_expect_success 'abbrev empty value handling differs ' '
> +	test_must_fail git -c core.abbrev= log -1 --pretty=format:%h 2>stderr &&
> +	test_i18ngrep "bad numeric config value.*invalid unit" stderr &&
> +
> +	git branch -v --abbrev= | cut_tr_d_n_field_n 3 >branch &&
> +	test_byte_count = 40 branch &&

Sounds like a good thing to unify.  If anything, --options=value
should be stricter than vari.able=value but it is the other way
around.

> +	git log --abbrev= -1 --pretty=format:%h >log &&
> +	test_byte_count = 4 log &&

Makes readers wonder if 4 is about 3 hex plus terminating LF.  The
reason why this works is because --pretty=format:%h (not --format=%h
or --pretty=tformat:%h) uses delimiter semantics and we won't need
any LF to show a single record.

If we use the helper to measure the length of hexadecimal digits, it
may make sense to add a wrapper around test_byte_count that strips
LF; that way, the caller can use --format=%h instead and there will
be one less thing for the reader to worry about.

> +	git diff --raw --abbrev= HEAD~ >diff &&
> +	cut_tr_d_n_field_n 3 <diff >diff.3 &&
> +	test_byte_count = 4 diff.3 &&
> +	cut_tr_d_n_field_n 4 <diff >diff.4 &&
> +	test_byte_count = 4 diff.4 &&

These all depend on the fact that we do not have excessive number of
irrelevant objects to force us to abbreviate using more hexdigits
than the minimum 4, right?  I _think_ that is a reasonable
assumption we can depend on, even across the hash function
transition.  We may want to leave in-code comment, though.

> +	test_must_fail git diff --raw --abbrev= --no-index X Y >diff &&
> +	cut_tr_d_n_field_n 3 <diff >diff.3 &&
> +	test_byte_count = 4 diff.3 &&