Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Jul 11, 2018, 19:41 UTC
- 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:
Show 32 quoted lines
>
>>> +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