Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
> ---
> builtin/cat-file.c | 22 ++++++++++++++++++++++
> t/t1050-large.sh | 2 +-
> 2 files changed, 23 insertions(+), 1 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 8ed501f..3f3b558 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -82,6 +82,24 @@ static void pprint_tag(const unsigned char *sha1, const char *buf, unsigned long
> write_or_die(1, cp, endp - cp);
> }
>
> +static int write_blob(const unsigned char *sha1)
> +{
> + unsigned char new_sha1[20];
> +
> + if (sha1_object_info(sha1, NULL) == OBJ_TAG) {
This smells bad. Why in the world could an API be sane if lets a caller
call "write_blob()" with something that can be a tag?
Both of your callsites call this function when (type == OBJ_BLOB), but the
"case 0:" arm in the large switch in cat_one_file() only checks "expected
type" which may not match the real type at all, so it is wrong to switch
on that in the first place. In addition, that call site alone needs to
deref tag to the requested/expected type.
This block does not belong to this function, but to only one of its
callers among two.
> + enum object_type type;
> + unsigned long size;
> + char *buffer = read_sha1_file(sha1, &type, &size);
> + if (memcmp(buffer, "object ", 7) ||
> + get_sha1_hex(buffer + 7, new_sha1))
> + die("%s not a valid tag", sha1_to_hex(sha1));
> + sha1 = new_sha1;
> + free(buffer);
> + }
> +
> + return streaming_write_sha1(1, 0, sha1, OBJ_BLOB, NULL);
I do not think your previous refactoring added a fall-back codepath to the
function you are calling here. In the original context, the caller of
streaming_write_entry() made sure that the blob is suitable for streaming
write by getting an istream, and called the function only when that is the
case. Blobs unsuitable for streaming (e.g. an deltified object in a pack)
were handled by the caller that decided not to call
streaming_write_entry() with the conventional "read to core and then write
it out" codepath.
And I do not think your updated caller in cat_one_file() is equipped to do
so at all.
So it looks to me that this patch totally breaks the cat-file. What am I
missing?
> +}
> +
> static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
> {
> unsigned char sha1[20];
> @@ -127,6 +145,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
> return cmd_ls_tree(2, ls_args, NULL);
> }
>
> + if (type == OBJ_BLOB)
> + return write_blob(sha1);
> buf = read_sha1_file(sha1, &type, &size);
> if (!buf)
> die("Cannot read object %s", obj_name);
> @@ -149,6 +169,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
> break;
>
> case 0:
> + if (type_from_string(exp_type) == OBJ_BLOB)
> + return write_blob(sha1);
> buf = read_object_with_reference(sha1, exp_type, &size, NULL);
> break;
>
> diff --git a/t/t1050-large.sh b/t/t1050-large.sh
> index f245e59..39a3e77 100755
> --- a/t/t1050-large.sh
> +++ b/t/t1050-large.sh
> @@ -114,7 +114,7 @@ test_expect_success 'hash-object' '
> git hash-object large1
> '
>
> -test_expect_failure 'cat-file a large file' '
> +test_expect_success 'cat-file a large file' '
> git cat-file blob :large1 >/dev/null
> '
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html