{"thread":{"id":"57145","subject":"[PATCH 0/3] preliminary fixes for reftable support","startedAt":"2021-12-23T19:29:54Z","lastAt":"2022-01-10T19:14:43Z","messageCount":9,"participants":["Han-Wen Nienhuys via GitGitGadget","Junio C Hamano","Han-Wen Nienhuys"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"444876","messageId":"pull.1164.git.git.1640287790.gitgitgadget@gmail.com","threadId":"57145","inReplyTo":null,"subject":"[PATCH 0/3] preliminary fixes for reftable support","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-23T19:29:47Z","receivedAt":"2021-12-23T19:29:54Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"these two commits to the reftable library prepare for making 2 tests in the\ntest suite pass in my pending changes for reftable support.\n\nThis series was built against 'master'. It also has a fix for a fd leak (>=\n0 vs > 0), which is part of my reftable-coverity fixes topic.\n\nHan-Wen Nienhuys (3):\n  reftable: fix typo in header\n  reftable: signal overflow\n  reftable: support preset file mode for writing\n\n reftable/block.h           |  2 +-\n reftable/error.c           |  2 ++\n reftable/readwrite_test.c  | 35 +++++++++++++++++++++++++++++++++++\n reftable/reftable-error.h  |  4 ++++\n reftable/reftable-writer.h |  3 +++\n reftable/stack.c           | 30 ++++++++++++++++++++++++------\n reftable/stack_test.c      | 33 +++++++++++++++++++++++++++++----\n reftable/writer.c          |  3 +++\n 8 files changed, 101 insertions(+), 11 deletions(-)\n\n\nbase-commit: fae76fe5da3df25d752f2251b7ccda3f62813aa9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1164%2Fhanwen%2Freftable-features-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1164/hanwen/reftable-features-v1\nPull-Request: https://github.com/git/git/pull/1164\n-- \ngitgitgadget\n"},{"id":"444877","messageId":"603b0a1ada98692f504f2a004b49d4f7fe5b30c4.1640287790.git.gitgitgadget@gmail.com","threadId":"57145","inReplyTo":"pull.1164.git.git.1640287790.gitgitgadget@gmail.com","subject":"[PATCH 1/3] reftable: fix typo in header","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-23T19:29:48Z","receivedAt":"2021-12-23T19:29:57Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n reftable/block.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/reftable/block.h b/reftable/block.h\nindex e207706a644..87c77539b5b 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -21,7 +21,7 @@ struct block_writer {\n \tuint8_t *buf;\n \tuint32_t block_size;\n \n-\t/* Offset ofof the global header. Nonzero in the first block only. */\n+\t/* Offset of the global header. Nonzero in the first block only. */\n \tuint32_t header_off;\n \n \t/* How often to restart keys. */\n-- \ngitgitgadget\n\n"},{"id":"444878","messageId":"53c1519525ff170bcc086de66cd82294d43bc69e.1640287790.git.gitgitgadget@gmail.com","threadId":"57145","inReplyTo":"pull.1164.git.git.1640287790.gitgitgadget@gmail.com","subject":"[PATCH 2/3] reftable: signal overflow","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-23T19:29:49Z","receivedAt":"2021-12-23T19:29:59Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nreflog entries have unbounded size. In theory, each log ('g') block in reftable\ncan have an arbitrary size, so the format allows for arbitrarily sized reflog\nmessages. However, in the implementation, we are not scaling the log blocks up\nwith the message, and writing a large message fails.\n\nThis triggers a failure for reftable in t7006-pager.sh.\n\nUntil this is fixed more structurally, report an error from within the reftable\nlibrary for easier debugging.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n reftable/error.c          |  2 ++\n reftable/readwrite_test.c | 35 +++++++++++++++++++++++++++++++++++\n reftable/reftable-error.h |  4 ++++\n reftable/writer.c         |  3 +++\n 4 files changed, 44 insertions(+)\n\ndiff --git a/reftable/error.c b/reftable/error.c\nindex f6f16def921..93941f21457 100644\n--- a/reftable/error.c\n+++ b/reftable/error.c\n@@ -32,6 +32,8 @@ const char *reftable_error_str(int err)\n \t\treturn \"wrote empty table\";\n \tcase REFTABLE_REFNAME_ERROR:\n \t\treturn \"invalid refname\";\n+\tcase REFTABLE_ENTRY_TOO_BIG_ERROR:\n+\t\treturn \"entry too large\";\n \tcase -1:\n \t\treturn \"general error\";\n \tdefault:\ndiff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c\nindex 5f6bcc2f775..70c7aedba2c 100644\n--- a/reftable/readwrite_test.c\n+++ b/reftable/readwrite_test.c\n@@ -155,6 +155,40 @@ static void test_log_buffer_size(void)\n \tstrbuf_release(&buf);\n }\n \n+static void test_log_overflow(void)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tchar msg[256] = { 0 };\n+\tstruct reftable_write_options opts = {\n+\t\t.block_size = ARRAY_SIZE(msg),\n+\t};\n+\tint err;\n+\tstruct reftable_log_record\n+\t\tlog = { .refname = \"refs/heads/master\",\n+\t\t\t.update_index = 0xa,\n+\t\t\t.value_type = REFTABLE_LOG_UPDATE,\n+\t\t\t.value = { .update = {\n+\t\t\t\t\t   .name = \"Han-Wen Nienhuys\",\n+\t\t\t\t\t   .email = \"hanwen@google.com\",\n+\t\t\t\t\t   .tz_offset = 100,\n+\t\t\t\t\t   .time = 0x5e430672,\n+\t\t\t\t\t   .message = msg,\n+\t\t\t\t   } } };\n+\tstruct reftable_writer *w =\n+\t\treftable_new_writer(&strbuf_add_void, &buf, &opts);\n+\n+\tuint8_t hash1[GIT_SHA1_RAWSZ]  = {1}, hash2[GIT_SHA1_RAWSZ] = { 2 };\n+\n+\tmemset(msg, 'x', sizeof(msg) - 1);\n+\tlog.value.update.old_hash = hash1;\n+\tlog.value.update.new_hash = hash2;\n+\treftable_writer_set_limits(w, update_index, update_index);\n+\terr = reftable_writer_add_log(w, &log);\n+\tEXPECT(err == REFTABLE_ENTRY_TOO_BIG_ERROR);\n+\treftable_writer_free(w);\n+\tstrbuf_release(&buf);\n+}\n+\n static void test_log_write_read(void)\n {\n \tint N = 2;\n@@ -648,5 +682,6 @@ int readwrite_test_main(int argc, const char *argv[])\n \tRUN_TEST(test_table_refs_for_no_index);\n \tRUN_TEST(test_table_refs_for_obj_index);\n \tRUN_TEST(test_write_empty_table);\n+\tRUN_TEST(test_log_overflow);\n \treturn 0;\n }\ndiff --git a/reftable/reftable-error.h b/reftable/reftable-error.h\nindex 6f89bedf1a5..4c457aaaf89 100644\n--- a/reftable/reftable-error.h\n+++ b/reftable/reftable-error.h\n@@ -53,6 +53,10 @@ enum reftable_error {\n \n \t/* Invalid ref name. */\n \tREFTABLE_REFNAME_ERROR = -10,\n+\n+\t/* Entry does not fit. This can happen when writing outsize reflog\n+\t   messages. */\n+\tREFTABLE_ENTRY_TOO_BIG_ERROR = -11,\n };\n \n /* convert the numeric error code to a string. The string should not be\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 3ca721e9f64..35c8649c9b7 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -239,6 +239,9 @@ static int writer_add_record(struct reftable_writer *w,\n \twriter_reinit_block_writer(w, reftable_record_type(rec));\n \terr = block_writer_add(w->block_writer, rec);\n \tif (err < 0) {\n+\t\t/* we are writing into memory, so an error can only mean it\n+\t\t * doesn't fit. */\n+\t\terr = REFTABLE_ENTRY_TOO_BIG_ERROR;\n \t\tgoto done;\n \t}\n \n-- \ngitgitgadget\n\n"},{"id":"444879","messageId":"c01b1c335a33e5f44289c520a1634d071d882223.1640287790.git.gitgitgadget@gmail.com","threadId":"57145","inReplyTo":"pull.1164.git.git.1640287790.gitgitgadget@gmail.com","subject":"[PATCH 3/3] reftable: support preset file mode for writing","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-23T19:29:50Z","receivedAt":"2021-12-23T19:30:04Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nCreate files with mode 0666, so umask works as intended. Provides an override,\nwhich is useful to support shared repos (test t1301-shared-repo.sh).\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n reftable/reftable-writer.h |  3 +++\n reftable/stack.c           | 30 ++++++++++++++++++++++++------\n reftable/stack_test.c      | 33 +++++++++++++++++++++++++++++----\n 3 files changed, 56 insertions(+), 10 deletions(-)\n\ndiff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\nindex af36462ced5..a560dc17255 100644\n--- a/reftable/reftable-writer.h\n+++ b/reftable/reftable-writer.h\n@@ -35,6 +35,9 @@ struct reftable_write_options {\n \t */\n \tuint32_t hash_id;\n \n+\t/* Default mode for creating files. If unset, use 0666 (+umask) */\n+\tunsigned int default_permissions;\n+\n \t/* boolean: do not check ref names for validity or dir/file conflicts.\n \t */\n \tunsigned skip_name_check : 1;\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex df5021ebf08..56bf5f2d84a 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -469,7 +469,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \tstrbuf_addstr(&add->lock_file_name, \".lock\");\n \n \tadd->lock_file_fd = open(add->lock_file_name.buf,\n-\t\t\t\t O_EXCL | O_CREAT | O_WRONLY, 0644);\n+\t\t\t\t O_EXCL | O_CREAT | O_WRONLY, 0666);\n \tif (add->lock_file_fd < 0) {\n \t\tif (errno == EEXIST) {\n \t\t\terr = REFTABLE_LOCK_ERROR;\n@@ -478,6 +478,13 @@ 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_name.buf, st->config.default_permissions) < 0) {\n+\t\t\terr = REFTABLE_IO_ERROR;\n+\t\t\tgoto done;\n+\t\t}\n+\t}\n+\n \terr = stack_uptodate(st);\n \tif (err < 0)\n \t\tgoto done;\n@@ -644,7 +651,12 @@ int reftable_addition_add(struct reftable_addition *add,\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n-\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+\t}\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@@ -900,7 +912,7 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,\n \tstrbuf_addstr(&lock_file_name, \".lock\");\n \n \tlock_file_fd =\n-\t\topen(lock_file_name.buf, O_EXCL | O_CREAT | O_WRONLY, 0644);\n+\t\topen(lock_file_name.buf, O_EXCL | O_CREAT | O_WRONLY, 0666);\n \tif (lock_file_fd < 0) {\n \t\tif (errno == EEXIST) {\n \t\t\terr = 1;\n@@ -931,8 +943,8 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,\n \t\tstrbuf_addstr(&subtab_lock, \".lock\");\n \n \t\tsublock_file_fd = open(subtab_lock.buf,\n-\t\t\t\t       O_EXCL | O_CREAT | O_WRONLY, 0644);\n-\t\tif (sublock_file_fd > 0) {\n+\t\t\t\t       O_EXCL | O_CREAT | O_WRONLY, 0666);\n+\t\tif (sublock_file_fd >= 0) {\n \t\t\tclose(sublock_file_fd);\n \t\t} else if (sublock_file_fd < 0) {\n \t\t\tif (errno == EEXIST) {\n@@ -967,7 +979,7 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,\n \t\tgoto done;\n \n \tlock_file_fd =\n-\t\topen(lock_file_name.buf, O_EXCL | O_CREAT | O_WRONLY, 0644);\n+\t\topen(lock_file_name.buf, O_EXCL | O_CREAT | O_WRONLY, 0666);\n \tif (lock_file_fd < 0) {\n \t\tif (errno == EEXIST) {\n \t\t\terr = 1;\n@@ -977,6 +989,12 @@ 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+\t}\n \n \tformat_name(&new_table_name, st->readers[first]->min_update_index,\n \t\t    st->readers[last]->max_update_index);\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex eb0b7228b0c..f4c743db80c 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -17,6 +17,7 @@ https://developers.google.com/open-source/licenses/bsd\n #include \"record.h\"\n #include \"test_framework.h\"\n #include \"reftable-tests.h\"\n+#include \"reader.h\"\n \n #include <sys/types.h>\n #include <dirent.h>\n@@ -138,8 +139,11 @@ static int write_test_log(struct reftable_writer *wr, void *arg)\n static void test_reftable_stack_add_one(void)\n {\n \tchar *dir = get_tmp_dir(__LINE__);\n-\n-\tstruct reftable_write_options cfg = { 0 };\n+\tstruct strbuf scratch = STRBUF_INIT;\n+\tint mask = umask(002);\n+\tstruct reftable_write_options cfg = {\n+\t\t.default_permissions = 0660,\n+\t};\n \tstruct reftable_stack *st = NULL;\n \tint err;\n \tstruct reftable_ref_record ref = {\n@@ -149,8 +153,7 @@ static void test_reftable_stack_add_one(void)\n \t\t.value.symref = \"master\",\n \t};\n \tstruct reftable_ref_record dest = { NULL };\n-\n-\n+\tstruct stat stat_result = { 0 };\n \terr = reftable_new_stack(&st, dir, cfg);\n \tEXPECT_ERR(err);\n \n@@ -160,6 +163,7 @@ static void test_reftable_stack_add_one(void)\n \terr = reftable_stack_read_ref(st, ref.refname, &dest);\n \tEXPECT_ERR(err);\n \tEXPECT(0 == strcmp(\"master\", dest.value.symref));\n+\tEXPECT(st->readers_len > 0);\n \n \tprintf(\"testing print functionality:\\n\");\n \terr = reftable_stack_print_directory(dir, GIT_SHA1_FORMAT_ID);\n@@ -168,9 +172,30 @@ static void test_reftable_stack_add_one(void)\n \terr = reftable_stack_print_directory(dir, GIT_SHA256_FORMAT_ID);\n \tEXPECT(err == REFTABLE_FORMAT_ERROR);\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 \treftable_ref_record_release(&dest);\n \treftable_stack_destroy(st);\n+\tstrbuf_release(&scratch);\n \tclear_dir(dir);\n+\tumask(mask);\n }\n \n static void test_reftable_stack_uptodate(void)\n-- \ngitgitgadget\n"},{"id":"444880","messageId":"xmqqk0fv83sk.fsf@gitster.g","threadId":"57145","inReplyTo":"pull.1164.git.git.1640287790.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] preliminary fixes for reftable support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-23T20:27:55Z","receivedAt":"2021-12-23T20:28:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> these two commits to the reftable library prepare for making 2 tests in the\n> test suite pass in my pending changes for reftable support.\n\nWelcome again to our \"we cannot count to three\" club.\n\n> This series was built against 'master'. It also has a fix for a fd leak (>=\n> 0 vs > 0), which is part of my reftable-coverity fixes topic.\n\nWhat is going on here?  We have the same fix in two series?  Are\nthese two series meant to be applied, or is this a beginning of\nsplitting and resubmitting the other larger series into smaller\nchunks?\n\nI am not opposed to having two identical fixes to a high priority\nproblem, one in a long series that may take longer to graduate and\nthe other one in a short series that is trivial to verify.  I am not\nopposed to retract a longer series and trickle a number of series'\nthat replace it, either.  I just wanted to know what is happening\nhere.\n\nThanks.\n\n> Han-Wen Nienhuys (3):\n>   reftable: fix typo in header\n>   reftable: signal overflow\n>   reftable: support preset file mode for writing\n>\n>  reftable/block.h           |  2 +-\n>  reftable/error.c           |  2 ++\n>  reftable/readwrite_test.c  | 35 +++++++++++++++++++++++++++++++++++\n>  reftable/reftable-error.h  |  4 ++++\n>  reftable/reftable-writer.h |  3 +++\n>  reftable/stack.c           | 30 ++++++++++++++++++++++++------\n>  reftable/stack_test.c      | 33 +++++++++++++++++++++++++++++----\n>  reftable/writer.c          |  3 +++\n>  8 files changed, 101 insertions(+), 11 deletions(-)\n>\n>\n> base-commit: fae76fe5da3df25d752f2251b7ccda3f62813aa9\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1164%2Fhanwen%2Freftable-features-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1164/hanwen/reftable-features-v1\n> Pull-Request: https://github.com/git/git/pull/1164\n"},{"id":"444893","messageId":"xmqqa6gq6959.fsf@gitster.g","threadId":"57145","inReplyTo":"xmqqk0fv83sk.fsf@gitster.g","subject":"Re: [PATCH 0/3] preliminary fixes for reftable support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-24T02:15:14Z","receivedAt":"2021-12-24T02:15:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> What is going on here?  We have the same fix in two series?  Are\n> these two series meant to be applied, or is this a beginning of\n> splitting and resubmitting the other larger series into smaller\n> chunks?\n>\n> I am not opposed to having two identical fixes to a high priority\n> problem, one in a long series that may take longer to graduate and\n> the other one in a short series that is trivial to verify.  I am not\n> opposed to retract a longer series and trickle a number of series'\n> that replace it, either.  I just wanted to know what is happening\n> here.\n\nI guess the answer is neither.\n\nAn unrelated change in this topic needed to touch a line that is\nclose to a line that was changed in the other topic, and you made\nthe same change as the latter in the same hunk---which in this case\nhelps with the merge of these two topics into the integration\nbranches.\n\nThanks.\n"},{"id":"444897","messageId":"xmqqsfui4nkr.fsf@gitster.g","threadId":"57145","inReplyTo":"c01b1c335a33e5f44289c520a1634d071d882223.1640287790.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] reftable: support preset file mode for writing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-24T04:46:28Z","receivedAt":"2021-12-24T04:46:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Han-Wen Nienhuys <hanwen@google.com>\n>\n> Create files with mode 0666, so umask works as intended. Provides an override,\n> which is useful to support shared repos (test t1301-shared-repo.sh).\n\n> +\t/* Default mode for creating files. If unset, use 0666 (+umask) */\n> +\tunsigned int default_permissions;\n> +\n\nPresumably this is primed by a call to get_shared_repository(),\npossibly followed by calc_shared_perm(), but having it in the\ninterface is a good idea, as it allows us to avoid making these\ngit-core specific calls from reftable/ library proper?\n\n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index df5021ebf08..56bf5f2d84a 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -469,7 +469,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n>  \tstrbuf_addstr(&add->lock_file_name, \".lock\");\n>  \n>  \tadd->lock_file_fd = open(add->lock_file_name.buf,\n> -\t\t\t\t O_EXCL | O_CREAT | O_WRONLY, 0644);\n> +\t\t\t\t O_EXCL | O_CREAT | O_WRONLY, 0666);\n\nThis change makes sense.\n\n>  \tif (add->lock_file_fd < 0) {\n>  \t\tif (errno == EEXIST) {\n>  \t\t\terr = REFTABLE_LOCK_ERROR;\n> @@ -478,6 +478,13 @@ 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_name.buf, st->config.default_permissions) < 0) {\n> +\t\t\terr = REFTABLE_IO_ERROR;\n> +\t\t\tgoto done;\n\nThis part does not exactly make sense, though.\n\nIf this were a library code meant to link with ONLY with Git, I\nwould have recommended to make a adjust_shared_perm() call from\nhere, without having to have \"unsigned int default_permissions\" to\nreftable_write_options structure.\n\nI wonder if it is a better design to make the new member in the\nstructure a pointer to a generic and opaque helper function that is\ncalled from here, i.e.\n\n\tif (st->config.adjust_perm &&\n            st->config.adjust_perm(add->lock_file_name.buf) < 0) {\n\t\terr = REFTABLE_IO_ERROR;\n\t\tgoto done;\n\t}\n\nso that when linking with and calling from git, we can just stuff\nthe pointer to adjust_shared_perm function to the member?\n\n> +\t\t}\n> +\t}\n> +\n"},{"id":"445865","messageId":"CAFQ2z_MEN2j-sHmGK4SA6N8P9bHvjO2=+G0XxXp_9vzLRxwKuw@mail.gmail.com","threadId":"57145","inReplyTo":"xmqqsfui4nkr.fsf@gitster.g","subject":"Re: [PATCH 3/3] reftable: support preset file mode for writing","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2022-01-10T19:01:54Z","receivedAt":"2022-01-10T19:02:08Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Fri, Dec 24, 2021 at 5:46 AM Junio C Hamano <gitster@pobox.com> wrote:\n> >       if (add->lock_file_fd < 0) {\n> >               if (errno == EEXIST) {\n> >                       err = REFTABLE_LOCK_ERROR;\n> > @@ -478,6 +478,13 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n> >               }\n> >               goto done;\n> >       }\n> > +     if (st->config.default_permissions) {\n> > +             if (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) {\n> > +                     err = REFTABLE_IO_ERROR;\n> > +                     goto done;\n>\n> This part does not exactly make sense, though.\n\nwhy?\n\n> If this were a library code meant to link with ONLY with Git, I\n> would have recommended to make a adjust_shared_perm() call from\n> here, without having to have \"unsigned int default_permissions\" to\n> reftable_write_options structure.\n>\n> I wonder if it is a better design to make the new member in the\n> structure a pointer to a generic and opaque helper function that is\n> called from here, i.e.\n>\n>         if (st->config.adjust_perm &&\n>             st->config.adjust_perm(add->lock_file_name.buf) < 0) {\n>                 err = REFTABLE_IO_ERROR;\n>                 goto done;\n>         }\n>\n> so that when linking with and calling from git, we can just stuff\n> the pointer to adjust_shared_perm function to the member?\n\nI read over the adjust_shared_perm function for a bit, but I'm puzzled\nwhy its complexity is necessary. It seems to do something with X-bits,\nmaybe for directories, but that doesn't apply here as we're only\nhandling files?\nWe also only write new files, so we never have to look at the existing\nmode of a file.\n\nWith the current approach, the option is very clear about what it\ndoes, and the unittest is straightforward: set the option, do a write,\nand check that the files have the specified mode.\n\n--\nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"445867","messageId":"xmqqk0f7z9lr.fsf@gitster.g","threadId":"57145","inReplyTo":"CAFQ2z_MEN2j-sHmGK4SA6N8P9bHvjO2=+G0XxXp_9vzLRxwKuw@mail.gmail.com","subject":"Re: [PATCH 3/3] reftable: support preset file mode for writing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-10T19:14:40Z","receivedAt":"2022-01-10T19:14:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\n> On Fri, Dec 24, 2021 at 5:46 AM Junio C Hamano <gitster@pobox.com> wrote:\n>> >       if (add->lock_file_fd < 0) {\n>> >               if (errno == EEXIST) {\n>> >                       err = REFTABLE_LOCK_ERROR;\n>> > @@ -478,6 +478,13 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n>> >               }\n>> >               goto done;\n>> >       }\n>> > +     if (st->config.default_permissions) {\n>> > +             if (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) {\n>> > +                     err = REFTABLE_IO_ERROR;\n>> > +                     goto done;\n>>\n>> This part does not exactly make sense, though.\n>\n> why?\n\nExplained in the part that follows that statement in the message you\nare responding to.\n\n> I read over the adjust_shared_perm function for a bit, but I'm puzzled\n> why its complexity is necessary. It seems to do something with X-bits,\n> maybe for directories, but that doesn't apply here as we're only\n> handling files?\n\nWe are propagating executable bit, which affects both directories\nand files.\n\n> We also only write new files, so we never have to look at the existing\n> mode of a file.\n\nIt is mostly about defeating the umask of whoever is esecuting \"git\".\n"}]}