| Message ID | c01b1c335a33e5f44289c520a1634d071d882223.1640287790.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cd1799dea097cee064db1d09966fee88078476e9 |
| Headers | show |
| Series | preliminary fixes for reftable support | expand |
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Han-Wen Nienhuys <hanwen@google.com> > > Create files with mode 0666, so umask works as intended. Provides an override, > which is useful to support shared repos (test t1301-shared-repo.sh). > + /* Default mode for creating files. If unset, use 0666 (+umask) */ > + unsigned int default_permissions; > + Presumably this is primed by a call to get_shared_repository(), possibly followed by calc_shared_perm(), but having it in the interface is a good idea, as it allows us to avoid making these git-core specific calls from reftable/ library proper? > diff --git a/reftable/stack.c b/reftable/stack.c > index df5021ebf08..56bf5f2d84a 100644 > --- a/reftable/stack.c > +++ b/reftable/stack.c > @@ -469,7 +469,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add, > strbuf_addstr(&add->lock_file_name, ".lock"); > > add->lock_file_fd = open(add->lock_file_name.buf, > - O_EXCL | O_CREAT | O_WRONLY, 0644); > + O_EXCL | O_CREAT | O_WRONLY, 0666); This change makes sense. > if (add->lock_file_fd < 0) { > if (errno == EEXIST) { > err = REFTABLE_LOCK_ERROR; > @@ -478,6 +478,13 @@ static int reftable_stack_init_addition(struct reftable_addition *add, > } > goto done; > } > + if (st->config.default_permissions) { > + if (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) { > + err = REFTABLE_IO_ERROR; > + goto done; This part does not exactly make sense, though. If this were a library code meant to link with ONLY with Git, I would have recommended to make a adjust_shared_perm() call from here, without having to have "unsigned int default_permissions" to reftable_write_options structure. I wonder if it is a better design to make the new member in the structure a pointer to a generic and opaque helper function that is called from here, i.e. if (st->config.adjust_perm && st->config.adjust_perm(add->lock_file_name.buf) < 0) { err = REFTABLE_IO_ERROR; goto done; } so that when linking with and calling from git, we can just stuff the pointer to adjust_shared_perm function to the member? > + } > + } > +
On Fri, Dec 24, 2021 at 5:46 AM Junio C Hamano <gitster@pobox.com> wrote: > > if (add->lock_file_fd < 0) { > > if (errno == EEXIST) { > > err = REFTABLE_LOCK_ERROR; > > @@ -478,6 +478,13 @@ static int reftable_stack_init_addition(struct reftable_addition *add, > > } > > goto done; > > } > > + if (st->config.default_permissions) { > > + if (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) { > > + err = REFTABLE_IO_ERROR; > > + goto done; > > This part does not exactly make sense, though. why? > If this were a library code meant to link with ONLY with Git, I > would have recommended to make a adjust_shared_perm() call from > here, without having to have "unsigned int default_permissions" to > reftable_write_options structure. > > I wonder if it is a better design to make the new member in the > structure a pointer to a generic and opaque helper function that is > called from here, i.e. > > if (st->config.adjust_perm && > st->config.adjust_perm(add->lock_file_name.buf) < 0) { > err = REFTABLE_IO_ERROR; > goto done; > } > > so that when linking with and calling from git, we can just stuff > the pointer to adjust_shared_perm function to the member? I read over the adjust_shared_perm function for a bit, but I'm puzzled why its complexity is necessary. It seems to do something with X-bits, maybe for directories, but that doesn't apply here as we're only handling files? We also only write new files, so we never have to look at the existing mode of a file. With the current approach, the option is very clear about what it does, and the unittest is straightforward: set the option, do a write, and check that the files have the specified mode. -- Han-Wen Nienhuys - Google Munich I work 80%. Don't expect answers from me on Fridays. -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Han-Wen Nienhuys <hanwen@google.com> writes: > On Fri, Dec 24, 2021 at 5:46 AM Junio C Hamano <gitster@pobox.com> wrote: >> > if (add->lock_file_fd < 0) { >> > if (errno == EEXIST) { >> > err = REFTABLE_LOCK_ERROR; >> > @@ -478,6 +478,13 @@ static int reftable_stack_init_addition(struct reftable_addition *add, >> > } >> > goto done; >> > } >> > + if (st->config.default_permissions) { >> > + if (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) { >> > + err = REFTABLE_IO_ERROR; >> > + goto done; >> >> This part does not exactly make sense, though. > > why? Explained in the part that follows that statement in the message you are responding to. > I read over the adjust_shared_perm function for a bit, but I'm puzzled > why its complexity is necessary. It seems to do something with X-bits, > maybe for directories, but that doesn't apply here as we're only > handling files? We are propagating executable bit, which affects both directories and files. > We also only write new files, so we never have to look at the existing > mode of a file. It is mostly about defeating the umask of whoever is esecuting "git".
diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h index af36462ced5..a560dc17255 100644 --- a/reftable/reftable-writer.h +++ b/reftable/reftable-writer.h @@ -35,6 +35,9 @@ struct reftable_write_options { */ uint32_t hash_id; + /* Default mode for creating files. If unset, use 0666 (+umask) */ + unsigned int default_permissions; + /* boolean: do not check ref names for validity or dir/file conflicts. */ unsigned skip_name_check : 1; diff --git a/reftable/stack.c b/reftable/stack.c index df5021ebf08..56bf5f2d84a 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -469,7 +469,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add, strbuf_addstr(&add->lock_file_name, ".lock"); add->lock_file_fd = open(add->lock_file_name.buf, - O_EXCL | O_CREAT | O_WRONLY, 0644); + O_EXCL | O_CREAT | O_WRONLY, 0666); if (add->lock_file_fd < 0) { if (errno == EEXIST) { err = REFTABLE_LOCK_ERROR; @@ -478,6 +478,13 @@ static int reftable_stack_init_addition(struct reftable_addition *add, } goto done; } + if (st->config.default_permissions) { + if (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) { + err = REFTABLE_IO_ERROR; + goto done; + } + } + err = stack_uptodate(st); if (err < 0) goto done; @@ -644,7 +651,12 @@ int reftable_addition_add(struct reftable_addition *add, err = REFTABLE_IO_ERROR; goto done; } - + if (add->stack->config.default_permissions) { + if (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) { + err = REFTABLE_IO_ERROR; + goto done; + } + } wr = reftable_new_writer(reftable_fd_write, &tab_fd, &add->stack->config); err = write_table(wr, arg); @@ -900,7 +912,7 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last, strbuf_addstr(&lock_file_name, ".lock"); lock_file_fd = - open(lock_file_name.buf, O_EXCL | O_CREAT | O_WRONLY, 0644); + open(lock_file_name.buf, O_EXCL | O_CREAT | O_WRONLY, 0666); if (lock_file_fd < 0) { if (errno == EEXIST) { err = 1; @@ -931,8 +943,8 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last, strbuf_addstr(&subtab_lock, ".lock"); sublock_file_fd = open(subtab_lock.buf, - O_EXCL | O_CREAT | O_WRONLY, 0644); - if (sublock_file_fd > 0) { + O_EXCL | O_CREAT | O_WRONLY, 0666); + if (sublock_file_fd >= 0) { close(sublock_file_fd); } else if (sublock_file_fd < 0) { if (errno == EEXIST) { @@ -967,7 +979,7 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last, goto done; lock_file_fd = - open(lock_file_name.buf, O_EXCL | O_CREAT | O_WRONLY, 0644); + open(lock_file_name.buf, O_EXCL | O_CREAT | O_WRONLY, 0666); if (lock_file_fd < 0) { if (errno == EEXIST) { err = 1; @@ -977,6 +989,12 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last, goto done; } have_lock = 1; + if (st->config.default_permissions) { + if (chmod(lock_file_name.buf, st->config.default_permissions) < 0) { + err = REFTABLE_IO_ERROR; + goto done; + } + } format_name(&new_table_name, st->readers[first]->min_update_index, st->readers[last]->max_update_index); diff --git a/reftable/stack_test.c b/reftable/stack_test.c index eb0b7228b0c..f4c743db80c 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -17,6 +17,7 @@ https://developers.google.com/open-source/licenses/bsd #include "record.h" #include "test_framework.h" #include "reftable-tests.h" +#include "reader.h" #include <sys/types.h> #include <dirent.h> @@ -138,8 +139,11 @@ static int write_test_log(struct reftable_writer *wr, void *arg) static void test_reftable_stack_add_one(void) { char *dir = get_tmp_dir(__LINE__); - - struct reftable_write_options cfg = { 0 }; + struct strbuf scratch = STRBUF_INIT; + int mask = umask(002); + struct reftable_write_options cfg = { + .default_permissions = 0660, + }; struct reftable_stack *st = NULL; int err; struct reftable_ref_record ref = { @@ -149,8 +153,7 @@ static void test_reftable_stack_add_one(void) .value.symref = "master", }; struct reftable_ref_record dest = { NULL }; - - + struct stat stat_result = { 0 }; err = reftable_new_stack(&st, dir, cfg); EXPECT_ERR(err); @@ -160,6 +163,7 @@ static void test_reftable_stack_add_one(void) err = reftable_stack_read_ref(st, ref.refname, &dest); EXPECT_ERR(err); EXPECT(0 == strcmp("master", dest.value.symref)); + EXPECT(st->readers_len > 0); printf("testing print functionality:\n"); err = reftable_stack_print_directory(dir, GIT_SHA1_FORMAT_ID); @@ -168,9 +172,30 @@ static void test_reftable_stack_add_one(void) err = reftable_stack_print_directory(dir, GIT_SHA256_FORMAT_ID); EXPECT(err == REFTABLE_FORMAT_ERROR); +#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 + reftable_ref_record_release(&dest); reftable_stack_destroy(st); + strbuf_release(&scratch); clear_dir(dir); + umask(mask); } static void test_reftable_stack_uptodate(void)