Re: [PATCH 2/5] name-rev: extend --refs to accept multiple patterns

From: Jacob Keller <jacob.keller@gmail.com>
Date: Jan 13, 2017, 00:56 UTC
Message-ID: <CA+P7+xpHQ7AobJNXTnHs1jf8Y67PJYqoR3yONFBb4k_=63CH5g@mail.gmail.com>
In-Reply-To: <alpine.DEB.2.20.1701121049530.3469@virtualbox>

On Thu, Jan 12, 2017 at 1:56 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 18 quoted lines

> Hi Jake,
>
> On Wed, 11 Jan 2017, Jacob Keller wrote:
>
>> diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
>> index 1408b608eb03..d072ec43b016 100755
>> --- a/t/t6007-rev-list-cherry-pick-file.sh
>> +++ b/t/t6007-rev-list-cherry-pick-file.sh
>> @@ -99,6 +99,36 @@ test_expect_success '--cherry-pick bar does not come up empty (II)' '
>>       test_cmp actual.named expect
>>  '
>>
>> +test_expect_success 'name-rev multiple --refs combine inclusive' '
>> +     git rev-list --left-right --cherry-pick F...E -- bar > actual &&
>
> Our current coding style seems to skip the space between `>` and `actual`
> (this applies to all redirections added in this patch).
>

Right that's easy to fix.

Show 17 quoted lines

>> +     git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" \
>> +             < actual > actual.named &&
>> +     test_cmp actual.named expect
>> +'
>> +
>> +cat >expect <<EOF
>> +<tags/F
>> +$(git rev-list --left-right --right-only --cherry-pick F...E -- bar)
>> +EOF
>
> In the current revision of t6007, we seem to list the expected output
> explicitly, i.e. *not* generating it dynamically.
>
> If you *do* insist to generate the `expect` file dynamically, a better way
> would be to include that generation in the `test_expect_success` code so
> that errors in the call can be caught, too:
>

The main reason I don't like static expecations is that often other tests inserted before or after my test now have to know what to put here. Specifically, the problem is that we expect the output not to be named, but actually to be sha1 hex output. This seems really brittle for a test.

Show 13 quoted lines

> test_expect_success 'name-rev --refs excludes non-matched patterns' '
>         echo "<tags/F" >expect &&
>         git rev-list --left-right --right-only --cherry-pick F...E -- \
>                 bar >>expect &&
>         [...]
>
> However, if I was asked for my preference, I would suggest to specify the
> `expect` contents explicitly, to document the expectation as of time of
> writing. The reason: I debugged my share of test breakages and these
> dynamically-generated `expect` files are the worst. When things break, you
> have to dig *real* deep to figure out what is going wrong, as sometimes
> the *generation of the `expect` file* regresses.
>

Do you have a better suggestion for how to check the expect vs actual output ignoring the raw hex data that would be there otherwise? What I want to avoid is a brittle test that depends precisely on actions of prior tests in generating commits and tags. Specifically in this case, we're going to end up with the sha1 hex ID of the commit in the expected value.. hard-coding that feels wrong.

Thanks, Jake

> Ciao,
> Dscho