Re: [PATCH 09/20] abbrev tests: test for "git-log" behavior
- Date: Sat, 9 Jun 2018 15:56:23 +0200
- From: Martin Ågren <martin.agren@xxxxxxxxx>
- Subject: Re: [PATCH 09/20] abbrev tests: test for "git-log" behavior
On 9 June 2018 at 11:56, Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
> On Sat, Jun 09 2018, Martin Ågren wrote:
>> On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
>>> The "log" family of commands does its own parsing for --abbrev in
>>> revision.c, so having dedicated tests for it makes sense.
>>> +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
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.
>> 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  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.