Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cat-file: add %(objectmode) avoid verifying submodules' OIDs #1689

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

cat-file: add %(objectmode) avoid verifying submodules' OIDs #1689

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
765d724
t1006: update 'run_tests' to test generic object specifiers
vdye Jan 3, 2024
b836bc6
cat-file: add %(objectmode) atom
vdye Jan 3, 2024
951f733
cat-file: avoid verifying submodules' OIDs
dscho Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Documentation/git-cat-file.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,11 @@ newline. The available atoms are:
`objecttype`::
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <[email protected]> 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'.

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).
Expand Down
9 changes: 7 additions & 2 deletions builtin/cat-file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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;

Expand All @@ -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,
Expand Down
69 changes: 39 additions & 30 deletions t/t1006-cat-file.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,65 +112,67 @@ strlen () {

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Victoria Dye via GitGitGadget" <[email protected]> writes:

> From: Victoria Dye <[email protected]>
>
> Update the 'run_tests' test wrapper so that the first argument may refer to
> any specifier that uniquely identifies an object (e.g. a ref name,
> '<OID>:<path>', '<OID>^{<type>}', etc.), rather than only a full object ID.
> Also, add a test that uses a non-OID identifier, ensuring appropriate
> parsing in 'cat-file'.
>
> Signed-off-by: Victoria Dye <[email protected]>
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
>  t/t1006-cat-file.sh | 46 +++++++++++++++++++++++----------------------
>  1 file changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> index e0c6482797e..ac1f754ee32 100755
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -112,65 +112,66 @@ strlen () {
>  
>  run_tests () {
>      type=$1
> -    sha1=$2
> +    object_name=$2
> +    oid=$(git rev-parse --verify $object_name)
>      size=$3
>      content=$4
>      pretty_content=$5
>  
> -    batch_output="$sha1 $type $size
> +    batch_output="$oid $type $size
>  $content"

As "object_name" is now allowed to be any name in the 'extended
SHA-1' syntax (cf. Documentation/revisions.txt), you should be a bit
more careful in quoting.

	oid=$(git rev-parse --verify "$object_name")

>      test_expect_success "$type exists" '
> -	git cat-file -e $sha1
> +	git cat-file -e $object_name
>      '

Likewise.  You may not currently use a path with SP in it to name a
tree object, e.g., "HEAD:Read Me.txt", but protecting against such a
pathname is a cheap investment for futureproofing.

Looking good otherwise.  Thanks.

run_tests () {
type=$1
sha1=$2
size=$3
content=$4
pretty_content=$5

batch_output="$sha1 $type $size
object_name=$2
oid=$(git rev-parse --verify $object_name)
mode=$3
size=$4
content=$5
pretty_content=$6

batch_output="$oid $type $size
$content"

test_expect_success "$type exists" '
git cat-file -e $sha1
git cat-file -e $object_name
'

test_expect_success "Type of $type is correct" '
echo $type >expect &&
git cat-file -t $sha1 >actual &&
git cat-file -t $object_name >actual &&
test_cmp expect actual
'

test_expect_success "Size of $type is correct" '
echo $size >expect &&
git cat-file -s $sha1 >actual &&
git cat-file -s $object_name >actual &&
test_cmp expect actual
'

test_expect_success "Type of $type is correct using --allow-unknown-type" '
echo $type >expect &&
git cat-file -t --allow-unknown-type $sha1 >actual &&
git cat-file -t --allow-unknown-type $object_name >actual &&
test_cmp expect actual
'

test_expect_success "Size of $type is correct using --allow-unknown-type" '
echo $size >expect &&
git cat-file -s --allow-unknown-type $sha1 >actual &&
git cat-file -s --allow-unknown-type $object_name >actual &&
test_cmp expect actual
'

test -z "$content" ||
test_expect_success "Content of $type is correct" '
echo_without_newline "$content" >expect &&
git cat-file $type $sha1 >actual &&
git cat-file $type $object_name >actual &&
test_cmp expect actual
'

test_expect_success "Pretty content of $type is correct" '
echo_without_newline "$pretty_content" >expect &&
git cat-file -p $sha1 >actual &&
git cat-file -p $object_name >actual &&
test_cmp expect actual
'

test -z "$content" ||
test_expect_success "--batch output of $type is correct" '
echo "$batch_output" >expect &&
echo $sha1 | git cat-file --batch >actual &&
echo $object_name | git cat-file --batch >actual &&
test_cmp expect actual
'

test_expect_success "--batch-check output of $type is correct" '
echo "$sha1 $type $size" >expect &&
echo_without_newline $sha1 | git cat-file --batch-check >actual &&
echo "$oid $type $size" >expect &&
echo_without_newline $object_name | git cat-file --batch-check >actual &&
test_cmp expect actual
'

Expand All @@ -179,44 +181,50 @@ $content"
test -z "$content" ||
test_expect_success "--batch-command $opt output of $type content is correct" '
echo "$batch_output" >expect &&
test_write_lines "contents $sha1" | git cat-file --batch-command $opt >actual &&
test_write_lines "contents $object_name" | git cat-file --batch-command $opt >actual &&
test_cmp expect actual
'

test_expect_success "--batch-command $opt output of $type info is correct" '
echo "$sha1 $type $size" >expect &&
test_write_lines "info $sha1" |
echo "$oid $type $size" >expect &&
test_write_lines "info $object_name" |
git cat-file --batch-command $opt >actual &&
test_cmp expect actual
'
done

test_expect_success "custom --batch-check format" '
echo "$type $sha1" >expect &&
echo $sha1 | git cat-file --batch-check="%(objecttype) %(objectname)" >actual &&
echo "$type $oid" >expect &&
echo $object_name | git cat-file --batch-check="%(objecttype) %(objectname)" >actual &&
test_cmp expect actual
'

test_expect_success "custom --batch-command format" '
echo "$type $sha1" >expect &&
echo "info $sha1" | git cat-file --batch-command="%(objecttype) %(objectname)" >actual &&
echo "$type $oid" >expect &&
echo "info $object_name" | git cat-file --batch-command="%(objecttype) %(objectname)" >actual &&
test_cmp expect actual
'

test_expect_success '--batch-check with %(rest)' '
echo "$type this is some extra content" >expect &&
echo "$sha1 this is some extra content" |
echo "$object_name this is some extra content" |
git cat-file --batch-check="%(objecttype) %(rest)" >actual &&
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)" '
{
echo "$size" &&
echo "$content"
} >expect &&
echo $sha1 | git cat-file --batch="%(objectsize)" >actual &&
echo $object_name | git cat-file --batch="%(objectsize)" >actual &&
test_cmp expect actual
'

Expand All @@ -226,7 +234,7 @@ $content"
echo "$type" &&
echo "$content"
} >expect &&
echo $sha1 | git cat-file --batch="%(objecttype)" >actual &&
echo $object_name | git cat-file --batch="%(objecttype)" >actual &&
test_cmp expect actual
'
}
Expand All @@ -240,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 &&
Expand Down Expand Up @@ -270,7 +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 '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)
Expand All @@ -281,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
Expand All @@ -295,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 &&
Expand Down