Re: [PATCH v7 3/5] object-file.c: refactor write_loose_object() to reuse in stream version

From: Jiang Xin <worldhello.net@gmail.com>
Date: Dec 22, 2021, 12:02 UTC
Message-ID: <CANYiYbFN4fn6hG8_=kLDENnvUtGCsXnksgTeBNG=VYzyaM9yrQ@mail.gmail.com>
In-Reply-To: <211221.867dbyrqe9.gmgdl@evledraar.gmail.com>

On Wed, Dec 22, 2021 at 8:40 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 28 quoted lines

>
>
> On Tue, Dec 21 2021, Han Xin wrote:
>
> > From: Han Xin <hanxin.hx@alibaba-inc.com>
> > [...]
> > @@ -1854,17 +1876,48 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename)
> >               strbuf_reset(tmp);
> >               strbuf_add(tmp, filename, dirlen - 1);
> >               if (mkdir(tmp->buf, 0777) && errno != EEXIST)
> > -                     return -1;
> > +                     break;
> >               if (adjust_shared_perm(tmp->buf))
> > -                     return -1;
> > +                     break;
> >
> >               /* Try again */
> >               strbuf_addstr(tmp, "/tmp_obj_XXXXXX");
> >               fd = git_mkstemp_mode(tmp->buf, 0444);
> > +     } while (0);
> > +
> > +     if (fd < 0 && !(flags & HASH_SILENT)) {
> > +             if (errno == EACCES)
> > +                     return error(_("insufficient permission for adding an "
> > +                                    "object to repository database %s"),
> > +                                  get_object_directory());
>
> This should be an error_errno() instead, ...

We already know the errno (EACCESS) and output a decent error message, so using error() is OK. BTW, it's just a refactor by copy & paste.

Show 12 quoted lines

>
> > +             else
> > +                     return error_errno(_("unable to create temporary file"));
>
> ...and we can just fold this whole if/else into one condition with a
> briefer message, e.g.:
>
>     error_errno(_("unable to add object to '%s'"), get_object_directory());
>
> Or whatever, unless there's another bug here where you inverted these
> conditions, and the "else" really should not use "error_errno" but
> "error".... (I don't know...)