preliminary fixes for reftable support
cover preliminary fixes for reftable support
these two commits to the reftable library prepare for making 2 tests in the test suite pass in my pending changes for reftable support.
This series was built against 'master'. It also has a fix for a fd leak (>= 0 vs > 0), which is part of my reftable-coverity fixes topic.
Han-Wen Nienhuys (3):
reftable: fix typo in header
reftable: signal overflow
reftable: support preset file mode for writing
reftable/block.h | 2 +- reftable/error.c | 2 ++ reftable/readwrite_test.c | 35 +++++++++++++++++++++++++++++++++++ reftable/reftable-error.h | 4 ++++ reftable/reftable-writer.h | 3 +++ reftable/stack.c | 30 ++++++++++++++++++++++++------ reftable/stack_test.c | 33 +++++++++++++++++++++++++++++---- reftable/writer.c | 3 +++ 8 files changed, 101 insertions(+), 11 deletions(-)
base-commit: fae76fe5da3df25d752f2251b7ccda3f62813aa9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1164%2Fhanwen%2Freftable-features-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1164/hanwen/reftable-features-v1
Pull-Request: https://github.com/git/git/pull/1164
Signature · 1 line
gitgitgadget
PATCH 1/3 reftable: fix typo in header
From: Han-Wen Nienhuys <[email protected]>
--- reftable/block.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
reftable/block.h +1 -1
@@ -21,7 +21,7 @@ struct block_writer {uint8_t *buf;uint32_t block_size; -/* Offset ofof the global header. Nonzero in the first block only. */ +/* Offset of the global header. Nonzero in the first block only. */ uint32_t header_off;/* How often to restart keys. */
Signature · 1 line
gitgitgadget
PATCH 2/3 reftable: signal overflow
In reply to Han-Wen Nienhuys via GitGitGadgetFrom: Han-Wen Nienhuys <[email protected]>
reflog entries have unbounded size. In theory, each log ('g') block in reftable can have an arbitrary size, so the format allows for arbitrarily sized reflog messages. However, in the implementation, we are not scaling the log blocks up with the message, and writing a large message fails.
This triggers a failure for reftable in t7006-pager.sh.
Until this is fixed more structurally, report an error from within the reftable library for easier debugging.
--- reftable/error.c | 2 ++ reftable/readwrite_test.c | 35 +++++++++++++++++++++++++++++++++++ reftable/reftable-error.h | 4 ++++ reftable/writer.c | 3 +++ 4 files changed, 44 insertions(+)
reftable/error.c +2 -0
@@ -32,6 +32,8 @@ const char *reftable_error_str(int err)return "wrote empty table" ;case REFTABLE_REFNAME_ERROR :return "invalid refname" ; +case REFTABLE_ENTRY_TOO_BIG_ERROR : +return "entry too large" ;case -1 :return "general error" ;default :
reftable/readwrite_test.c +35 -0
@@ -155,6 +155,40 @@ static void test_log_buffer_size(void)strbuf_release (&buf); } +static void test_log_overflow (void ) +{ +struct strbuf buf =STRBUF_INIT ; +char msg[256 ] = {0 }; +struct reftable_write_options opts = { + .block_size =ARRAY_SIZE (msg), + }; +int err; +struct reftable_log_record + log = { .refname ="refs/heads/master" , + .update_index =0xa , + .value_type =REFTABLE_LOG_UPDATE , + .value = { .update = { + .name ="Han-Wen Nienhuys" , + .email ="[email protected]" , + .tz_offset =100 , + .time =0x5e430672 , + .message = msg, + } } }; +struct reftable_writer *w = +reftable_new_writer (&strbuf_add_void, &buf, &opts); + +uint8_t hash1[GIT_SHA1_RAWSZ ] = {1 }, hash2[GIT_SHA1_RAWSZ ] = {2 }; + +memset (msg,'x' ,sizeof (msg) -1 ); + log.value.update.old_hash = hash1; + log.value.update.new_hash = hash2; +reftable_writer_set_limits (w, update_index, update_index); + err =reftable_writer_add_log (w, &log); +EXPECT (err ==REFTABLE_ENTRY_TOO_BIG_ERROR ); +reftable_writer_free (w); +strbuf_release (&buf); +} +static void test_log_write_read (void ) {int N =2 ;
@@ -648,5 +682,6 @@ int readwrite_test_main(int argc, const char *argv[])RUN_TEST (test_table_refs_for_no_index);RUN_TEST (test_table_refs_for_obj_index);RUN_TEST (test_write_empty_table); +RUN_TEST (test_log_overflow);return 0 ; }
reftable/reftable-error.h +4 -0
@@ -53,6 +53,10 @@ enum reftable_error {/* Invalid ref name. */ REFTABLE_REFNAME_ERROR =-10 , + +/* Entry does not fit. This can happen when writing outsize reflog +messages. */ + REFTABLE_ENTRY_TOO_BIG_ERROR = -11, };/* convert the numeric error code to a string. The string should not be
reftable/writer.c +3 -0
@@ -239,6 +239,9 @@ static int writer_add_record(struct reftable_writer *w,writer_reinit_block_writer (w,reftable_record_type (rec)); err =block_writer_add (w->block_writer, rec);if (err <0 ) { +/* we are writing into memory, so an error can only mean it +* doesn't fit. */ + err =REFTABLE_ENTRY_TOO_BIG_ERROR ; goto done; }
Signature · 1 line
gitgitgadget
PATCH 3/3 reftable: support preset file mode for writing
In reply to Han-Wen Nienhuys via GitGitGadgetFrom: Han-Wen Nienhuys <[email protected]>
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).
--- reftable/reftable-writer.h | 3 +++ reftable/stack.c | 30 ++++++++++++++++++++++++------ reftable/stack_test.c | 33 +++++++++++++++++++++++++++++---- 3 files changed, 56 insertions(+), 10 deletions(-)
reftable/reftable-writer.h +3 -0
@@ -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;
reftable/stack.c +24 -6
@@ -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);
reftable/stack_test.c +29 -4
@@ -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 )
Signature · 1 line
gitgitgadget
Re: [PATCH 3/3] reftable: support preset file mode for writing
reftable/reftable-writer.h struct reftable_write_options { · line 40
Quoted 8 lines · Han-Wen Nienhuys via GitGitGadget
From: Han-Wen Nienhuys <[email protected]>
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).reftable/reftable-writer.h +3
-0+/* 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?
reftable/stack.c static int reftable_stack_init_addition(struct reftable_addition *add, · line 472
Quoted 10 lines
reftable/stack.c +1
-1@@ -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.
reftable/stack.c static int reftable_stack_init_addition(struct reftable_addition *add, · line 484
Quoted 11 lines
reftable/stack.c +4
-0if (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?
Quoted 3 lines
reftable/stack.c +3
-0+ } + } +
Re: [PATCH 3/3] reftable: support preset file mode for writing
Quoted 5 lines · Junio C Hamano
Quoted 5 lines
if (add->lock_file_fd < 0) { if (errno == EEXIST) { err = REFTABLE_LOCK_ERROR;(quoted diff) +0
-0@@ -478,6 +478,13 @@ static int reftable_stack_init_addition(struct reftable_addition *add, }
Quoted 8 lines
reftable/stack.c static int reftable_stack_init_addition(struct reftable_addition *add, · line 484
Quoted 6 lines
reftable/stack.c +4
-0goto 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?
Quoted 9 lines
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 intdefault_permissions" toreftable_write_optionsstructure.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.
Quoted 8 lines
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_permfunction 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
Re: [PATCH 3/3] reftable: support preset file mode for writing
Quoted 15 lines · Han-Wen Nienhuys
Quoted 5 lines · Junio C Hamano
Quoted 5 lines
if (add->lock_file_fd < 0) { if (errno == EEXIST) { err = REFTABLE_LOCK_ERROR;(quoted diff) +0
-0@@ -478,6 +478,13 @@ static int reftable_stack_init_addition(struct reftable_addition *add, }Quoted 8 lines
reftable/stack.c static int reftable_stack_init_addition(struct reftable_addition *add, · line 484
Quoted 6 lines
reftable/stack.c +4
-0goto 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.
Quoted 1 line
why?
Explained in the part that follows that statement in the message you are responding to.
Quoted 4 lines
I read over the
adjust_shared_permfunction 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.
Quoted 2 lines
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".
Quoted 2 lines · Han-Wen Nienhuys via GitGitGadget
these two commits to the reftable library prepare for making 2 tests in the test suite pass in my pending changes for reftable support.
Welcome again to our "we cannot count to three" club.
Quoted 2 lines
This series was built against 'master'. It also has a fix for a fd leak (>= 0 vs > 0), which is part of my reftable-coverity fixes topic.
What is going on here? We have the same fix in two series? Are these two series meant to be applied, or is this a beginning of splitting and resubmitting the other larger series into smaller chunks?
I am not opposed to having two identical fixes to a high priority problem, one in a long series that may take longer to graduate and the other one in a short series that is trivial to verify. I am not opposed to retract a longer series and trickle a number of series' that replace it, either. I just wanted to know what is happening here.
Thanks.
Quoted 20 lines
Han-Wen Nienhuys (3):
reftable: fix typo in header
reftable: signal overflow
reftable: support preset file mode for writingreftable/block.h | 2 +- reftable/error.c | 2 ++ reftable/readwrite_test.c | 35 +++++++++++++++++++++++++++++++++++ reftable/reftable-error.h | 4 ++++ reftable/reftable-writer.h | 3 +++ reftable/stack.c | 30 ++++++++++++++++++++++++------ reftable/stack_test.c | 33 +++++++++++++++++++++++++++++---- reftable/writer.c | 3 +++ 8 files changed, 101 insertions(+), 11 deletions(-)base-commit:
fae76fe5da3df25d752f2251b7ccda3f62813aa9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1164%2Fhanwen%2Freftable-features-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1164/hanwen/reftable-features-v1
Pull-Request: https://github.com/git/git/pull/1164
Quoted 11 lines · Junio C Hamano
What is going on here? We have the same fix in two series? Are these two series meant to be applied, or is this a beginning of splitting and resubmitting the other larger series into smaller chunks?
I am not opposed to having two identical fixes to a high priority problem, one in a long series that may take longer to graduate and the other one in a short series that is trivial to verify. I am not opposed to retract a longer series and trickle a number of series' that replace it, either. I just wanted to know what is happening here.
I guess the answer is neither.
An unrelated change in this topic needed to touch a line that is close to a line that was changed in the other topic, and you made the same change as the latter in the same hunk---which in this case helps with the merge of these two topics into the integration branches.
Thanks.