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

From: "Martin Ågren" <martin.agren@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Taylor Blau <me@ttaylorr.com>,
	Git Mailing List <git@vger.kernel.org>,
	Derrick Stolee <dstolee@microsoft.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 2/6] commit-graph.h: store object directory in 'struct commit_graph'
Date: Fri, 31 Jan 2020 20:19:12 +0100	[thread overview]
Message-ID: <CAN0heSo1a3s5chz=FD-XnLncLwH_N59BPZzqtEUC_nJ8sLJtdA@mail.gmail.com> (raw)
In-Reply-To: <20200131102049.GC2916051@coredump.intra.peff.net>

On Fri, 31 Jan 2020 at 11:20, Jeff King <peff@peff.net> 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