Re: [PATCH 3/3] reftable: support preset file mode for writing

[Han-Wen Nienhuys <hanwen@google.com>]

On Fri, Dec 24, 2021 at 5:46 AM Junio C Hamano <gitster@pobox.com> wrote:
> >       if (add->lock_file_fd < 0) {
> >               if (errno == EEXIST) {
> >                       err = REFTABLE_LOCK_ERROR;
> > @@ -478,6 +478,13 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
> >               }
> >               goto done;
> >       }
> > +     if (st->config.default_permissions) {
> > +             if (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) {
> > +                     err = REFTABLE_IO_ERROR;
> > +                     goto done;
>
> This part does not exactly make sense, though.

why?

> If this were a library code meant to link with ONLY with Git, I
> would have recommended to make a adjust_shared_perm() call from
> here, without having to have "unsigned int default_permissions" to
> reftable_write_options structure.
>
> I wonder if it is a better design to make the new member in the
> structure a pointer to a generic and opaque helper function that is
> called from here, i.e.
>
>         if (st->config.adjust_perm &&
>             st->config.adjust_perm(add->lock_file_name.buf) < 0) {
>                 err = REFTABLE_IO_ERROR;
>                 goto done;
>         }
>
> so that when linking with and calling from git, we can just stuff
> the pointer to adjust_shared_perm function to the member?

I read over the adjust_shared_perm function for a bit, but I'm puzzled
why its complexity is necessary. It seems to do something with X-bits,
maybe for directories, but that doesn't apply here as we're only
handling files?
We also only write new files, so we never have to look at the existing
mode of a file.

With the current approach, the option is very clear about what it
does, and the unittest is straightforward: set the option, do a write,
and check that the files have the specified mode.

--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado