From: Junio C Hamano Date: Wed, 30 Sep 2026 17:42:10 GMT Subject: Re: [PATCH 1/4] pack-objects: introduce `stdin_packs_context` struct Message-ID: In-Reply-To: <64bb13e2db2e5c22e842c188e08861d63e99dc77.1790731662.git.me@ttaylorr.com> Taylor Blau writes: > static int add_object_entry_from_pack(const struct object_id *oid, > struct packed_git *p, > uint32_t pos, > void *_data) > { > + struct stdin_packs_context *ctx = _data; > off_t ofs; > struct object_info oi = OBJECT_INFO_INIT; > enum object_type type = OBJ_NONE; > @@ -3827,7 +3833,6 @@ static int add_object_entry_from_pack(const struct object_id *oid, > die(_("could not get type of object %s in pack %s"), > oid_to_hex(oid), p->pack_name); > } else if (type == OBJ_COMMIT) { > - struct rev_info *revs = _data; > /* > * commits in included packs are used as starting points > * for the subsequent revision walk > @@ -3840,7 +3845,7 @@ static int add_object_entry_from_pack(const struct object_id *oid, > * However, we'll only add those objects to the packing > * list after checking `want_object_in_pack()` below. > */ > - add_pending_oid(revs, NULL, oid, 0); > + add_pending_oid(ctx->revs, NULL, oid, 0); > } > > if (!want_object_in_pack(oid, 0, &p, &ofs)) > @@ -3954,8 +3959,9 @@ static int stdin_packs_include_check(struct commit *commit, void *data) > } We used to take _data that is rev_info, but no longer. We lost decl for "struct rev_info *revs" and rewrote its only use to directly reference ctx->revs. As long as the result compiles, we know there is no stray reference to "revs" left in this function, so the rewrite is complete. It is rare but I love this kind of patch whose correctness can be seen without reading beyond the context ;-) > static void stdin_packs_add_pack_entries(struct strmap *packs, > - struct rev_info *revs) > + struct stdin_packs_context *ctx) > { > + struct rev_info *revs = ctx->revs; > struct string_list keys = STRING_LIST_INIT_NODUP; > struct string_list_item *item; > struct hashmap_iter iter; > @@ -3994,15 +4000,14 @@ static void stdin_packs_add_pack_entries(struct strmap *packs, > (info->kind & STDIN_PACK_EXCLUDE_OPEN)) > for_each_object_in_pack(info->p, > add_object_entry_from_pack, > - revs, > + ctx, > ODB_FOR_EACH_OBJECT_PACK_ORDER); > } > > string_list_clear(&keys, 0); > } Ditto. > -static void stdin_packs_read_input(struct rev_info *revs, > - enum stdin_packs_mode mode) > +static void stdin_packs_read_input(struct stdin_packs_context *ctx) We used to take two separately, but now we can take them in one package. > { > struct strbuf buf = STRBUF_INIT; > struct strmap packs = STRMAP_INIT; > @@ -4017,7 +4022,7 @@ static void stdin_packs_read_input(struct rev_info *revs, > continue; > else if (*key == '^') > kind = STDIN_PACK_EXCLUDE_CLOSED; > - else if (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW) > + else if (*key == '!' && ctx->mode == STDIN_PACKS_MODE_FOLLOW) > kind = STDIN_PACK_EXCLUDE_OPEN; > > if (kind != STDIN_PACK_INCLUDE) > @@ -4082,19 +4087,23 @@ static void stdin_packs_read_input(struct rev_info *revs, > info->p = p; > } > > - stdin_packs_add_pack_entries(&packs, revs); > + stdin_packs_add_pack_entries(&packs, ctx); > > strbuf_release(&buf); > strmap_clear(&packs, 1); > } The same argument tells us that this is the right refactoring as long as the result compiles. > -static void add_unreachable_loose_objects(struct rev_info *revs); > +static void add_unreachable_loose_objects(struct stdin_packs_context *ctx); > > static void read_stdin_packs(struct repository *repo, > enum stdin_packs_mode mode, int rev_list_unpacked) > { > int prev_fetch_if_missing = repo->fetch_if_missing; > struct rev_info revs; > + struct stdin_packs_context ctx = { > + .revs = &revs, > + .mode = mode, > + }; > > /* > * The revision walk may hit objects that are promised, only. As the > @@ -4131,9 +4140,9 @@ static void read_stdin_packs(struct repository *repo, > */ > ignore_packed_keep_in_core_open = 1; > } > - stdin_packs_read_input(&revs, mode); > + stdin_packs_read_input(&ctx); > if (rev_list_unpacked) > - add_unreachable_loose_objects(&revs); > + add_unreachable_loose_objects(&ctx); > > if (prepare_revision_walk(&revs)) > die(_("revision walk setup failed")); Ditto. > @@ -4541,7 +4550,7 @@ static void add_objects_in_unpacked_packs(void) > static int add_loose_object(const struct object_id *oid, const char *path, > void *data) > { > - struct rev_info *revs = data; > + struct stdin_packs_context *ctx = data; > enum object_type type = odb_read_object_info(the_repository->objects, oid, NULL); > > if (type < 0) { > @@ -4563,8 +4572,8 @@ static int add_loose_object(const struct object_id *oid, const char *path, > add_object_entry(oid, type, "", 0); > } > > - if (revs && type == OBJ_COMMIT) > - add_pending_oid(revs, NULL, oid, 0); > + if (ctx && type == OBJ_COMMIT) > + add_pending_oid(ctx->revs, NULL, oid, 0); > > return 0; > } This one, ... > @@ -4574,10 +4583,10 @@ static int add_loose_object(const struct object_id *oid, const char *path, > * add_object_entry will weed out duplicates, so we just add every > * loose object we find. > */ > -static void add_unreachable_loose_objects(struct rev_info *revs) > +static void add_unreachable_loose_objects(struct stdin_packs_context *ctx) > { > for_each_loose_file_in_source(the_repository->objects->sources, > - add_loose_object, NULL, NULL, revs); > + add_loose_object, NULL, NULL, ctx); > } ... together with the change to add_unreachable_loose_objects() here, it is not immediately obvious if we do not have to worry about the case where (ctx && !ctx->revs). Given that 'struct stdin_packs_context' is a new structure, the fact that the instantiation on the stack in read_stdin_packs() is the only one that can give us a non-NULL 'ctx' pointer we can see in this patch means that a non-NULL 'ctx' cannot have a NULL '.revs' pointer in it. Again, as long as this patch alone compiles, we know this refactoring is correct. It is not clear to me what the implication of assuming a non-NULL 'ctx' always means a non-NULL 'ctx->revs' is for the code health in the longer term, though. Thanks.