On Thu, Jan 25, 2024 at 10:02:15AM -0600, Justin Tobler wrote:
> On Wed, Jan 24, 2024 at 7:21 AM Patrick Steinhardt <ps@xxxxxx> 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.
> > + 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
Attachment:
signature.asc
Description: PGP signature
