"Victoria Dye via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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?
