On 2/28/2022 3:20 PM, Keller, Jacob E wrote:

On 2/28/2022 11:50 AM, Derrick Stolee wrote:

quoted

On 2/28/2022 2:07 PM, Jacob Keller wrote:

quoted

From: Jacob Keller <redacted>
+/* Check if a commit is before the cutoff. Prioritize generation numbers
+ * first, but use the commit timestamp if we lack generation data.
+ */
+static int commit_is_before_cutoff(struct commit *commit)
+{
+	if (generation_cutoff < GENERATION_NUMBER_INFINITY)
+		return commit_graph_generation(commit) < generation_cutoff;
+
+	return commit->date < cutoff;
+}

There are two subtle things going on here when generation_cutoff is
zero:

1. In a commit-graph with topological levels _or_ generation numbers v2,
   commit_graph_generation(commit) will always be positive, so we don't
   need to do the lookup.

I.e. once we have a generation_cutoff of 0 we can just completely bypass
the lookup, saving some time.

I think we can do "return generation_cutoff &&
commit_graph_generation(commit) < generation_cutoff"

quoted

2. If the commit-graph was written by an older Git version before
   topological levels were implemented, then the generation of commits
   in the commit-graph are all zero(!). This means that the logic here
   would be incorrect for the "all" case.

The fix for both cases is to return 1 if generation_cutoff is zero:

I think you mean return 0? Because this returns true if the commit is
before the cutoff, but false if its not. (i.e. if its true, we should
stop searching this commit, but if its false we should continue searching?

Yes, sorry I had it mixed up. Your generation_cutoff && ... approach
will work in that case.

quoted

quoted

+test_expect_success 'name-rev --all works with non-monotonic' '

This is working because of the commit-graph, right? We still have
it from the previous test, so we aren't testing that this works
when we only have the commit date as a cutoff.

I can either extend this test or add a separate test which covers this.
The test failed before I added the commit graph line.

quoted

quoted

+	(
+		cd non-monotonic &&
+
+		cat >expect <<-\EOF &&
+		E
+		D
+		D~1
+		D~2
+		A
+		EOF
+
+		git log --pretty=%H >revs &&
+		git name-rev --tags --annotate-stdin --name-only <revs >actual &&
+
+		test_cmp expect actual
+	)

Do you want to include a test showing the "expected" behavior
when there isn't a commit-graph file? You might need to delete
an existing commit-graph (it will exist in the case of
GIT_TEST_COMMIT_GRAPH=1).

This test actually is intended to show that it works regardless of
whether we have a commit graph. (Because in --annotate-stdin mode we
disable the heuristic since we don't know what commits we'll see in advance)

Is there a good way to delete the graph file?

The basic way is "rm -rf .git/info/commit-graph*" to be absolutely
sure (there might be an incremental commit-graph which appears as
a "commit-graphs" directory).
 

quoted

I also see that you intended to test the "--all" option, which
is not included in your test. That's probably the real key to
getting this test to work correctly. Deleting the graph will
probably cause a failure on this test unless "--all" is added.

Actually both --all and --annotate-stdin disable the heuristic. However,
I think adding a test for both makes sense.

Ah. OK. They could be assertions within the same test since the
output is expected to be the same.

Thanks,
-Stolee