Re: [PATCH v4 2/5] object-file.c: handle undetermined oid in write_loose_object()
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Dec 3, 2021, 13:21 UTC
- Message-ID
- <211203.86v905stru.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <20211203093530.93589-3-chiyutianyi@gmail.com>
On Fri, Dec 03 2021, Han Xin wrote:
Show 55 quoted lines
> From: Han Xin <hanxin.hx@alibaba-inc.com>
>
> When streaming a large blob object to "write_loose_object()", we have no
> chance to run "write_object_file_prepare()" to calculate the oid in
> advance. So we need to handle undetermined oid in function
> "write_loose_object()".
>
> In the original implementation, we know the oid and we can write the
> temporary file in the same directory as the final object, but for an
> object with an undetermined oid, we don't know the exact directory for
> the object, so we have to save the temporary file in ".git/objects/"
> directory instead.
>
> Helped-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> Signed-off-by: Han Xin <hanxin.hx@alibaba-inc.com>
> ---
> object-file.c | 30 ++++++++++++++++++++++++++++--
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/object-file.c b/object-file.c
> index 82656f7428..1c41587bfb 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1892,7 +1892,14 @@ static int write_loose_object(const struct object_id *oid, char *hdr,
> const void *buf;
> unsigned long len;
>
> - loose_object_path(the_repository, &filename, oid);
> + if (is_null_oid(oid)) {
> + /* When oid is not determined, save tmp file to odb path. */
> + strbuf_reset(&filename);
> + strbuf_addstr(&filename, the_repository->objects->odb->path);
> + strbuf_addch(&filename, '/');
> + } else {
> + loose_object_path(the_repository, &filename, oid);
> + }
>
> fd = create_tmpfile(&tmp_file, filename.buf);
> if (fd < 0) {
> @@ -1939,12 +1946,31 @@ static int write_loose_object(const struct object_id *oid, char *hdr,
> die(_("deflateEnd on object %s failed (%d)"), oid_to_hex(oid),
> ret);
> the_hash_algo->final_oid_fn(¶no_oid, &c);
> - if (!oideq(oid, ¶no_oid))
> + if (!is_null_oid(oid) && !oideq(oid, ¶no_oid))
> die(_("confused by unstable object source data for %s"),
> oid_to_hex(oid));
>
> close_loose_object(fd);
>
> + if (is_null_oid(oid)) {
> + int dirlen;
> +
> + oidcpy((struct object_id *)oid, ¶no_oid);
> + loose_object_path(the_repository, &filename, oid);Why are we breaking the promise that "oid" is constant here? I tested locally with the below on top, and it seems to work (at least no tests broke). Isn't it preferrable to the cast & the caller having its "oid" changed?
diff --git a/object-file.c b/object-file.c index 71d510614b9..d014e6942ea 100644 --- a/object-file.c +++ b/object-file.c @@ -1958,10 +1958,11 @@ int write_loose_object(const struct object_id *oid, char *hdr, close_loose_object(fd); if (is_null_oid(oid)) { + struct object_id oid2; int dirlen; - oidcpy((struct object_id *)oid, ¶no_oid); - loose_object_path(the_repository, &filename, oid); + oidcpy(&oid2, ¶no_oid); + loose_object_path(the_repository, &filename, &oid2); /* We finally know the object path, and create the missing dir. */ dirlen = directory_size(filename.buf);