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

> -----Original Message-----
> From: Derrick Stolee <derrickstolee@github.com>
> Sent: Monday, March 07, 2022 12:27 PM
> To: Jacob Keller <jacob.keller@gmail.com>; Junio C Hamano
> <gitster@pobox.com>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Git mailing list
> <git@vger.kernel.org>
> Subject: Re: [PATCH] name-rev: use generation numbers if available
> 
> On 3/7/2022 3:22 PM, Jacob Keller wrote:
> > On Wed, Mar 2, 2022 at 5:10 PM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> "Keller, Jacob E" <jacob.e.keller@intel.com> writes:
> >>
> >>> 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.
> >>
> >> Sounds like a sensible thing to do.
> >>
> >> In any case, in the current patch, #2 is not working in
> >> linux-TEST-vars job at CI.  You can visit this URL
> >>
> >>
> https://github.com/git/git/runs/5400048732?check_suite_focus=true#step:4:680
> 62
> >>
> >> while logged into your GitHub account for details.
> >
> > Looks like this job sets all the TEST variables including
> > GIT_TEST_COMMIT_GRAPH=1? The negative test passes because the commit
> > graph is enforced on and we then succeed even though we were trying to
> > test the negative case.
> >
> > I'm going to remove that test in v3 anyways, so I don't think it is a
> > big deal. However, I wonder is there some way to mark a test as
> > explicitely "don't run if GIT_TEST_COMMIT_GRAPH is set"?
> 
> Typically, we try to keep them compatible in both cases. However,
> you can set GIT_TEST_COMMIT_GRAPH=0 for the test, if you want. Be
> careful to only change it locally to the single test, not "globally"
> to the full test script.
> 
> Thanks,
> -Stolee