From: Derrick Stolee <stolee@gmail.com>
Date: Fri, 29 Jun 2018 12:47:55 GMT
Subject: Re: [PATCH v7 07/22] commit-graph: add 'verify' subcommand
Message-ID: <f6116a1b-8e5a-9cb9-9c8f-53f56a9b79da@gmail.com>
In-Reply-To: <20180627215926.119376-1-jonathantanmy@google.com>

On 6/27/2018 5:59 PM, Jonathan Tan wrote:
>> +int verify_commit_graph(struct repository *r, struct commit_graph *g)
> I haven't had the time to review this patch set, but I did rebase my
> object store refactoring [1] on this and wrote a test:
>
>      static void test_verify_commit_graph(const char *gitdir, const char *worktree)
>      {
>      	struct repository r;
>      	char *graph_name;
>      	struct commit_graph *graph;
>      
>      	repo_init(&r, gitdir, worktree);
>      
>      	graph_name = get_commit_graph_filename(r.objects->objectdir);
>      	graph = load_commit_graph_one(graph_name);
>      
>      	printf("verification returned %d\n", verify_commit_graph(&r, graph));
>      
>      	repo_clear(&r);
>      }
>
> However, it doesn't work because verify_commit_graph() invokes
> parse_commit_internal(), which tries to look up replace refs in
> the_repository.
>
> I think that verify_commit_graph() should not take a repository argument
> for now. To minimize churn on the review of this patch set, and to
> minimize diffs when we migrate parse_commit_internal() (and likely other
> functions) to take in a repository argument, I would be OK with
> something like the following instead:
>
>      int verify_commit_graph(struct commit_graph *g)
>      {
> 	    /*
> 	     * NEEDSWORK: Make r into a parameter when all functions
> 	     * invoked by this function are not hardcoded to operate on
> 	     * the_repository.
> 	     */
> 	    struct repository *r = the_repository;
> 	    /* ... */
>
> As for my rebased refactoring, I'll send the patches to the mailing list
> once Junio updates ds/commit-graph-fsck with these latest changes, so
> that I can rebase once again on that and ensure that everything still
> works.
>
> [1] https://public-inbox.org/git/cover.1529616356.git.jonathantanmy@google.com/

Thanks for looking into this, Jonathan. At some point I took my series 
and moved it on top of Stefan's lookup_object() series, but then moved 
off of it since those commits were not in 'pu'. This 'struct repository 
*r' didn't get removed in that process.

Thanks,

-Stolee