Re: [PATCH v3 1/9] packfile: add repository to struct `packed_git`
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Oct 30, 2024, 20:00 UTC
- Message-ID
- <ZyKQRu+1v6IJjbSR@nand.local>
- In-Reply-To
- <5afb9af0afdf6062b16dea2d1347170485e7d73e.1730297934.git.karthik.188@gmail.com>
On Wed, Oct 30, 2024 at 03:32:26PM +0100, Karthik Nayak wrote:
Show 6 quoted lines
> [...] > > We do need to consider that a pack file could be part of the alternates > of a repository, but considering that we only have one repository struct > and also that we currently anyways use 'the_repository'. We should be > OK with this change.
Nicely explained.
Show 20 quoted lines
> diff --git a/object-store-ll.h b/object-store-ll.h
> index 53b8e693b1..e8a22ab5fc 100644
> --- a/object-store-ll.h
> +++ b/object-store-ll.h
> @@ -10,6 +10,7 @@
> struct oidmap;
> struct oidtree;
> struct strbuf;
> +struct repository;
>
> struct object_directory {
> struct object_directory *next;
> @@ -135,6 +136,10 @@ struct packed_git {
> */
> const uint32_t *mtimes_map;
> size_t mtimes_size;
> +
> + /* repo dentoes the repository this packed file belongs to */
> + struct repository *r;
> +Hmm. What I meant in my earlier suggestion was that we should leave the member of the struct called "repo", but change the name only in function arguments.
Sorry to split hairs, but I am somewhat opposed to having such a short variable name in a struct. In either event, the comment should be made consistent with the variable name.
Show 13 quoted lines
> /* something like ".git/objects/pack/xxxxx.pack" */ > char pack_name[FLEX_ARRAY]; /* more */ > }; > diff --git a/packfile.c b/packfile.c > index 9560f0a33c..1423f23f57 100644 > --- a/packfile.c > +++ b/packfile.c > @@ -217,11 +217,12 @@ uint32_t get_pack_fanout(struct packed_git *p, uint32_t value) > return ntohl(level1_ofs[value]); > } > > -static struct packed_git *alloc_packed_git(int extra) > +static struct packed_git *alloc_packed_git(struct repository *r, int extra)
This spot I would leave alone.
Show 5 quoted lines
> {
> struct packed_git *p = xmalloc(st_add(sizeof(*p), extra));
> memset(p, 0, sizeof(*p));
> p->pack_fd = -1;
> + p->r = r;And this spot I would change to:
p->repo = r;
The rest is looking good.
Thanks, Taylor