Re: [PATCH GSoC v2 4/6] fetch-object-info: parse type from server response

From: Jeff King <peff@peff.net>
Date: Aug 1, 2026, 23:14 UTC
Message-ID: <20260801231437.GA2097059@coredump.intra.peff.net>
In-Reply-To: <DKDYGQRTSF2W.25OU81K306HJN@gmail.com>

On Sun, Aug 02, 2026 at 12:20:28AM +0200, Pablo Sabater wrote:

Show 10 quoted lines

> > We are probably using this pointer indirection to say "ah, '.typep'
> > is NULL so the caller did not ask for this information and the
> > object layer does not have to provide it", plus "'.typep' is NULL
> > so the engine did not give this information for the object".  But we
> > can do so with two bitfields
> >
> >     unsigned type_asked:1,
> >              type_valid:1;
> >
> > instead of paying ~24 bytes or more of heap allocation overhead.

Yes, this conditional loading is exactly how we use the pointers. I agree that a bool would be smaller, though I doubt it really matters in practice. You shouldn't have a large number of object_info structs. You should have one that you use over and over. And it does not point to a heap allocation, but usually to a stack variable in the caller.

I don't think you'd need type_valid (at least not as the object_info code is written now). If you ask for it, then either the query is satisfied, or we return an error.

I think the pointer system goes all the way back to 9a49059022 (sha1_object_info_extended(): expose a bit more info, 2011-05-12). It is mostly just mirroring the pointers that would be passed directly to the function (but marshalling them in a struct so callers don't have to pass a zillion NULLs). So:

  read_object_info(oid, &size);

became:

  struct object_info query;
  query.sizep = &size;
  read_object_info(oid, &query);

One minor benefit the pointer system gets you is that the compiler can more easily tell what has been loaded. Imagine that we had a type_asked bool, but you forgot to set it. Now you look at oi.type, and it's garbage (or maybe some sentinel value). But the compiler has no clue without looking at the innards of the read_object_info() function.

Whereas with the pointers, you do this:

  enum object_type type;
  struct object_info oi = OBJECT_INFO_INIT;

  oi.typep = &type; /* what if we forget this? */

  read_object_info(oid, &oi);
  do_something(type);

If you forget the pointer assignment, the compiler will realize that "type" never got passed to anybody and complain.

I don't know how valuable that is in practice, though.

Anyway, that is the history.

Show 6 quoted lines

> This is related to what had to be done to fix a bug at "contents"
> commands a few days ago [1].
> 
> In that patch it had to save the previous state of typep and then
> restore it, because other commands like "info" and this series one
> "remote-object-info" use this pointer for the "is this asked?" question.

Yes, though you'd have the same thing with bools. You'd have to save type_asked, set it, and then restore it.

Show 15 quoted lines

> If we take a look at expand_atom():
> 
> 	...
> 	} else if (is_atom("objecttype", atom, len)) {
> 		if (data->mark_query) {
> 			data->info.typep = &data->type;
> 		} else {
> 			const char *t = type_name(data->type);
> 			strbuf_addstr(sb, t ? t : "");
> 		}
> 	...
> 
> expand_atom() has two responsibilities, it is called at the start to map
> which atoms are asked (when data->mark_query), and a second to expand
> those atoms.

Yep. But again, you'd have to set the bools somewhere. And it would be here (in the mark_query half).

Show 9 quoted lines

> For example, typep being non-NULL does this effect on these commands:
> 
> info: makes a type lookup, and fills type.
> 
> remote-object-info: typep is directly used to know whether a client has
>                     asked for %(objecttype).
> 
> For both commands what we pay is extra work because at the end the data
> shown is the one expanded from the format.

There should be no extra work. We do a single read_object_info() that grabs all of the data and writes it into expand_data. If we are getting data from elsewhere (say, a remote server) then we should not be using object_info at all! The concrete data goes into expand_data, which is a data structure specific to cat-file expansion.

-Peff