Re: [PATCH v4 2/5] object-file.c: handle undetermined oid in write_loose_object()

On Fri, Dec 03 2021, Han Xin wrote:

quoted

From: Han Xin <redacted>

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 [off-list ref]
Signed-off-by: Han Xin <redacted>
---
 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(&parano_oid, &c);
-     if (!oideq(oid, &parano_oid))
+     if (!is_null_oid(oid) && !oideq(oid, &parano_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, &parano_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, &parano_oid);
-               loose_object_path(the_repository, &filename, oid);
+               oidcpy(&oid2, &parano_oid);
+               loose_object_path(the_repository, &filename, &oid2);

                /* We finally know the object path, and create the missing dir. */
                dirlen = directory_size(filename.buf);