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

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