Re: [PATCH v2 1/8] packfile: add repository to struct `packed_git`

[Taylor Blau <me@ttaylorr.com>]

On Tue, Oct 29, 2024 at 07:46:35AM -0400, karthik nayak wrote:
> >> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> >> index 76d5c20f14..ffee7d3abd 100644
> >> --- a/builtin/fast-import.c
> >> +++ b/builtin/fast-import.c
> >> @@ -765,6 +765,7 @@ static void start_packfile(void)
> >>
> >>  	p->pack_fd = pack_fd;
> >>  	p->do_not_close = 1;
> >> +	p->repo = the_repository;
> >
> > Makes sense. Here we are crafting the packfile by hand, so initializing
> > ->repo directly makes sense here.
> >
> > It would be nice if we could rewrite this in terms of
> > packfile.c:alloc_packed_git(), but that is a static function. Exposing
> > it as non-static is probably showing too much of the internals, so I
> > think leaving this as-is makes sense.
>
> Yes, I did consider that too, but dropped it for the same reasons you
> stated.

Makes sense, and yeah, I think that was a reasonable choice here. I
probably would have done the same thing :-).

> > Hmm. Do we need to include all of repository.h here? I don't think we
> > do, because we never peek into any of the fields of that structure from
> > within this header. So I think you could do something like:
> >
> > --- 8< ---
> > diff --git a/object-store-ll.h b/object-store-ll.h
> > index 6f9f4276e6..bcfae2e1bf 100644
> > --- a/object-store-ll.h
> > +++ b/object-store-ll.h
> > @@ -4,13 +4,13 @@
> >  #include "hashmap.h"
> >  #include "object.h"
> >  #include "list.h"
> > -#include "repository.h"
> >  #include "thread-utils.h"
> >  #include "oidset.h"
> >
> >  struct oidmap;
> >  struct oidtree;
> >  struct strbuf;
> > +struct repository;
> >
> >  struct object_directory {
> >  	struct object_directory *next;
> > --- >8 ---
> >
> > instead of #include-ing the whole thing, which would be preferable.
> >
>
> This is much better, I will patch it in.

Great, thanks.

> I agree, by now you know I prefer readability over fewer characters, so
> it more of an intentional choice. But here, I think it can be '*r'
> though, since it is sort of obvious what 'r' refers to in most cases.
>
> I will change this in all commits in the next version.

;-).

> >>  {
> >>  	struct packed_git *p = xmalloc(st_add(sizeof(*p), extra));
> >>  	memset(p, 0, sizeof(*p));
> >
> > Not at all the fault of this patch, but it feels like a bit of a
> > foot-gun to allocate a bounds-checked version of 'sizeof(*p)+extra',
> > while only zero'ing the first 'sizeof(*p)' bytes. I think in all cases
> > where it actually matters via add_packed_git() we fill out that extra
> > space anyway, but it might be nice cleanup to do something like:
> >
> >     struct packed_git *p;
> >     size_t sz = sizeof(*p) + extra;
> >
> >     p = xcalloc(1, sz);
> >
> > , or something. But that can be dealt with later and/or as #leftoverbits.
> >
>
> Like you stated, there is bunch of refactoring we could do here, I mostly
> don't want to digress and create too much noise, so I will turn a blind
> eye in some cases and leave it for later.

Yeah; to be clear I don't think that we have to address in detail all of
these potential refactorings as a prerequisite to merging this series. I
just wanted to mention them on the list to have some record of having
thought about it, and to avoid the risk that searching for #leftoverbits
returns no results ;-).

Thanks,
Taylor