On Fri, Dec 3, 2021 at 9:54 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);
>
> Why re-use this & leak memory? An existing strbuf use in this function
> doesn't leak in the same way. Just release it as in the below patch on
> top (the ret v.s. err variable naming is a bit confused, maybe could do
> with a prep cleanup step.).
>
> > + strbuf_addstr(&filename, the_repository->objects->odb->path);
> > + strbuf_addch(&filename, '/');
>
> And once we do that this could just become:
>
> strbuf_addf($filename, "%s/", ...)
>
> Is there's existing uses of this pattern, so mayb e not worth it, but it
> allows you to remove the braces on the if/else.
>
> diff --git a/object-file.c b/object-file.c
> index 8bd89e7b7ba..2b52f3fc1cc 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1880,7 +1880,7 @@ int write_loose_object(const struct object_id *oid, char *hdr,
> int hdrlen, struct input_stream *in_stream,
> time_t mtime, unsigned flags)
> {
> - int fd, ret;
> + int fd, ret, err = 0;
> unsigned char compressed[4096];
> git_zstream stream;
> git_hash_ctx c;
> @@ -1892,7 +1892,6 @@ int write_loose_object(const struct object_id *oid, char *hdr,
>
> 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 {
> @@ -1902,11 +1901,12 @@ int write_loose_object(const struct object_id *oid, char *hdr,
> fd = create_tmpfile(&tmp_file, filename.buf);
> if (fd < 0) {
> if (flags & HASH_SILENT)
> - return -1;
> + err = -1;
> else if (errno == EACCES)
> - return error(_("insufficient permission for adding an object to repository database %s"), get_object_directory());
> + err = error(_("insufficient permission for adding an object to repository database %s"), get_object_directory());
> else
> - return error_errno(_("unable to create temporary file"));
> + err = error_errno(_("unable to create temporary file"));
> + goto cleanup;
> }
>
> /* Set it up */
> @@ -1968,10 +1968,13 @@ int write_loose_object(const struct object_id *oid, char *hdr,
> struct strbuf dir = STRBUF_INIT;
> strbuf_add(&dir, filename.buf, dirlen - 1);
> if (mkdir(dir.buf, 0777) && errno != EEXIST)
> - return -1;
> - if (adjust_shared_perm(dir.buf))
> - return -1;
> - strbuf_release(&dir);
> + err = -1;
> + else if (adjust_shared_perm(dir.buf))
> + err = -1;
> + else
> + strbuf_release(&dir);
> + if (err < 0)
> + goto cleanup;
> }
> }
>
> @@ -1984,7 +1987,10 @@ int write_loose_object(const struct object_id *oid, char *hdr,
> warning_errno(_("failed utime() on %s"), tmp_file.buf);
> }
>
> - return finalize_object_file(tmp_file.buf, filename.buf);
> + err = finalize_object_file(tmp_file.buf, filename.buf);
> +cleanup:
> + strbuf_release(&filename);
> + return err;
> }
>
> static int freshen_loose_object(const struct object_id *oid)
Yes, this will be much better. Will apply.
