[PATCH 1/2] lookup_commit_in_graph(): use prepare_commit_graph() to check for graph

[Jeff King <peff@peff.net>]

We exit early from lookup_commit_in_graph() if the commit_graph pointer
is NULL, under the assumption that we don't have a graph to look at. But
the graph pointer is lazy-loaded; if no other code happens to have
called prepare_commit_graph(), we'll incorrectly assume that one isn't
available at all.

This has a pretty small performance impact in practice, because the
fallback will generally be to call parse_object() instead. That ends up
in parse_commit_buffer(), which loads the graph data itself. So the
first commit we see won't use the graph, but subsequent ones will. Since
using the graph is just an optimization there's generally no
user-visible difference, but if you instrument rev-list like so:

  diff --git a/revision.c b/revision.c
  index ee702e498a..63c488ffb6 100644
  --- a/revision.c
  +++ b/revision.c
  @@ -381,6 +381,9 @@ static struct object *get_reference(struct rev_info *revs, const char *name,
           * parsing commit data from disk.
           */
          commit = lookup_commit_in_graph(revs->repo, oid);
  +       warning("%s %s in commit graph",
  +               commit ? "found" : "did not find",
  +               name);
          if (commit)
                  object = &commit->object;
          else

and run (in git.git):

  git commit-graph write --reachable
  git rev-list origin/master origin/next >/dev/null

you'll see that we fail to find the first one:

  warning: did not find origin/master in commit graph
  warning: found origin/next in commit graph

After this patch, you'll see that we find both:

  warning: found origin/master in commit graph
  warning: found origin/next in commit graph

Even though the performance implication is small here, there are two
important reasons to do this:

  - it's downright confusing if you are hunting a bug triggered by the
    use of the commit graph. It may or may not trigger depending on the
    number and ordering of tips you ask for.

  - prepare_commit_graph() has other policy logic, too. In particular,
    if we've loaded a commit graph and then disabled the graph via
    disable_commit_graph(), that should take precedence.

    I'm not sure if this can trigger bad behavior in practice. The only
    caller there is upload-pack's deepen_by_rev_list(), which should be
    avoiding the commit graph for its traversal tips, but probably
    wasn't before this patch. Whether you could come up with a case
    where that mattered is unclear. Still, this is obviously the right
    thing to be doing.

Signed-off-by: Jeff King <peff@peff.net>
---
I couldn't find any other reason to avoid calling prepare_commit_graph()
here (especially since it ends up lazy-loaded as discussed above). The
cost of the call should not be high; after the first call, it is
simplified down to a few integer checks.

 commit-graph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index f2a36032f8..aef076e145 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -901,7 +901,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje
 	struct commit *commit;
 	uint32_t pos;
 
-	if (!repo->objects->commit_graph)
+	if (!prepare_commit_graph(repo))
 		return NULL;
 	if (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))
 		return NULL;
-- 
2.37.3.1134.gfd534b3986