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

[Han Xin <chiyutianyi@xxxxxxxxx>]

On Fri, Dec 3, 2021 at 9:27 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
>
>
> On Fri, Dec 03 2021, Han Xin wrote:
>
> > From: Han Xin <hanxin.hx@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Han Xin <hanxin.hx@xxxxxxxxxxxxxxx>
> > ---
> >  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);

Maybe I should change the promise that "oid" is constant in
"write_loose_object()".

The original write_object_file_flags() defines a variable "oid", and
completes the calculation of the "oid" in
"write_object_file_prepare()" which will be passed to
"write_loose_object()".

If a null oid is maintained after calling "write_loose_object()",
"--strict" will become meaningless, although it does not break existing
test cases.