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@xxxxxxxxxxxxxxxxxxx/; 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. 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. 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. But I digress :)
