Re: [PATCH 09/20] abbrev tests: test for "git-log" behavior

On 9 June 2018 at 11:56, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

On Sat, Jun 09 2018, Martin Ågren wrote:

quoted

On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

The "log" family of commands does its own parsing for --abbrev in
revision.c, so having dedicated tests for it makes sense.

quoted

+for i in $(test_seq 4 40)

I've just been skimming so might have missed something, but I see
several instances of this construct, and I wonder what this brute-force
approach really buys us. An alternative would be, e.g., "for i in 4 23
40". That is, min/max and some arbitrary number in between (odd because
the others are even).

Of course, we might have a bug which magically happens for the number 9,
but I'd expect us to test for that only if we have some reason to
believe that number 9 is indeed magical.

Good point, I'll change this in v2, or at least guard it with
EXPENSIVE. I hacked it up like this while exhaustively testing things
during development, and discovered some edge cases (e.g. "0" is special
sometimes).

Ah, "useful during hacking" explains why you did it like this. Of your
two approaches, I'd probably favour "make it cheaper" over "mark it as
EXPENSIVE". Nothing I feel strongly about.

quoted

Also, 40 is of course tied to SHA-1. You could perhaps define a variable
at the top of this file to simplify a future generalization. (Same for
39/41 which are related to 40.)

I forgot to note this in the commit message, but I intentionally didn't
guard this test with the SHA1 prereq, there's nothing per-se specific to
SHA-1 here, it's not a given that whatever our NewHash is that we won't
use 40 characters, and the rest of the magic constants like 4 and 7 is
something we're likely to retain with NewHash.

I'd tend to agree about not marking this SHA1.

Although maybe we should expose GIT_SHA1_HEXSZ to the test suite.

It seems like brian's "test_translate"-approach [1] would be a good
choice of tool for this. That is, you'd just define something at the top
of this file for now, then once that tool is in place, a one-line change
could get "hexsz" from `test_translate` instead.

[1] https://public-inbox.org/git/20180604235229.279814-2-sandals@crustytoothpaste.net/

Martin