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

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Jan 10, 2022, 19:01 UTC
Message-ID: <CAFQ2z_MEN2j-sHmGK4SA6N8P9bHvjO2=+G0XxXp_9vzLRxwKuw@mail.gmail.com>
In-Reply-To: <xmqqsfui4nkr.fsf@gitster.g>

On Fri, Dec 24, 2021 at 5:46 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

> >       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?

Show 17 quoted lines

> 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