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

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

> -----Original Message-----
> From: Junio C Hamano <gitster@pobox.com>
> Sent: Tuesday, March 01, 2022 12:23 PM
> To: Derrick Stolee <derrickstolee@github.com>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Jacob Keller
> <jacob.keller@gmail.com>; Git mailing list <git@vger.kernel.org>
> Subject: Re: [PATCH] name-rev: use generation numbers if available
> 
> Derrick Stolee <derrickstolee@github.com> writes:
> 
> >> 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.
> >
> > Ah, so this _is_ documenting a new behavior that didn't exist
> > before the series. That is good to include, then. If it was
> > "just" testing the behavior before this series, then it would
> > have less reason to exist.
> 
> With of without the additional codepath to handle the case where
> commit graph is available, the original heuristics that is based on
> commit timestamps are fooled by a history with skewed timestamps.
> 


Let's clarify. There are two versions of the test in this version:

1) test which enables commit graph and tests that it does the right behavior.

2) test which removes commit graph and tests that it behaves the old way.


test (1) checks the flow with the commit graph enabled, and verifies that with a commit graph the new behavior is used. This test will fail if you revert the name-rev commit-graph support.

test (2) always performs the way we don't like. (since we disable the commit graph and the new flow doesn't kick in) This is the test I think I will eliminate in the next revision.


I will remove the 2nd test, since the first test covers the change in behavior just fine, and I think I agree we don't need to set in-stone the implementation without commit graph.

I will also look at adding a test which performs a count of which revisions get inspected and makes sure that we actually are doing the optimization.

> So I thought this "without commit graph, the algorithm must fail
> this way" test would be testing the current behaviour *and* the
> behaviour of the new code, no?