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

From: Derrick Stolee <derrickstolee@github.com>
Date: Mar 1, 2022, 15:09 UTC
Message-ID: <6b00c67b-01c9-bf22-a8e6-904f47fa7acf@github.com>
In-Reply-To: <xmqqfso2t9cu.fsf@gitster.g>

On 3/1/2022 2:33 AM, Junio C Hamano wrote:

Show 34 quoted lines

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

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