On Fri, 31 Jan 2020 at 11:20, Jeff King <peff@xxxxxxxx> wrote: > > On Fri, Jan 31, 2020 at 07:52:02AM +0100, Martin Ågren wrote: > > > > --- 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. Thanks for explaining the issue. > 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()). ... and these ways of addressing it. Martin
