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

[Junio C Hamano <gitster@pobox.com>]

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

> 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.

> @@ -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.

> 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?