[PATCH 0/3] preliminary fixes for reftable support

[PATCH 0/3] preliminary fixes for reftable support

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

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

[PATCH 1/3] reftable: fix typo in header

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 reftable/block.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/reftable/block.h b/reftable/block.h
index e207706a644..87c77539b5b 100644
--- a/reftable/block.h
+++ b/reftable/block.h
@@ -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. */
-- 
gitgitgadget

[PATCH 2/3] reftable: signal overflow

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

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.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 reftable/error.c          |  2 ++
 reftable/readwrite_test.c | 35 +++++++++++++++++++++++++++++++++++
 reftable/reftable-error.h |  4 ++++
 reftable/writer.c         |  3 +++
 4 files changed, 44 insertions(+)

diff --git a/reftable/error.c b/reftable/error.c
index f6f16def921..93941f21457 100644
--- a/reftable/error.c
+++ b/reftable/error.c
@@ -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:
diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c
index 5f6bcc2f775..70c7aedba2c 100644
--- a/reftable/readwrite_test.c
+++ b/reftable/readwrite_test.c
@@ -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 = "hanwen@google.com",
+					   .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;
 }
diff --git a/reftable/reftable-error.h b/reftable/reftable-error.h
index 6f89bedf1a5..4c457aaaf89 100644
--- a/reftable/reftable-error.h
+++ b/reftable/reftable-error.h
@@ -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
diff --git a/reftable/writer.c b/reftable/writer.c
index 3ca721e9f64..35c8649c9b7 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -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;
 	}
 
-- 
gitgitgadget

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

[Han-Wen Nienhuys via GitGitGadget]

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

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 reftable/reftable-writer.h |  3 +++
 reftable/stack.c           | 30 ++++++++++++++++++++++++------
 reftable/stack_test.c      | 33 +++++++++++++++++++++++++++++----
 3 files changed, 56 insertions(+), 10 deletions(-)

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)
-- 
gitgitgadget

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

[Junio C Hamano]

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

> +		}
> +	}
> +

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

[Han-Wen Nienhuys]

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

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

[Junio C Hamano]

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

Re: [PATCH 0/3] preliminary fixes for reftable support

[Junio C Hamano]

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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

> 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

Re: [PATCH 0/3] preliminary fixes for reftable support

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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