Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs

From: Elijah Newren <newren@gmail.com>
Date: Sep 29, 2021, 04:22 UTC
Message-ID: <CABPp-BG_qigBoirMGR-Yk9Niyxt0UmYCEqojsYxbSEarLAmraA@mail.gmail.com>
In-Reply-To: <87r1d9xh71.fsf@evledraar.gmail.com>

On Tue, Sep 28, 2021 at 1:00 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 28 quoted lines

>
> On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:
>
> I commented just now on how this API is duplicated between here &
> another in-flight series in
> https://lore.kernel.org/git/87v92lxhh4.fsf@evledraar.gmail.com/; Just
> comments on this patch here:
>
> > diff --git a/tmp-objdir.c b/tmp-objdir.c
> > index b8d880e3626..9f75a75d1c0 100644
> > --- a/tmp-objdir.c
> > +++ b/tmp-objdir.c
> > @@ -288,7 +288,36 @@ const char **tmp_objdir_env(const struct tmp_objdir *t)
> >       return t->env.v;
> >  }
> >
> > +const char *tmp_objdir_path(struct tmp_objdir *t)
> > +{
> > +     return t->path.buf;
> > +}
>
> Not your fault & this pre-dates your patch, but FWIW I prefer our APIs
> that don't have these "hidden struct" shenanigans (like say "struct
> string_list") so a caller could just access this, we can just declare it
> "const" appropriately.
>
> We're also all in-tree friends here, so having various accessors for no
> reason other than to access struct members seems a bit too much.

That's a fair point, but just to play counterpoint for a minute...

FWIW, I dislike when our public facing APIs are polluted with all kinds of internal details. merge-recursive being a case in point. When writing merge-ort, although I had a struct with public fields, that struct also contained an opaque struct (pointer) within it to hide several private fields. (I would have liked to hide or remove a few more fields, but couldn't do so while the merge_recursive_options struct was shared between both merge-ort and merge-recursive.)

That said, I agree it can certainly be overdone, and tmp_objdir is pretty simple. However, sometimes even in simple cases I like when folks make use of an opaque struct because it means folks put some effort into thinking more about the API that should be exposed. That's something we as a project have often overlooked in the past, as evidenced both by our one-shot usage mentality, and the existence of external projects like libgit2 attempting to correct this design shortcoming. I'd like git to move more towards being structured as a reusable library as well as a useful command-line tool.

> All of which is to say if you + Neeraj come up with some common API I
> for one wouldn't mind moving the struct deceleration to tmp-objdir.h,
> but it looks like in his version it can stay "private" more easily.

Peff's ideas to just delete and recreate the temporary object store after every merge would let me reduce the API extensions and keep things more private. So I think our ideas are converging a bit. :-)

> In this particular case I hadn't understood on a previous reading of
> tmp-objdir.c why this "path" isn't a statically allocated "char
> path[PATH_MAX]", or a least we that hardcoded "1024" should be a
> PATH_MAX, as it is in some other cases.

Actually, PATH_MAX shouldn't be used:

https://lore.kernel.org/git/7d1237c7-5a83-d766-7d93-5f0d59166067@web.de/ https://lore.kernel.org/git/20180720180427.GE22486@sigill.intra.peff.net/