Re: [PATCH 2/3] cat-file: add %(objectmode) atom

From: Jeff King <peff@peff.net>
Date: Jun 4, 2025, 19:36 UTC
Message-ID: <20250604193631.GA1500045@coredump.intra.peff.net>
In-Reply-To: <1ff39aa3c6e613137edde9e05321a7df5c165e99.1748890555.git.gitgitgadget@gmail.com>

On Mon, Jun 02, 2025 at 06:55:54PM +0000, Victoria Dye via GitGitGadget wrote:

Show 5 quoted lines

> Add a formatting atom, used with the --batch-check/--batch-command options,
> that prints the octal representation of the object mode if a given revision
> includes that information, e.g. one that follows the format
> <tree-ish>:<path>. If the mode information does not exist, an empty string
> is printed instead.

Overall, this looks good to me. I have a few small comments below, though I'm not sure if they merit a re-roll or not.

Show 10 quoted lines

> @@ -345,6 +347,9 @@ static int expand_atom(struct strbuf *sb, const char *atom, int len,
>  		else
>  			strbuf_addstr(sb,
>  				      oid_to_hex(&data->delta_base_oid));
> +	} else if (is_atom("objectmode", atom, len)) {
> +		if (!data->mark_query && !(S_IFINVALID == data->mode))
> +			strbuf_addf(sb, "%06o", data->mode);
>  	} else
>  		return 0;
>  	return 1;

Looking at this hunk raised a few questions. Fortunately with answers. ;)

First, in other parts of this if/else chain, when mark_query is set we need to perform some action (usually setting up the object_info pointers). But we _don't_ need to do that here, since we get the mode info "for free" from get_oid_with_context(). Good.

Second, how do we reliably get S_IFINVALID? We can see that the expand_data struct is now initialized with it:

> +#define EXPAND_DATA_INIT  { .mode = S_IFINVALID }

But that seems like it would be a bug, since we only initialize it once, in batch_objects():

Show 20 quoted lines

> @@ -866,7 +872,7 @@ static int batch_objects(struct batch_options *opt)
>  {
>  	struct strbuf input = STRBUF_INIT;
>  	struct strbuf output = STRBUF_INIT;
> -	struct expand_data data;
> +	struct expand_data data = EXPAND_DATA_INIT;
>  	int save_warning;
>  	int retval = 0;
>  
> @@ -875,7 +881,6 @@ static int batch_objects(struct batch_options *opt)
>  	 * object_info to be handed to oid_object_info_extended for each
>  	 * object.
>  	 */
> -	memset(&data, 0, sizeof(data));
>  	data.mark_query = 1;
>  	expand_format(&output,
>  		      opt->format ? opt->format : DEFAULT_FORMAT,
>  
>  static int is_atom(const char *atom, const char *s, int slen)
>  {

...and then call batch_one_object() over and over. So at first glance, doing this:

  (echo HEAD:Makefile; echo HEAD) |
  git cat-file --batch-check='%(objectmode)'

would let the mode from the first object bleed over into the second. But that doesn't happen, because we overwrite expand_data.mode for each object unconditionally, here:

Show 8 quoted lines

> @@ -613,6 +618,7 @@ static void batch_one_object(const char *obj_name,
>  		goto out;
>  	}
>  
> +	data->mode = ctx.mode;
>  	batch_object_write(obj_name, scratch, opt, data, NULL, 0);
>  
>  out:

And there we are relying on ctx.mode, which we get from get_oid_with_context(), which always falls back to S_IFINVALID if no mode is available. Good.

But I think that means that the value set in EXPAND_DATA_INIT is never used, and we could continue to zero-initialize the struct with memset?

That said, it's probably OK to err on the side of over-initializing. The worst case is probably somebody later reading the code being confused about the importance of the line. And at best it may prevent a future code path from unexpectedly reading a funny value.

And on to the third question. In the non-batch code path of cat_one_file(), we do:

          if (obj_context.mode == S_IFINVALID)
                  obj_context.mode = 0100644;

which made me wonder if we should be harmonizing our behavior. But that mode is used only for passing to filter_object() and textconv_object(). Neither of which really care about the mode, and this is mostly just saying "eh, do your regular thing as if it were a blob we found at --path". I suspect we could get the same effect by just passing a hard-coded 100644 to those functions, but probably not worth changing now (and certainly very orthogonal to your patch). But the important thing is we do not really need to worry about being consistent with this line. Good.

-Peff