Re: [PATCH v2] reftable/stack: adjust permissions of compacted tables

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 26, 2024, 10:05 UTC
Message-ID: <ZbODzwjpMjdYpOh3@tanuki>
In-Reply-To: <CAGAWz+7hQGMbnc8c9iCzyWQgS=wkaEXXbC7-Biqw2i7oc6rneQ@mail.gmail.com>

On Thu, Jan 25, 2024 at 10:02:15AM -0600, Justin Tobler wrote:

Show 24 quoted lines

> On Wed, Jan 24, 2024 at 7:21 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > diff --git a/reftable/stack_test.c b/reftable/stack_test.c
> > index 289e902146..2e7d1768b7 100644
> > --- a/reftable/stack_test.c
> > +++ b/reftable/stack_test.c
> > @@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)
> >         int err = 0;
> >         struct reftable_write_options cfg = {
> >                 .exact_log_message = 1,
> > +               .default_permissions = 0660,
> >         };
> >         struct reftable_stack *st = NULL;
> >         char *dir = get_tmp_dir(__LINE__);
> > -
> >         struct reftable_ref_record refs[2] = { { NULL } };
> >         struct reftable_log_record logs[2] = { { NULL } };
> > +       struct strbuf scratch = STRBUF_INIT;
> 
> The variable name `scratch` seems rather vague to me as opposed to something
> like `path`. After a quick search though, `scratch` appears to be a fairly
> common name used in similar scenarios. So probably not a big deal, but
> something
> I thought I'd mention.

Yeah. I basically copied the below checks from another test where we already had the permission checks, and also adopted the name of the `scratch` variable. I agree though that `path` would be a better name, so let me change it.

Show 32 quoted lines

> > +       struct stat stat_result;
> >         int N = ARRAY_SIZE(refs);
> >
> > -
> >         err = reftable_new_stack(&st, dir, cfg);
> >         EXPECT_ERR(err);
> >         st->disable_auto_compact = 1;
> > @@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)
> >                 reftable_log_record_release(&dest);
> >         }
> >
> > +#ifndef GIT_WINDOWS_NATIVE
> > +       strbuf_addstr(&scratch, dir);
> > +       strbuf_addstr(&scratch, "/tables.list");
> > +       err = stat(scratch.buf, &stat_result);
> > +       EXPECT(!err);
> > +       EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
> > +
> > +       strbuf_reset(&scratch);
> > +       strbuf_addstr(&scratch, dir);
> > +       strbuf_addstr(&scratch, "/");
> > +       /* do not try at home; not an external API for reftable. */
> > +       strbuf_addstr(&scratch, st->readers[0]->name);
> > +       err = stat(scratch.buf, &stat_result);
> > +       EXPECT(!err);
> > +       EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
> > +#else
> > +       (void) stat_result;
> > +#endif
> 
> Why do we ignore Windows here? And would it warrant explaining in the commit
> message?

Because Windows has a different acccess control model for files and doesn't natively use POSIX permissions. I'm not a 100% sure whether we do emulate the permission bits or not, but I cannot test on Windows and the other test where this was ripped out of also makes the code conditional.

Will explain in the commit message.

Thanks!

Patrick