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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 12, 2018, 18:31 UTC
Message-ID: <xmqq4li7ambf.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180608224136.20220-5-avarab@gmail.com>

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

Show 15 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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