Re: [PATCH v2 0/4] Lazy-load trees when reading commit-graph

From: Jeff King <peff@peff.net>
Date: Apr 6, 2018, 19:21 UTC
Message-ID: <20180406192146.GC921@sigill.intra.peff.net>
In-Reply-To: <20180406190919.167092-1-dstolee@microsoft.com>

On Fri, Apr 06, 2018 at 07:09:30PM +0000, Derrick Stolee wrote:

Show 5 quoted lines

> 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.

-Peff