| Message ID | b2fb6f5ad0c558527341bd8040544d6b0ae5d8a2.1706100744.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] reftable/stack: adjust permissions of compacted tables | expand |
Patrick Steinhardt <ps@pks.im> writes: > When creating a new compacted table from a range of preexisting ones we > don't set the default permissions on the resulting table when specified > by the user. This has the effect that the "core.sharedRepository" config > will not be honored correctly. > > Fix this bug and add a test to catch this issue. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > > Ugh, I totally forgot about the fact that fchmod(3P) isn't actually > available on Windows. I've thus evicted the first patch and changed the > second patch to use chmod(3P) instead. Too bad. > Good catch. This version looks good to me! Nothing to add :)
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. > + 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
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@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. > > + 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
diff --git a/reftable/stack.c b/reftable/stack.c index 7ffeb3ee10..38e784a8ab 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -731,6 +731,12 @@ static int stack_compact_locked(struct reftable_stack *st, int first, int last, strbuf_addstr(temp_tab, ".temp.XXXXXX"); tab_fd = mkstemp(temp_tab->buf); + if (st->config.default_permissions && + chmod(temp_tab->buf, st->config.default_permissions) < 0) { + err = REFTABLE_IO_ERROR; + goto done; + } + wr = reftable_new_writer(reftable_fd_write, &tab_fd, &st->config); err = stack_write_compact(st, wr, first, last, config); 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; + 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 + /* cleanup */ reftable_stack_destroy(st); for (i = 0; i < N; i++) { reftable_ref_record_release(&refs[i]); reftable_log_record_release(&logs[i]); } + strbuf_release(&scratch); clear_dir(dir); }
When creating a new compacted table from a range of preexisting ones we don't set the default permissions on the resulting table when specified by the user. This has the effect that the "core.sharedRepository" config will not be honored correctly. Fix this bug and add a test to catch this issue. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Ugh, I totally forgot about the fact that fchmod(3P) isn't actually available on Windows. I've thus evicted the first patch and changed the second patch to use chmod(3P) instead. Too bad. reftable/stack.c | 6 ++++++ reftable/stack_test.c | 25 +++++++++++++++++++++++-- 2 files changed, 29 insertions(+), 2 deletions(-) base-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c