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