On Wed, Dec 22, 2021 at 8:40 AM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
>
>
> On Tue, Dec 21 2021, Han Xin wrote:
>
> > From: Han Xin <hanxin.hx@xxxxxxxxxxxxxxx>
> > [...]
> > @@ -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.
>
> > + 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...)
