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

From: Justin Tobler <jltobler@gmail.com>
Date: Jan 25, 2024, 16:02 UTC
Message-ID: <CAGAWz+7hQGMbnc8c9iCzyWQgS=wkaEXXbC7-Biqw2i7oc6rneQ@mail.gmail.com>
In-Reply-To: <b2fb6f5ad0c558527341bd8040544d6b0ae5d8a2.1706100744.git.ps@pks.im>

On Wed, Jan 24, 2024 at 7:21 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 17 quoted lines

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

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

-Justin