{"thread":{"id":"60787","subject":"[PATCH 0/2] reftable: adjust permissions of compacted tables","startedAt":"2024-01-24T12:31:36Z","lastAt":"2024-01-30T16:56:28Z","messageCount":9,"participants":["Patrick Steinhardt","Karthik Nayak","Justin Tobler"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"487324","messageId":"cover.1706099090.git.ps@pks.im","threadId":"60787","inReplyTo":null,"subject":"[PATCH 0/2] reftable: adjust permissions of compacted tables","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-24T12:31:30Z","receivedAt":"2024-01-24T12:31:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series fixes a bug in the reftable stack implementation\nwhere the \"default_permissions\" write option is not honored for newly\nwritten compacted tables. The effect is that the \"core.sharedRepository\"\nconfig is ignored when performing compaction.\n\nThe series runs into two minor conflicts with jc/reftable-core-fsync\nthat can be fixed like follows. As this series has been merged to next\nalready I'd also be happy to rebase these patches on top of it. Please\nlet me know your preference.\n\nPatrick\n\ndiff --cc reftable/stack.c\nindex 27cc586460,ab295341cc..0000000000\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@@ -633,13 -643,13 +643,13 @@@ int reftable_addition_add(struct reftab\n                err = REFTABLE_IO_ERROR;\n                goto done;\n        }\n -      if (add->stack->config.default_permissions) {\n -              if (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) {\n -                      err = REFTABLE_IO_ERROR;\n -                      goto done;\n -              }\n +      if (add->stack->config.default_permissions &&\n +          fchmod(tab_fd, add->stack->config.default_permissions) < 0) {\n +              err = REFTABLE_IO_ERROR;\n +              goto done;\n        }\n +\n-       wr = reftable_new_writer(reftable_fd_write, &tab_fd,\n+       wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd,\n                                 &add->stack->config);\n        err = write_table(wr, arg);\n        if (err < 0)\n@@@ -731,13 -741,7 +741,13 @@@ static int stack_compact_locked(struct\n        strbuf_addstr(temp_tab, \".temp.XXXXXX\");\n\n        tab_fd = mkstemp(temp_tab->buf);\n +      if (st->config.default_permissions &&\n +          fchmod(tab_fd, st->config.default_permissions) < 0) {\n +              err = REFTABLE_IO_ERROR;\n +              goto done;\n +      }\n +\n-       wr = reftable_new_writer(reftable_fd_write, &tab_fd, &st->config);\n+       wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd, &st->config);\n\n        err = stack_write_compact(st, wr, first, last, config);\n        if (err < 0)\n\nPatrick Steinhardt (2):\n  reftable/stack: use fchmod(3P) to set permissions\n  reftable/stack: adjust permissions of compacted tables\n\n reftable/stack.c      | 35 ++++++++++++++++++++---------------\n reftable/stack_test.c | 25 +++++++++++++++++++++++--\n 2 files changed, 43 insertions(+), 17 deletions(-)\n\n-- \n2.43.GIT\n\n"},{"id":"487325","messageId":"4b8d76789b41c6540878810ee2312c49b199026e.1706099090.git.ps@pks.im","threadId":"60787","inReplyTo":"cover.1706099090.git.ps@pks.im","subject":"[PATCH 1/2] reftable/stack: use fchmod(3P) to set permissions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-24T12:31:34Z","receivedAt":"2024-01-24T12:31:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We use chmod(3P) to modify permissions of \"tables.list\" locks as well as\ntemporary new tables we're writing. In all of these cases we do have a\nfile descriptor readily available though. So instead of using chmod(3P)\nwe can use fchmod(3P), which should both be more efficient while also\navoiding a potential race where we change permissions of the wrong file\nin case it was swapped out after we have created it.\n\nRefactor the code to do so.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c | 29 ++++++++++++++---------------\n 1 file changed, 14 insertions(+), 15 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 7ffeb3ee10..c6e4dc4b2b 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -467,11 +467,11 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \t\t}\n \t\tgoto done;\n \t}\n-\tif (st->config.default_permissions) {\n-\t\tif (chmod(add->lock_file->filename.buf, st->config.default_permissions) < 0) {\n-\t\t\terr = REFTABLE_IO_ERROR;\n-\t\t\tgoto done;\n-\t\t}\n+\tif (st->config.default_permissions &&\n+\t    fchmod(get_tempfile_fd(add->lock_file),\n+\t\t   st->config.default_permissions) < 0) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n \t}\n \n \terr = stack_uptodate(st);\n@@ -633,12 +633,12 @@ int reftable_addition_add(struct reftable_addition *add,\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n-\tif (add->stack->config.default_permissions) {\n-\t\tif (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) {\n-\t\t\terr = REFTABLE_IO_ERROR;\n-\t\t\tgoto done;\n-\t\t}\n+\tif (add->stack->config.default_permissions &&\n+\t    fchmod(tab_fd, add->stack->config.default_permissions) < 0) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n \t}\n+\n \twr = reftable_new_writer(reftable_fd_write, &tab_fd,\n \t\t\t\t &add->stack->config);\n \terr = write_table(wr, arg);\n@@ -967,11 +967,10 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,\n \t\tgoto done;\n \t}\n \thave_lock = 1;\n-\tif (st->config.default_permissions) {\n-\t\tif (chmod(lock_file_name.buf, st->config.default_permissions) < 0) {\n-\t\t\terr = REFTABLE_IO_ERROR;\n-\t\t\tgoto done;\n-\t\t}\n+\tif (st->config.default_permissions &&\n+\t    fchmod(lock_file_fd, st->config.default_permissions) < 0) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n \t}\n \n \tformat_name(&new_table_name, st->readers[first]->min_update_index,\n-- \n2.43.GIT\n\n"},{"id":"487326","messageId":"cd5172193660a334ae6ec022dd0b54457d06f25c.1706099090.git.ps@pks.im","threadId":"60787","inReplyTo":"cover.1706099090.git.ps@pks.im","subject":"[PATCH 2/2] reftable/stack: adjust permissions of compacted tables","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-24T12:31:41Z","receivedAt":"2024-01-24T12:31:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When creating a new compacted table from a range of preexisting ones we\ndon't set the default permissions on the resulting table when specified\nby the user. This has the effect that the \"core.sharedRepository\" config\nwill not be honored correctly.\n\nFix this bug and add a test to catch this issue.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c      |  6 ++++++\n reftable/stack_test.c | 25 +++++++++++++++++++++++--\n 2 files changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex c6e4dc4b2b..27cc586460 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -731,6 +731,12 @@ static int stack_compact_locked(struct reftable_stack *st, int first, int last,\n \tstrbuf_addstr(temp_tab, \".temp.XXXXXX\");\n \n \ttab_fd = mkstemp(temp_tab->buf);\n+\tif (st->config.default_permissions &&\n+\t    fchmod(tab_fd, st->config.default_permissions) < 0) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n+\t}\n+\n \twr = reftable_new_writer(reftable_fd_write, &tab_fd, &st->config);\n \n \terr = stack_write_compact(st, wr, first, last, config);\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 289e902146..2e7d1768b7 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)\n \tint err = 0;\n \tstruct reftable_write_options cfg = {\n \t\t.exact_log_message = 1,\n+\t\t.default_permissions = 0660,\n \t};\n \tstruct reftable_stack *st = NULL;\n \tchar *dir = get_tmp_dir(__LINE__);\n-\n \tstruct reftable_ref_record refs[2] = { { NULL } };\n \tstruct reftable_log_record logs[2] = { { NULL } };\n+\tstruct strbuf scratch = STRBUF_INIT;\n+\tstruct stat stat_result;\n \tint N = ARRAY_SIZE(refs);\n \n-\n \terr = reftable_new_stack(&st, dir, cfg);\n \tEXPECT_ERR(err);\n \tst->disable_auto_compact = 1;\n@@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)\n \t\treftable_log_record_release(&dest);\n \t}\n \n+#ifndef GIT_WINDOWS_NATIVE\n+\tstrbuf_addstr(&scratch, dir);\n+\tstrbuf_addstr(&scratch, \"/tables.list\");\n+\terr = stat(scratch.buf, &stat_result);\n+\tEXPECT(!err);\n+\tEXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);\n+\n+\tstrbuf_reset(&scratch);\n+\tstrbuf_addstr(&scratch, dir);\n+\tstrbuf_addstr(&scratch, \"/\");\n+\t/* do not try at home; not an external API for reftable. */\n+\tstrbuf_addstr(&scratch, st->readers[0]->name);\n+\terr = stat(scratch.buf, &stat_result);\n+\tEXPECT(!err);\n+\tEXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);\n+#else\n+\t(void) stat_result;\n+#endif\n+\n \t/* cleanup */\n \treftable_stack_destroy(st);\n \tfor (i = 0; i < N; i++) {\n \t\treftable_ref_record_release(&refs[i]);\n \t\treftable_log_record_release(&logs[i]);\n \t}\n+\tstrbuf_release(&scratch);\n \tclear_dir(dir);\n }\n \n-- \n2.43.GIT\n\n"},{"id":"487327","messageId":"b2fb6f5ad0c558527341bd8040544d6b0ae5d8a2.1706100744.git.ps@pks.im","threadId":"60787","inReplyTo":"cover.1706099090.git.ps@pks.im","subject":"[PATCH v2] reftable/stack: adjust permissions of compacted tables","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-24T12:53:50Z","receivedAt":"2024-01-24T12:53:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When creating a new compacted table from a range of preexisting ones we\ndon't set the default permissions on the resulting table when specified\nby the user. This has the effect that the \"core.sharedRepository\" config\nwill not be honored correctly.\n\nFix this bug and add a test to catch this issue.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n\nUgh, I totally forgot about the fact that fchmod(3P) isn't actually\navailable on Windows. I've thus evicted the first patch and changed the\nsecond patch to use chmod(3P) instead. Too bad.\n\n reftable/stack.c      |  6 ++++++\n reftable/stack_test.c | 25 +++++++++++++++++++++++--\n 2 files changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 7ffeb3ee10..38e784a8ab 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -731,6 +731,12 @@ static int stack_compact_locked(struct reftable_stack *st, int first, int last,\n \tstrbuf_addstr(temp_tab, \".temp.XXXXXX\");\n \n \ttab_fd = mkstemp(temp_tab->buf);\n+\tif (st->config.default_permissions &&\n+\t    chmod(temp_tab->buf, st->config.default_permissions) < 0) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n+\t}\n+\n \twr = reftable_new_writer(reftable_fd_write, &tab_fd, &st->config);\n \n \terr = stack_write_compact(st, wr, first, last, config);\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 289e902146..2e7d1768b7 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)\n \tint err = 0;\n \tstruct reftable_write_options cfg = {\n \t\t.exact_log_message = 1,\n+\t\t.default_permissions = 0660,\n \t};\n \tstruct reftable_stack *st = NULL;\n \tchar *dir = get_tmp_dir(__LINE__);\n-\n \tstruct reftable_ref_record refs[2] = { { NULL } };\n \tstruct reftable_log_record logs[2] = { { NULL } };\n+\tstruct strbuf scratch = STRBUF_INIT;\n+\tstruct stat stat_result;\n \tint N = ARRAY_SIZE(refs);\n \n-\n \terr = reftable_new_stack(&st, dir, cfg);\n \tEXPECT_ERR(err);\n \tst->disable_auto_compact = 1;\n@@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)\n \t\treftable_log_record_release(&dest);\n \t}\n \n+#ifndef GIT_WINDOWS_NATIVE\n+\tstrbuf_addstr(&scratch, dir);\n+\tstrbuf_addstr(&scratch, \"/tables.list\");\n+\terr = stat(scratch.buf, &stat_result);\n+\tEXPECT(!err);\n+\tEXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);\n+\n+\tstrbuf_reset(&scratch);\n+\tstrbuf_addstr(&scratch, dir);\n+\tstrbuf_addstr(&scratch, \"/\");\n+\t/* do not try at home; not an external API for reftable. */\n+\tstrbuf_addstr(&scratch, st->readers[0]->name);\n+\terr = stat(scratch.buf, &stat_result);\n+\tEXPECT(!err);\n+\tEXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);\n+#else\n+\t(void) stat_result;\n+#endif\n+\n \t/* cleanup */\n \treftable_stack_destroy(st);\n \tfor (i = 0; i < N; i++) {\n \t\treftable_ref_record_release(&refs[i]);\n \t\treftable_log_record_release(&logs[i]);\n \t}\n+\tstrbuf_release(&scratch);\n \tclear_dir(dir);\n }\n \n\nbase-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c\n-- \n2.43.GIT\n\n"},{"id":"487361","messageId":"CAOLa=ZSDRr8v0uo2aCtdnbv0F=cFMQymnNLMTzpUt-OCdh5NMA@mail.gmail.com","threadId":"60787","inReplyTo":"b2fb6f5ad0c558527341bd8040544d6b0ae5d8a2.1706100744.git.ps@pks.im","subject":"Re: [PATCH v2] reftable/stack: adjust permissions of compacted tables","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-01-25T13:05:37Z","receivedAt":"2024-01-25T13:05:39Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> When creating a new compacted table from a range of preexisting ones we\n> don't set the default permissions on the resulting table when specified\n> by the user. This has the effect that the \"core.sharedRepository\" config\n> will not be honored correctly.\n>\n> Fix this bug and add a test to catch this issue.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>\n> Ugh, I totally forgot about the fact that fchmod(3P) isn't actually\n> available on Windows. I've thus evicted the first patch and changed the\n> second patch to use chmod(3P) instead. Too bad.\n>\n\nGood catch. This version looks good to me! Nothing to add :)\n"},{"id":"487364","messageId":"CAGAWz+7hQGMbnc8c9iCzyWQgS=wkaEXXbC7-Biqw2i7oc6rneQ@mail.gmail.com","threadId":"60787","inReplyTo":"b2fb6f5ad0c558527341bd8040544d6b0ae5d8a2.1706100744.git.ps@pks.im","subject":"Re: [PATCH v2] reftable/stack: adjust permissions of compacted tables","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-01-25T16:02:15Z","receivedAt":"2024-01-25T16:02:29Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On Wed, Jan 24, 2024 at 7:21 AM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> diff --git a/reftable/stack_test.c b/reftable/stack_test.c\n> index 289e902146..2e7d1768b7 100644\n> --- a/reftable/stack_test.c\n> +++ b/reftable/stack_test.c\n> @@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)\n>         int err = 0;\n>         struct reftable_write_options cfg = {\n>                 .exact_log_message = 1,\n> +               .default_permissions = 0660,\n>         };\n>         struct reftable_stack *st = NULL;\n>         char *dir = get_tmp_dir(__LINE__);\n> -\n>         struct reftable_ref_record refs[2] = { { NULL } };\n>         struct reftable_log_record logs[2] = { { NULL } };\n> +       struct strbuf scratch = STRBUF_INIT;\n\nThe variable name `scratch` seems rather vague to me as opposed to something\nlike `path`. After a quick search though, `scratch` appears to be a fairly\ncommon name used in similar scenarios. So probably not a big deal, but\nsomething\nI thought I'd mention.\n\n> +       struct stat stat_result;\n>         int N = ARRAY_SIZE(refs);\n>\n> -\n>         err = reftable_new_stack(&st, dir, cfg);\n>         EXPECT_ERR(err);\n>         st->disable_auto_compact = 1;\n> @@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)\n>                 reftable_log_record_release(&dest);\n>         }\n>\n> +#ifndef GIT_WINDOWS_NATIVE\n> +       strbuf_addstr(&scratch, dir);\n> +       strbuf_addstr(&scratch, \"/tables.list\");\n> +       err = stat(scratch.buf, &stat_result);\n> +       EXPECT(!err);\n> +       EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);\n> +\n> +       strbuf_reset(&scratch);\n> +       strbuf_addstr(&scratch, dir);\n> +       strbuf_addstr(&scratch, \"/\");\n> +       /* do not try at home; not an external API for reftable. */\n> +       strbuf_addstr(&scratch, st->readers[0]->name);\n> +       err = stat(scratch.buf, &stat_result);\n> +       EXPECT(!err);\n> +       EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);\n> +#else\n> +       (void) stat_result;\n> +#endif\n\nWhy do we ignore Windows here? And would it warrant explaining in the commit\nmessage?\n\n-Justin\n"},{"id":"487396","messageId":"ZbODzwjpMjdYpOh3@tanuki","threadId":"60787","inReplyTo":"CAGAWz+7hQGMbnc8c9iCzyWQgS=wkaEXXbC7-Biqw2i7oc6rneQ@mail.gmail.com","subject":"Re: [PATCH v2] reftable/stack: adjust permissions of compacted tables","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-26T10:05:03Z","receivedAt":"2024-01-26T10:05:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jan 25, 2024 at 10:02:15AM -0600, Justin Tobler wrote:\n> On Wed, Jan 24, 2024 at 7:21 AM Patrick Steinhardt <ps@pks.im> wrote:\n> >\n> > diff --git a/reftable/stack_test.c b/reftable/stack_test.c\n> > index 289e902146..2e7d1768b7 100644\n> > --- a/reftable/stack_test.c\n> > +++ b/reftable/stack_test.c\n> > @@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)\n> >         int err = 0;\n> >         struct reftable_write_options cfg = {\n> >                 .exact_log_message = 1,\n> > +               .default_permissions = 0660,\n> >         };\n> >         struct reftable_stack *st = NULL;\n> >         char *dir = get_tmp_dir(__LINE__);\n> > -\n> >         struct reftable_ref_record refs[2] = { { NULL } };\n> >         struct reftable_log_record logs[2] = { { NULL } };\n> > +       struct strbuf scratch = STRBUF_INIT;\n> \n> The variable name `scratch` seems rather vague to me as opposed to something\n> like `path`. After a quick search though, `scratch` appears to be a fairly\n> common name used in similar scenarios. So probably not a big deal, but\n> something\n> I thought I'd mention.\n\nYeah. I basically copied the below checks from another test where we\nalready had the permission checks, and also adopted the name of the\n`scratch` variable. I agree though that `path` would be a better name,\nso let me change it.\n\n> > +       struct stat stat_result;\n> >         int N = ARRAY_SIZE(refs);\n> >\n> > -\n> >         err = reftable_new_stack(&st, dir, cfg);\n> >         EXPECT_ERR(err);\n> >         st->disable_auto_compact = 1;\n> > @@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)\n> >                 reftable_log_record_release(&dest);\n> >         }\n> >\n> > +#ifndef GIT_WINDOWS_NATIVE\n> > +       strbuf_addstr(&scratch, dir);\n> > +       strbuf_addstr(&scratch, \"/tables.list\");\n> > +       err = stat(scratch.buf, &stat_result);\n> > +       EXPECT(!err);\n> > +       EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);\n> > +\n> > +       strbuf_reset(&scratch);\n> > +       strbuf_addstr(&scratch, dir);\n> > +       strbuf_addstr(&scratch, \"/\");\n> > +       /* do not try at home; not an external API for reftable. */\n> > +       strbuf_addstr(&scratch, st->readers[0]->name);\n> > +       err = stat(scratch.buf, &stat_result);\n> > +       EXPECT(!err);\n> > +       EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);\n> > +#else\n> > +       (void) stat_result;\n> > +#endif\n> \n> Why do we ignore Windows here? And would it warrant explaining in the commit\n> message?\n\nBecause Windows has a different acccess control model for files and\ndoesn't natively use POSIX permissions. I'm not a 100% sure whether we\ndo emulate the permission bits or not, but I cannot test on Windows and\nthe other test where this was ripped out of also makes the code\nconditional.\n\nWill explain in the commit message.\n\nThanks!\n\nPatrick\n"},{"id":"487397","messageId":"a211818108053754aca002726d0206623a347952.1706263589.git.ps@pks.im","threadId":"60787","inReplyTo":"cover.1706099090.git.ps@pks.im","subject":"[PATCH v3] reftable/stack: adjust permissions of compacted tables","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-26T10:09:10Z","receivedAt":"2024-01-26T10:09:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When creating a new compacted table from a range of preexisting ones we\ndon't set the default permissions on the resulting table when specified\nby the user. This has the effect that the \"core.sharedRepository\" config\nwill not be honored correctly.\n\nFix this bug and add a test to catch this issue. Note that we only test\non non-Windows platforms because Windows does not use POSIX permissions\nnatively.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n\nChanges compared to v2:\n\n  - Extended the commit message to say why we don't test on Windows\n    systems.\n\n  - Renamed the `scratch` variable to `path`.\n\nThanks!\n\nPatrick\n\n reftable/stack.c      |  6 ++++++\n reftable/stack_test.c | 25 +++++++++++++++++++++++--\n 2 files changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 7ffeb3ee10..38e784a8ab 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -731,6 +731,12 @@ static int stack_compact_locked(struct reftable_stack *st, int first, int last,\n \tstrbuf_addstr(temp_tab, \".temp.XXXXXX\");\n \n \ttab_fd = mkstemp(temp_tab->buf);\n+\tif (st->config.default_permissions &&\n+\t    chmod(temp_tab->buf, st->config.default_permissions) < 0) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n+\t}\n+\n \twr = reftable_new_writer(reftable_fd_write, &tab_fd, &st->config);\n \n \terr = stack_write_compact(st, wr, first, last, config);\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 289e902146..5089392f7b 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)\n \tint err = 0;\n \tstruct reftable_write_options cfg = {\n \t\t.exact_log_message = 1,\n+\t\t.default_permissions = 0660,\n \t};\n \tstruct reftable_stack *st = NULL;\n \tchar *dir = get_tmp_dir(__LINE__);\n-\n \tstruct reftable_ref_record refs[2] = { { NULL } };\n \tstruct reftable_log_record logs[2] = { { NULL } };\n+\tstruct strbuf path = STRBUF_INIT;\n+\tstruct stat stat_result;\n \tint N = ARRAY_SIZE(refs);\n \n-\n \terr = reftable_new_stack(&st, dir, cfg);\n \tEXPECT_ERR(err);\n \tst->disable_auto_compact = 1;\n@@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)\n \t\treftable_log_record_release(&dest);\n \t}\n \n+#ifndef GIT_WINDOWS_NATIVE\n+\tstrbuf_addstr(&path, dir);\n+\tstrbuf_addstr(&path, \"/tables.list\");\n+\terr = stat(path.buf, &stat_result);\n+\tEXPECT(!err);\n+\tEXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);\n+\n+\tstrbuf_reset(&path);\n+\tstrbuf_addstr(&path, dir);\n+\tstrbuf_addstr(&path, \"/\");\n+\t/* do not try at home; not an external API for reftable. */\n+\tstrbuf_addstr(&path, st->readers[0]->name);\n+\terr = stat(path.buf, &stat_result);\n+\tEXPECT(!err);\n+\tEXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);\n+#else\n+\t(void) stat_result;\n+#endif\n+\n \t/* cleanup */\n \treftable_stack_destroy(st);\n \tfor (i = 0; i < N; i++) {\n \t\treftable_ref_record_release(&refs[i]);\n \t\treftable_log_record_release(&logs[i]);\n \t}\n+\tstrbuf_release(&path);\n \tclear_dir(dir);\n }\n \n\nbase-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c\n-- \n2.43.GIT\n\n"},{"id":"487602","messageId":"CAGAWz+7enErUf9apL9nE13=bWSHL=Sz6PdUQ_DRH7-9NCvQ8Fw@mail.gmail.com","threadId":"60787","inReplyTo":"a211818108053754aca002726d0206623a347952.1706263589.git.ps@pks.im","subject":"Re: [PATCH v3] reftable/stack: adjust permissions of compacted tables","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-01-30T16:56:13Z","receivedAt":"2024-01-30T16:56:28Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On Fri, Jan 26, 2024 at 4:09 AM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> When creating a new compacted table from a range of preexisting ones we\n> don't set the default permissions on the resulting table when specified\n> by the user. This has the effect that the \"core.sharedRepository\" config\n> will not be honored correctly.\n>\n> Fix this bug and add a test to catch this issue. Note that we only test\n> on non-Windows platforms because Windows does not use POSIX permissions\n> natively.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>\n> Changes compared to v2:\n>\n>   - Extended the commit message to say why we don't test on Windows\n>     systems.\n>\n>   - Renamed the `scratch` variable to `path`.\n\nThanks Patrick! This version looks good to me. I have nothing else to add\n\n-Justin\n"}]}