From: Derrick Stolee <stolee@gmail.com>
Date: Wed, 11 Jul 2018 19:41:42 GMT
Subject: Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers
Message-ID: <1f0ab606-cf7d-e61c-1761-7a2982a2628e@gmail.com>
In-Reply-To: <20180710173147.104757-1-jonathantanmy@google.com>

On 7/10/2018 1:31 PM, Jonathan Tan wrote:
>
>>> +static void test_get_commit_tree_in_graph(const char *gitdir,
>>> +					  const char *worktree,
>>> +					  const struct object_id *commit_oid)
>>> +{
>>> +	struct repository r;
>>> +	struct commit *c;
>>> +	struct tree *tree;
>>> +
>>> +	/*
>>> +	 * Create a commit independent of any repository.
>>> +	 */
>>> +	c = lookup_commit(commit_oid);
>> Would this be more accurate to say we are creating a commit object
>> stored in the object cache of the_repository? How would you expect this
>> to work if/when lookup_commit() takes an arbitrary repository? You want
>> to provide &r, right (after initializing)?
> Yes, you're right - Stefan too mentioned that this will need to be moved
> below lookup_commit(). I'm not sure what the best way is to handle this
> - maybe move this, and add a "needswork" stating that we need to pass r
> to lookup_commit once it supports taking in a repository argument, as an
> aid to the person who performs the merge. I'll do that if a reroll is
> needed.
>
>> Also, this will conflict with sb/object-store-lookup, won't it? I'm
>> guessing this is why you didn't touch the "git commit-graph
>> [write|verify]"code paths.
> It will conflict because of the change to lookup_commit(), but the only
> new code I'm writing is in t/helper/test-repository.c, so hopefully the
> merge won't be too tedious. The main reason why I didn't touch the
> writing/verifying part is to reduce the size of this patch set, and
> because that change is not needed to update parse_commit() and others.

I guess my main complaint is that this won't be an actual "merge" 
conflict, but the result will not compile. Since Stefan already has a 
series out that changes this method, I recommend basing your series on 
it (in addition to basing it on ds/commit-graph-fsck).

Thanks,

-Stolee