Re: [PATCH 2/6] commit-graph.h: store object directory in 'struct commit_graph'

From: Jeff King <peff@peff.net>
Date: Jan 31, 2020, 10:20 UTC
Message-ID: <20200131102049.GC2916051@coredump.intra.peff.net>
In-Reply-To: <CAN0heSrrrWjBKnzjMfzEkTMVTge2AfVdwsp6D5Mx==5S8-ZLJQ@mail.gmail.com>

On Fri, Jan 31, 2020 at 07:52:02AM +0100, Martin Ågren wrote:

Show 7 quoted lines

> > --- a/commit-graph.c
> > +++ b/commit-graph.c
> 
> > +struct object_directory *find_odb(struct repository *r, const char *obj_dir)
> 
> This doesn't look commit-graph related -- could/should it go somewhere
> else?

I think the right place is actually as a static inside builtin/commit-graph.c, as this is really about handling its weird --object-dir options.

But it can't go there in this patch, because there's a caller in commit-graph.c. In patch 4, we convert write_commit_graph() to take an odb, too, and that call goes away. At that point, we could move it into the builtin as a static.

Ideally we could flip the order of this patch and patch 4, but that doesn't work either: by switching to an odb we lose our path normalization, but if the other side hasn't switched either, then we can't just compare odb pointers. It would be a temporary regression.

So there's a circular dependency between the two patches. I think we ought to do done of:

  - move find_odb() to a static as a cleanup on top

  - squash those two patches together into a single

  - swap the patch order, but have write_commit_graph_ctx store both the
    "odb" _and_ the normalized copy of the path we do now. That leaves
    it correct, and then it can be cleaned up in favor of an odb pointer
    comparison in patch 5, along with the rest of the normalized bits.

I'm OK with any of those. The second two have the added bonus that we could introduce the die() behavior into find_odb() immediately, and explain it (there's another temporary weirdness in this patch where specifying an --object-dir outside of the repository becomes a silent noop, and then the next patch turns it into an error, but that could all be done in a single step when we introduce find_odb()).

> I think you could drop `cmp` and that final check, and write the loop
> body as "if (!strcmp(...)) break". You could also have an empty loop
> body, but I wouldn't go there -- I'd find that less readable. (Maybe
> that's just me.)

Yeah, I believe you are correct (and this is a nice simplification worth doing).

-Peff