Re: [PATCH] name-rev: use generation numbers if available

["Keller, Jacob E" <jacob.e.keller@intel.com>]

On 3/1/2022 7:09 AM, Derrick Stolee wrote:
> On 3/1/2022 2:33 AM, Junio C Hamano wrote:
>> Jacob Keller <jacob.keller@gmail.com> writes:
>>
>>> On Mon, Feb 28, 2022 at 6:36 PM Junio C Hamano <gitster@pobox.com> wrote:
>>>>
>>>> Jacob Keller <jacob.e.keller@intel.com> writes:
>>>>
>>>>> +test_expect_success 'name-rev without commitGraph does not handle non-monotonic timestamps' '
>>>>> +     test_config -C non-monotonic core.commitGraph false &&
>>>>> +     (
>>>>> +             cd non-monotonic &&
>>>>> +
>>>>> +             rm -rf .git/info/commit-graph* &&
>>>>> +
>>>>> +             echo "main~3 undefined" >expect &&
>>>>> +             git name-rev --tags main~3 >actual &&
>>>>> +
>>>>> +             test_cmp expect actual
>>>>> +     )
>>>>> +'
>>>>
>>>> I doubt it is wise to "test" that a program does _not_ produce a
>>>> correct output, or even worse, it produces a particular wrong
>>>> output.  This test, for example, casts in stone that any future
>>>> optimization that does not depend on the commit-graph is forever
>>>> prohibited.
>>>>
>>>> Just dropping the test would be fine, I would think.
>>>
>>> Stolee mentioned it. We could also convert it to a
>>> "test_expect_failure" with the expected output too... But that makes
>>> it look like something we'll fix
>>
>> Neither sounds like a good idea anyway.  What we care most is with
>> commit graph, the algorithm will not be fooled by skewed timestamps.
> 
> I'm fine with losing this test.
> 
> I perhaps lean too hard on "tests should document current behavior"
> so we know when we are changing behavior, and the commit can justify
> that change. For this one, we are really documenting that we have
> an optimization that doesn't walk all commits based on the date of
> the target commit. If we dropped that optimization accidentally,
> then we have no test so far that verifies that we don't walk the
> entire commit history with these name-rev queries.
> 

I think the "tests should document current behavior" is handled by the
fact that this specific test fails if you revert the name-rev changes
but keep the test.

> If there is value in documenting that optimization, then a
> comment before the test could describe that the output is not
> desirable, but it's due to an optimization that we want to keep in
> place.
> 
> Thanks,
> -Stolee

What about a test which uses something like the trace system to list all
the commits it checked? I guess that might get a bit messy but that
could be used to cover the "this optimization is important" and that
applies to the commit graph implementation rather than keeping a
negative test of the other implementation.

Thanks,
Jake