On Fri, Dec 24, 2021 at 5:46 AM Junio C Hamano <gitster@xxxxxxxxx> 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
