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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 11, 2024, 22:15 UTC
Message-ID: <xmqq34swo1p2.fsf@gitster.g>
In-Reply-To: <b836bc64ddc06069d1722ae89ca049e9dfce7eec.1710183362.git.gitgitgadget@gmail.com>

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 10 quoted lines

> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index bbf851138ec..73bd78c0b63 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -272,6 +272,7 @@ struct expand_data {
>  	struct object_id oid;
>  	enum object_type type;
>  	unsigned long size;
> +	unsigned short mode;
>  	off_t disk_size;

We are not saving the storage used in this structure by using "unsigned short" due to alignment, so I got curious where the choice came from, but I do not think of any sensible explanation.

Let's to be consistent with the remainder of the system, like how the mode is stored in the in-core index (ce_mode) and in the in-core tree entry (name_entry.mode) and use "unsigned int" instead here.

> +#define EXPAND_DATA_INIT  { .mode = S_IFINVALID }

Thanks for knowing about and choosing to use the INVALID thing (I would have naively chosen 0 without looking around enough and made things inconsistent).

> +	} else if (is_atom("objectmode", atom, len)) {
> +		if (!data->mark_query && !(S_IFINVALID == data->mode))
> +			strbuf_addf(sb, "%06o", data->mode);

Nit.  I think

		if (!data->mark_query && data->mode != S_IFINVALID)

would be a more common way to write the same thing.

Show 14 quoted lines

> @@ -766,7 +772,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;
>  
> @@ -775,7 +781,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));

Nice to see this go with the _INIT thing.

Show 18 quoted lines

> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> index ac1f754ee32..6f25cc20ec6 100755
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -114,9 +114,10 @@ run_tests () {
>      type=$1
>      object_name=$2
>      oid=$(git rev-parse --verify $object_name)
> -    size=$3
> -    content=$4
> -    pretty_content=$5
> +    mode=$3
> +    size=$4
> +    content=$5
> +    pretty_content=$6
>  
>      batch_output="$oid $type $size
>  $content"

I wonder if appending $mode as an optional thing at the end would have made the patch less noisy? After all, the expectation above that does not have $mode, and the tests that are expected to produce output that match the expectation, do not have to change. And the existing invocation of run_tests that do not care about $mode do not have to change.

But I guess if the damage is only with the above 7-lines (which would become just 1 if we made mode the $6 last tthing), it is not a huge deal either way?