From: Derrick Stolee <stolee@gmail.com>
Date: Fri, 06 Apr 2018 19:41:53 GMT
Subject: Re: [PATCH v2 0/4] Lazy-load trees when reading commit-graph
Message-ID: <f3a8b9ff-4aba-100f-e59e-613f6a094513@gmail.com>
In-Reply-To: <20180406192146.GC921@sigill.intra.peff.net>

On 4/6/2018 3:21 PM, Jeff King wrote:
> On Fri, Apr 06, 2018 at 07:09:30PM +0000, Derrick Stolee wrote:
>
>> Derrick Stolee (4):
>>    treewide: rename tree to maybe_tree
>>    commit: create get_commit_tree() method
>>    treewide: replace maybe_tree with accessor methods
>>    commit-graph: lazy-load trees for commits
> I gave this only a cursory read, but it addresses my concern from the
> previous round.
>
> If I were doing it myself, I probably would have folded patches 1 and 3
> together. They are touching all the same spots, and it would be an error
> for any case converted in patch 1 to not get converted in patch 3. I'm
> assuming you caught them all due to Coccinelle, though IMHO it is
> somewhat overkill here. By folding them together the compiler could tell
> you which spots you missed.
>
> And going forward, I doubt it is going to be a common error for people
> to use maybe_tree directly. Between the name and the warning comment,
> you'd have to really try to shoot yourself in the foot with it. The
> primary concern was catching people using the existing "tree" name,
> whose semantics changed.
>
> All that said, I'm fine with having it done this way, too.

Thanks. As a double-check that I caught all of the 'maybe_tree' 
accesses, I ran the following:

$ git grep maybe_tree | grep -v get_commit_tree
commit-graph.c: item->maybe_tree = NULL;
commit-graph.c: c->maybe_tree = lookup_tree(&oid);
commit-graph.c: return c->maybe_tree;
commit-graph.c: if (c->maybe_tree)
commit-graph.c:         return c->maybe_tree;
commit.c:       if (commit->maybe_tree || !commit->object.parsed)
commit.c:               return commit->maybe_tree;
commit.c:       item->maybe_tree = lookup_tree(&parent);
commit.h:       struct tree *maybe_tree;
contrib/coccinelle/commit.cocci:- &c->maybe_tree->object.oid
contrib/coccinelle/commit.cocci:- c->maybe_tree->object.oid.hash
contrib/coccinelle/commit.cocci:- c->maybe_tree
contrib/coccinelle/commit.cocci:+ c->maybe_tree = s
contrib/coccinelle/commit.cocci:+ return c->maybe_tree;
merge-recursive.c:      commit->maybe_tree = tree;

Thanks,
-Stolee