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

Commit Message

Victoria Dye March 11, 2024, 6:56 p.m. UTC

From: Victoria Dye <[e-mail]>

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.

Signed-off-by: Victoria Dye <[e-mail]>
Signed-off-by: Johannes Schindelin <[e-mail]>
---
 Documentation/git-cat-file.txt |  5 +++++
 builtin/cat-file.c             |  9 +++++++--
 t/t1006-cat-file.sh            | 23 +++++++++++++++--------
 3 files changed, 27 insertions(+), 10 deletions(-)

Comments

Junio C Hamano March 11, 2024, 10:15 p.m. UTC | #1

"Victoria Dye via GitGitGadget" <[e-mail]> 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?

Junio C Hamano March 13, 2024, 9:23 p.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

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

Unfortunately, not really.

If we made the optional mode as the last thing, and allow
run_tests() to be called without an explicit "", it may have avoided
unnecessary conflicts with eb/hash-transition topic.  Interested
folks can see how well these three patches plays with the other
topic by trying to merge it to 'seen'.

Patch

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index bd95a6c10a7..de29e6d79d9 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -292,6 +292,11 @@  newline. The available atoms are:
 `objecttype`::
 	The type of the object (the same as `cat-file -t` reports).
 
+`objectmode`::
+	If the specified object has mode information (such as a tree or
+	index entry), the mode expressed as an octal integer. Otherwise,
+	empty string.
+
 `objectsize`::
 	The size, in bytes, of the object (the same as `cat-file -s`
 	reports).
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;
 	const char *rest;
 	struct object_id delta_base_oid;
@@ -303,6 +304,7 @@  struct expand_data {
 	 */
 	unsigned skip_object_info : 1;
 };
+#define EXPAND_DATA_INIT  { .mode = S_IFINVALID }
 
 static int is_atom(const char *atom, const char *s, int slen)
 {
@@ -342,6 +344,9 @@  static void 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
 		die("unknown format element: %.*s", len, atom);
 }
@@ -562,6 +567,7 @@  static void batch_one_object(const char *obj_name,
 		return;
 	}
 
+	data->mode = ctx.mode;
 	batch_object_write(obj_name, scratch, opt, data, NULL, 0);
 }
 
@@ -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));
 	data.mark_query = 1;
 	expand_format(&output,
 		      opt->format ? opt->format : DEFAULT_FORMAT,
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"
@@ -211,6 +212,12 @@  $content"
 	test_cmp expect actual
     '
 
+    test_expect_success '--batch-check with %(objectmode)' '
+	echo "$mode $oid" >expect &&
+	echo $object_name | git cat-file --batch-check="%(objectmode) %(objectname)" >actual &&
+	test_cmp expect actual
+    '
+
     test -z "$content" ||
     test_expect_success "--batch without type ($type)" '
 	{
@@ -241,7 +248,7 @@  test_expect_success "setup" '
 	git update-index --add hello
 '
 
-run_tests 'blob' $hello_sha1 $hello_size "$hello_content" "$hello_content"
+run_tests 'blob' $hello_sha1 "" $hello_size "$hello_content" "$hello_content"
 
 test_expect_success '--batch-command --buffer with flush for blob info' '
 	echo "$hello_sha1 blob $hello_size" >expect &&
@@ -271,8 +278,8 @@  tree_sha1=$(git write-tree)
 tree_size=$(($(test_oid rawsz) + 13))
 tree_pretty_content="100644 blob $hello_sha1	hello${LF}"
 
-run_tests 'tree' $tree_sha1 $tree_size "" "$tree_pretty_content"
-run_tests 'blob' "$tree_sha1:hello" $hello_size "" "$hello_content"
+run_tests 'tree' $tree_sha1 "" $tree_size "" "$tree_pretty_content"
+run_tests 'blob' "$tree_sha1:hello" "100644" $hello_size "" "$hello_content"
 
 commit_message="Initial commit"
 commit_sha1=$(echo_without_newline "$commit_message" | git commit-tree $tree_sha1)
@@ -283,7 +290,7 @@  committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
 
 $commit_message"
 
-run_tests 'commit' $commit_sha1 $commit_size "$commit_content" "$commit_content"
+run_tests 'commit' $commit_sha1 "" $commit_size "$commit_content" "$commit_content"
 
 tag_header_without_timestamp="object $hello_sha1
 type blob
@@ -297,7 +304,7 @@  $tag_description"
 tag_sha1=$(echo_without_newline "$tag_content" | git hash-object -t tag --stdin -w)
 tag_size=$(strlen "$tag_content")
 
-run_tests 'tag' $tag_sha1 $tag_size "$tag_content" "$tag_content"
+run_tests 'tag' $tag_sha1 "" $tag_size "$tag_content" "$tag_content"
 
 test_expect_success "Reach a blob from a tag pointing to it" '
 	echo_without_newline "$hello_content" >expect &&