Re: [PATCH 1/4] pack-objects: introduce `stdin_packs_context` struct
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 30, 2026, 17:42 UTC
- Message-ID
- <xmqqik3mbpql.fsf@gitster.g>
- In-Reply-To
- <64bb13e2db2e5c22e842c188e08861d63e99dc77.1790731662.git.me@ttaylorr.com>
Taylor Blau <ttaylorr@openai.com> writes:
Show 28 quoted lines
> 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 ;-)
Show 19 quoted lines
> 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.
Show 22 quoted lines
> {
> 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.
Show 27 quoted lines
> -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.
Show 20 quoted lines
> @@ -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, ...
Show 11 quoted lines
> @@ -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.