> -----Original Message----- > From: Derrick Stolee <derrickstolee@xxxxxxxxxx> > Sent: Monday, March 07, 2022 12:27 PM > To: Jacob Keller <jacob.keller@xxxxxxxxx>; Junio C Hamano > <gitster@xxxxxxxxx> > Cc: Keller, Jacob E <jacob.e.keller@xxxxxxxxx>; Git mailing list > <git@xxxxxxxxxxxxxxx> > 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@xxxxxxxxx> wrote: > >> > >> "Keller, Jacob E" <jacob.e.keller@xxxxxxxxx> 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 Ok. The problem is that specific test does not behave the same. In fact it *cannot* behave the same because we're trying to test the non-commit-graph flow there. Since i'm dropping it in v3 I won't worry too much about it. Thanks, Jake
