Skip to contentSkip to conversation

preliminary fixes for reftable support

PATCH In review Han-Wen Nienhuys via GitGitGadget 3 participants 9 messages Dec 23, 2021 – Jan 10, 2022

lore
Share & tools
lore
#0 lore raw

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
#1 lore raw

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
index e207706a644..87c77539b5b 100644
@@ -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
#2 lore raw

PATCH 2/3 reftable: signal overflow

In reply to Han-Wen Nienhuys via GitGitGadget

From: 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
index f6f16def921..93941f21457 100644
@@ -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
index 5f6bcc2f775..70c7aedba2c 100644
@@ -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
index 6f89bedf1a5..4c457aaaf89 100644
@@ -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
index 3ca721e9f64..35c8649c9b7 100644
@@ -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
#3 lore raw

PATCH 3/3 reftable: support preset file mode for writing

In reply to 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 +++
 reftable/stack.c           | 30 ++++++++++++++++++++++++------
 reftable/stack_test.c      | 33 +++++++++++++++++++++++++++++----
 3 files changed, 56 insertions(+), 10 deletions(-)
reftable/reftable-writer.h +3 -0
index af36462ced5..a560dc17255 100644
@@ -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
index df5021ebf08..56bf5f2d84a 100644
@@ -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
index eb0b7228b0c..f4c743db80c 100644
@@ -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
#7 lore raw
In reply to Han-Wen Nienhuys via GitGitGadget
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 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

#8 lore raw
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.

#4 lore raw

Re: [PATCH 3/3] reftable: support preset file mode for writing

In reply to Han-Wen Nienhuys via GitGitGadget

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
index df5021ebf08..56bf5f2d84a 100644
@@ -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 -0
 	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?

Quoted 3 lines
reftable/stack.c +3 -0
+		}
+	}
+
#5 lore raw

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 -0
              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?

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 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.

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_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

#6 lore raw

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 -0
              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.

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_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.

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".