Re: [PATCH 5/5] commit-graph: add repo arg to graph readers

[Jonathan Tan <jonathantanmy@google.com>]

> On 6/21/2018 7:06 PM, Jonathan Tan wrote:
> >>> diff --git a/commit.c b/commit.c
> >>> index 0030e79940..38c12b002f 100644
> >>> --- a/commit.c
> >>> +++ b/commit.c
> >>> @@ -317,7 +317,7 @@ struct tree *get_commit_tree(const struct commit *commit)
> >>>          if (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)
> >>>                  BUG("commit has NULL tree, but was not loaded from commit-graph");
> >>>
> >>> -       return get_commit_tree_in_graph(commit);
> >>> +       return get_commit_tree_in_graph(the_repository, commit);
> >> Here..
> >>
> >>>   }
> >>>
> >>>   struct object_id *get_commit_tree_oid(const struct commit *commit)
> >>> @@ -413,7 +413,7 @@ int parse_commit_gently(struct commit *item, int quiet_on_missing)
> >>>                  return -1;
> >>>          if (item->object.parsed)
> >>>                  return 0;
> >>> -       if (parse_commit_in_graph(item))
> >>> +       if (parse_commit_in_graph(the_repository, item))
> >> and here
> >>
> >>> +static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,
> >>> +                                      const struct object_id *commit_oid)
> >>> +{
> >>> +       struct repository r;
> >>> +       struct commit *c;
> >>> +       struct commit_list *parent;
> >>> +
> >>> +       /*
> >>> +        * Create a commit independent of any repository.
> >>> +        */
> >>> +       c = lookup_commit(commit_oid);
> >> .. and this one are unfortunate as the rest of the object store series
> >> has not progressed as far as needed.
> > I think the first 2 are in reverse - get_commit_tree depends on
> > get_commit_tree_in_graph and parse_commit_gently depends on
> > parse_commit_in_graph, so we need the commit-graph functions to be
> > changed first. But I agree about lookup_commit.
> >
> >> The lookup_commit series is out there already, and that will
> >> teach lookup_commit a repository argument. When rerolling
> >> that series I need to switch the order of repo_init and lookup_commit
> >> such that we can pass the repo to the lookup.
> > For future reference, Stefan is talking about this series:
> > https://public-inbox.org/git/20180613230522.55335-1-sbeller@google.com/
> >
> > Let me know if you want to reroll yours on top of mine, or vice versa. I
> > think it's clearer if mine goes in first, though, since (as you said in
> > that e-mail) parse_commit depends on this change in the commit graph.
> 
> I was about to comment that I thought 'parse_commit_in_graph' should 
> take a 'struct commit_graph' instead of 'struct repository', except for 
> these lookup_commit() calls will need the repository parameter.

Not sure what you mean by the lookup_commit() calls (if you're referring
to the part quoted above, that is test code), but
parse_commit_in_graph() has to take a struct repository (or at least a
struct object_store, perhaps) because it needs to load the commit graph
for the repository if it is not already loaded.

(An alternative, of course, is to require the user to explicitly load
the graph, but since parse_commit_in_graph() is called from
parse_commit(), I think that this implicit loading is fine.)

> Please also keep in mind that ds/commit-graph-fsck has already updated 
> this method to parse from a specific graph [1]. I'm just waiting for 
> some things like ds/generation-numbers to get into 'master' and some 
> more object-store patches to be final before I re-roll that series. I 
> mentioned this in a message that I had sent, but apparently didn't make 
> it on the list (so I re-sent it recently).
> 
> [1] 
> https://public-inbox.org/git/20180608135548.216405-4-dstolee@microsoft.com/

Thanks - I see that you introduced a new
parse_commit_in_graph_one(struct commit_graph *, struct commit *) and
made the existing parse_commit_in_graph(struct commit *item) use the
former. Combining our changes would be just adding a repository argument
to parse_commit_in_graph() and passing the graph through
parse_commit_in_graph_one() (after prepare_commit_graph() and ensuring
that the graph exists).