[PATCH v3 04/11] reftable: remove name checks

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 18516 bytes --]

In the preceding commit we have disabled name checks in the "reftable"
backend. These checks were responsible for verifying multiple things
when writing records to the reftable stack:

  - Detecting file/directory conflicts. Starting with the preceding
    commits this is now handled by the reftable backend itself via
    `refs_verify_refname_available()`.

  - Validating refnames. This is handled by `check_refname_format()` in
    the generic ref transacton layer.

The code in the reftable library is thus not used anymore and likely to
bitrot over time. Remove it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Makefile                   |   2 -
 refs/reftable-backend.c    |   5 -
 reftable/error.c           |   2 -
 reftable/refname.c         | 206 -------------------------------------
 reftable/refname.h         |  29 ------
 reftable/refname_test.c    | 101 ------------------
 reftable/reftable-error.h  |   3 -
 reftable/reftable-tests.h  |   1 -
 reftable/reftable-writer.h |   4 -
 reftable/stack.c           |  67 +-----------
 reftable/stack_test.c      |  39 -------
 t/helper/test-reftable.c   |   1 -
 12 files changed, 1 insertion(+), 459 deletions(-)
 delete mode 100644 reftable/refname.c
 delete mode 100644 reftable/refname.h
 delete mode 100644 reftable/refname_test.c

diff --git a/Makefile b/Makefile
index c43c1bd1a0..05e3d37581 100644
--- a/Makefile
+++ b/Makefile
@@ -2655,7 +2655,6 @@ REFTABLE_OBJS += reftable/merged.o
 REFTABLE_OBJS += reftable/pq.o
 REFTABLE_OBJS += reftable/reader.o
 REFTABLE_OBJS += reftable/record.o
-REFTABLE_OBJS += reftable/refname.o
 REFTABLE_OBJS += reftable/generic.o
 REFTABLE_OBJS += reftable/stack.o
 REFTABLE_OBJS += reftable/tree.o
@@ -2668,7 +2667,6 @@ REFTABLE_TEST_OBJS += reftable/merged_test.o
 REFTABLE_TEST_OBJS += reftable/pq_test.o
 REFTABLE_TEST_OBJS += reftable/record_test.o
 REFTABLE_TEST_OBJS += reftable/readwrite_test.o
-REFTABLE_TEST_OBJS += reftable/refname_test.o
 REFTABLE_TEST_OBJS += reftable/stack_test.o
 REFTABLE_TEST_OBJS += reftable/test_framework.o
 REFTABLE_TEST_OBJS += reftable/tree_test.o
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 7515dd3019..8a54b0d8b2 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -247,11 +247,6 @@ static struct ref_store *reftable_be_init(struct repository *repo,
 	refs->write_options.block_size = 4096;
 	refs->write_options.hash_id = repo->hash_algo->format_id;
 	refs->write_options.default_permissions = calc_shared_perm(0666 & ~mask);
-	/*
-	 * We verify names via `refs_verify_refname_available()`, so there is
-	 * no need to do the same checks in the reftable library again.
-	 */
-	refs->write_options.skip_name_check = 1;
 
 	/*
 	 * Set up the main reftable stack that is hosted in GIT_COMMON_DIR.
diff --git a/reftable/error.c b/reftable/error.c
index 0d1766735e..169f89d2f1 100644
--- a/reftable/error.c
+++ b/reftable/error.c
@@ -27,8 +27,6 @@ const char *reftable_error_str(int err)
 		return "misuse of the reftable API";
 	case REFTABLE_ZLIB_ERROR:
 		return "zlib failure";
-	case REFTABLE_NAME_CONFLICT:
-		return "file/directory conflict";
 	case REFTABLE_EMPTY_TABLE_ERROR:
 		return "wrote empty table";
 	case REFTABLE_REFNAME_ERROR:
diff --git a/reftable/refname.c b/reftable/refname.c
deleted file mode 100644
index bbfde15754..0000000000
--- a/reftable/refname.c
+++ /dev/null
@@ -1,206 +0,0 @@
-/*
-  Copyright 2020 Google LLC
-
-  Use of this source code is governed by a BSD-style
-  license that can be found in the LICENSE file or at
-  https://developers.google.com/open-source/licenses/bsd
-*/
-
-#include "system.h"
-#include "reftable-error.h"
-#include "basics.h"
-#include "refname.h"
-#include "reftable-iterator.h"
-
-struct refname_needle_lesseq_args {
-	char **haystack;
-	const char *needle;
-};
-
-static int refname_needle_lesseq(size_t k, void *_args)
-{
-	struct refname_needle_lesseq_args *args = _args;
-	return strcmp(args->needle, args->haystack[k]) <= 0;
-}
-
-static int modification_has_ref(struct modification *mod, const char *name)
-{
-	struct reftable_ref_record ref = { NULL };
-	int err = 0;
-
-	if (mod->add_len > 0) {
-		struct refname_needle_lesseq_args args = {
-			.haystack = mod->add,
-			.needle = name,
-		};
-		size_t idx = binsearch(mod->add_len, refname_needle_lesseq, &args);
-		if (idx < mod->add_len && !strcmp(mod->add[idx], name))
-			return 0;
-	}
-
-	if (mod->del_len > 0) {
-		struct refname_needle_lesseq_args args = {
-			.haystack = mod->del,
-			.needle = name,
-		};
-		size_t idx = binsearch(mod->del_len, refname_needle_lesseq, &args);
-		if (idx < mod->del_len && !strcmp(mod->del[idx], name))
-			return 1;
-	}
-
-	err = reftable_table_read_ref(&mod->tab, name, &ref);
-	reftable_ref_record_release(&ref);
-	return err;
-}
-
-static void modification_release(struct modification *mod)
-{
-	/* don't delete the strings themselves; they're owned by ref records.
-	 */
-	FREE_AND_NULL(mod->add);
-	FREE_AND_NULL(mod->del);
-	mod->add_len = 0;
-	mod->del_len = 0;
-}
-
-static int modification_has_ref_with_prefix(struct modification *mod,
-					    const char *prefix)
-{
-	struct reftable_iterator it = { NULL };
-	struct reftable_ref_record ref = { NULL };
-	int err = 0;
-
-	if (mod->add_len > 0) {
-		struct refname_needle_lesseq_args args = {
-			.haystack = mod->add,
-			.needle = prefix,
-		};
-		size_t idx = binsearch(mod->add_len, refname_needle_lesseq, &args);
-		if (idx < mod->add_len &&
-		    !strncmp(prefix, mod->add[idx], strlen(prefix)))
-			goto done;
-	}
-	err = reftable_table_seek_ref(&mod->tab, &it, prefix);
-	if (err)
-		goto done;
-
-	while (1) {
-		err = reftable_iterator_next_ref(&it, &ref);
-		if (err)
-			goto done;
-
-		if (mod->del_len > 0) {
-			struct refname_needle_lesseq_args args = {
-				.haystack = mod->del,
-				.needle = ref.refname,
-			};
-			size_t idx = binsearch(mod->del_len, refname_needle_lesseq, &args);
-			if (idx < mod->del_len &&
-			    !strcmp(ref.refname, mod->del[idx]))
-				continue;
-		}
-
-		if (strncmp(ref.refname, prefix, strlen(prefix))) {
-			err = 1;
-			goto done;
-		}
-		err = 0;
-		goto done;
-	}
-
-done:
-	reftable_ref_record_release(&ref);
-	reftable_iterator_destroy(&it);
-	return err;
-}
-
-static int validate_refname(const char *name)
-{
-	while (1) {
-		char *next = strchr(name, '/');
-		if (!*name) {
-			return REFTABLE_REFNAME_ERROR;
-		}
-		if (!next) {
-			return 0;
-		}
-		if (next - name == 0 || (next - name == 1 && *name == '.') ||
-		    (next - name == 2 && name[0] == '.' && name[1] == '.'))
-			return REFTABLE_REFNAME_ERROR;
-		name = next + 1;
-	}
-	return 0;
-}
-
-int validate_ref_record_addition(struct reftable_table tab,
-				 struct reftable_ref_record *recs, size_t sz)
-{
-	struct modification mod = {
-		.tab = tab,
-		.add = reftable_calloc(sz, sizeof(*mod.add)),
-		.del = reftable_calloc(sz, sizeof(*mod.del)),
-	};
-	int i = 0;
-	int err = 0;
-	for (; i < sz; i++) {
-		if (reftable_ref_record_is_deletion(&recs[i])) {
-			mod.del[mod.del_len++] = recs[i].refname;
-		} else {
-			mod.add[mod.add_len++] = recs[i].refname;
-		}
-	}
-
-	err = modification_validate(&mod);
-	modification_release(&mod);
-	return err;
-}
-
-static void strbuf_trim_component(struct strbuf *sl)
-{
-	while (sl->len > 0) {
-		int is_slash = (sl->buf[sl->len - 1] == '/');
-		strbuf_setlen(sl, sl->len - 1);
-		if (is_slash)
-			break;
-	}
-}
-
-int modification_validate(struct modification *mod)
-{
-	struct strbuf slashed = STRBUF_INIT;
-	int err = 0;
-	int i = 0;
-	for (; i < mod->add_len; i++) {
-		err = validate_refname(mod->add[i]);
-		if (err)
-			goto done;
-		strbuf_reset(&slashed);
-		strbuf_addstr(&slashed, mod->add[i]);
-		strbuf_addstr(&slashed, "/");
-
-		err = modification_has_ref_with_prefix(mod, slashed.buf);
-		if (err == 0) {
-			err = REFTABLE_NAME_CONFLICT;
-			goto done;
-		}
-		if (err < 0)
-			goto done;
-
-		strbuf_reset(&slashed);
-		strbuf_addstr(&slashed, mod->add[i]);
-		while (slashed.len) {
-			strbuf_trim_component(&slashed);
-			err = modification_has_ref(mod, slashed.buf);
-			if (err == 0) {
-				err = REFTABLE_NAME_CONFLICT;
-				goto done;
-			}
-			if (err < 0)
-				goto done;
-		}
-	}
-	err = 0;
-done:
-	strbuf_release(&slashed);
-	return err;
-}
diff --git a/reftable/refname.h b/reftable/refname.h
deleted file mode 100644
index a24b40fcb4..0000000000
--- a/reftable/refname.h
+++ /dev/null
@@ -1,29 +0,0 @@
-/*
-  Copyright 2020 Google LLC
-
-  Use of this source code is governed by a BSD-style
-  license that can be found in the LICENSE file or at
-  https://developers.google.com/open-source/licenses/bsd
-*/
-#ifndef REFNAME_H
-#define REFNAME_H
-
-#include "reftable-record.h"
-#include "reftable-generic.h"
-
-struct modification {
-	struct reftable_table tab;
-
-	char **add;
-	size_t add_len;
-
-	char **del;
-	size_t del_len;
-};
-
-int validate_ref_record_addition(struct reftable_table tab,
-				 struct reftable_ref_record *recs, size_t sz);
-
-int modification_validate(struct modification *mod);
-
-#endif
diff --git a/reftable/refname_test.c b/reftable/refname_test.c
deleted file mode 100644
index b9cc62554e..0000000000
--- a/reftable/refname_test.c
+++ /dev/null
@@ -1,101 +0,0 @@
-/*
-Copyright 2020 Google LLC
-
-Use of this source code is governed by a BSD-style
-license that can be found in the LICENSE file or at
-https://developers.google.com/open-source/licenses/bsd
-*/
-
-#include "basics.h"
-#include "block.h"
-#include "blocksource.h"
-#include "reader.h"
-#include "record.h"
-#include "refname.h"
-#include "reftable-error.h"
-#include "reftable-writer.h"
-#include "system.h"
-
-#include "test_framework.h"
-#include "reftable-tests.h"
-
-struct testcase {
-	char *add;
-	char *del;
-	int error_code;
-};
-
-static void test_conflict(void)
-{
-	struct reftable_write_options opts = { 0 };
-	struct strbuf buf = STRBUF_INIT;
-	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
-	struct reftable_ref_record rec = {
-		.refname = "a/b",
-		.value_type = REFTABLE_REF_SYMREF,
-		.value.symref = "destination", /* make sure it's not a symref.
-						*/
-		.update_index = 1,
-	};
-	int err;
-	int i;
-	struct reftable_block_source source = { NULL };
-	struct reftable_reader *rd = NULL;
-	struct reftable_table tab = { NULL };
-	struct testcase cases[] = {
-		{ "a/b/c", NULL, REFTABLE_NAME_CONFLICT },
-		{ "b", NULL, 0 },
-		{ "a", NULL, REFTABLE_NAME_CONFLICT },
-		{ "a", "a/b", 0 },
-
-		{ "p/", NULL, REFTABLE_REFNAME_ERROR },
-		{ "p//q", NULL, REFTABLE_REFNAME_ERROR },
-		{ "p/./q", NULL, REFTABLE_REFNAME_ERROR },
-		{ "p/../q", NULL, REFTABLE_REFNAME_ERROR },
-
-		{ "a/b/c", "a/b", 0 },
-		{ NULL, "a//b", 0 },
-	};
-	reftable_writer_set_limits(w, 1, 1);
-
-	err = reftable_writer_add_ref(w, &rec);
-	EXPECT_ERR(err);
-
-	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
-	reftable_writer_free(w);
-
-	block_source_from_strbuf(&source, &buf);
-	err = reftable_new_reader(&rd, &source, "filename");
-	EXPECT_ERR(err);
-
-	reftable_table_from_reader(&tab, rd);
-
-	for (i = 0; i < ARRAY_SIZE(cases); i++) {
-		struct modification mod = {
-			.tab = tab,
-		};
-
-		if (cases[i].add) {
-			mod.add = &cases[i].add;
-			mod.add_len = 1;
-		}
-		if (cases[i].del) {
-			mod.del = &cases[i].del;
-			mod.del_len = 1;
-		}
-
-		err = modification_validate(&mod);
-		EXPECT(err == cases[i].error_code);
-	}
-
-	reftable_reader_free(rd);
-	strbuf_release(&buf);
-}
-
-int refname_test_main(int argc, const char *argv[])
-{
-	RUN_TEST(test_conflict);
-	return 0;
-}
diff --git a/reftable/reftable-error.h b/reftable/reftable-error.h
index 4c457aaaf8..3a5f5b92c6 100644
--- a/reftable/reftable-error.h
+++ b/reftable/reftable-error.h
@@ -48,9 +48,6 @@ enum reftable_error {
 	/* Wrote a table without blocks. */
 	REFTABLE_EMPTY_TABLE_ERROR = -8,
 
-	/* Dir/file conflict. */
-	REFTABLE_NAME_CONFLICT = -9,
-
 	/* Invalid ref name. */
 	REFTABLE_REFNAME_ERROR = -10,
 
diff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h
index 0019cbcfa4..114cc3d053 100644
--- a/reftable/reftable-tests.h
+++ b/reftable/reftable-tests.h
@@ -14,7 +14,6 @@ int block_test_main(int argc, const char **argv);
 int merged_test_main(int argc, const char **argv);
 int pq_test_main(int argc, const char **argv);
 int record_test_main(int argc, const char **argv);
-int refname_test_main(int argc, const char **argv);
 int readwrite_test_main(int argc, const char **argv);
 int stack_test_main(int argc, const char **argv);
 int tree_test_main(int argc, const char **argv);
diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h
index 7c7cae5f99..3c119e2bbb 100644
--- a/reftable/reftable-writer.h
+++ b/reftable/reftable-writer.h
@@ -38,10 +38,6 @@ struct reftable_write_options {
 	/* 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;
-
 	/* boolean: copy log messages exactly. If unset, check that the message
 	 *   is a single line, and add '\n' if missing.
 	 */
diff --git a/reftable/stack.c b/reftable/stack.c
index 1ecf1b9751..e264df5ced 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -12,8 +12,8 @@ license that can be found in the LICENSE file or at
 #include "system.h"
 #include "merged.h"
 #include "reader.h"
-#include "refname.h"
 #include "reftable-error.h"
+#include "reftable-generic.h"
 #include "reftable-record.h"
 #include "reftable-merged.h"
 #include "writer.h"
@@ -27,8 +27,6 @@ static int stack_write_compact(struct reftable_stack *st,
 			       struct reftable_writer *wr,
 			       size_t first, size_t last,
 			       struct reftable_log_expiry_config *config);
-static int stack_check_addition(struct reftable_stack *st,
-				const char *new_tab_name);
 static void reftable_addition_close(struct reftable_addition *add);
 static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
 					     int reuse_open);
@@ -781,10 +779,6 @@ int reftable_addition_add(struct reftable_addition *add,
 		goto done;
 	}
 
-	err = stack_check_addition(add->stack, get_tempfile_path(tab_file));
-	if (err < 0)
-		goto done;
-
 	if (wr->min_update_index < add->next_update_index) {
 		err = REFTABLE_API_ERROR;
 		goto done;
@@ -1340,65 +1334,6 @@ int reftable_stack_read_log(struct reftable_stack *st, const char *refname,
 	return err;
 }
 
-static int stack_check_addition(struct reftable_stack *st,
-				const char *new_tab_name)
-{
-	int err = 0;
-	struct reftable_block_source src = { NULL };
-	struct reftable_reader *rd = NULL;
-	struct reftable_table tab = { NULL };
-	struct reftable_ref_record *refs = NULL;
-	struct reftable_iterator it = { NULL };
-	int cap = 0;
-	int len = 0;
-	int i = 0;
-
-	if (st->config.skip_name_check)
-		return 0;
-
-	err = reftable_block_source_from_file(&src, new_tab_name);
-	if (err < 0)
-		goto done;
-
-	err = reftable_new_reader(&rd, &src, new_tab_name);
-	if (err < 0)
-		goto done;
-
-	err = reftable_reader_seek_ref(rd, &it, "");
-	if (err > 0) {
-		err = 0;
-		goto done;
-	}
-	if (err < 0)
-		goto done;
-
-	while (1) {
-		struct reftable_ref_record ref = { NULL };
-		err = reftable_iterator_next_ref(&it, &ref);
-		if (err > 0)
-			break;
-		if (err < 0)
-			goto done;
-
-		REFTABLE_ALLOC_GROW(refs, len + 1, cap);
-		refs[len++] = ref;
-	}
-
-	reftable_table_from_merged_table(&tab, reftable_stack_merged_table(st));
-
-	err = validate_ref_record_addition(tab, refs, len);
-
-done:
-	for (i = 0; i < len; i++) {
-		reftable_ref_record_release(&refs[i]);
-	}
-
-	free(refs);
-	reftable_iterator_destroy(&it);
-	reftable_reader_free(rd);
-	return err;
-}
-
 static int is_table_name(const char *s)
 {
 	const char *dot = strrchr(s, '.');
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 0dc9a44648..b88097c3b6 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -353,44 +353,6 @@ static void test_reftable_stack_transaction_api_performs_auto_compaction(void)
 	clear_dir(dir);
 }
 
-static void test_reftable_stack_validate_refname(void)
-{
-	struct reftable_write_options cfg = { 0 };
-	struct reftable_stack *st = NULL;
-	int err;
-	char *dir = get_tmp_dir(__LINE__);
-
-	int i;
-	struct reftable_ref_record ref = {
-		.refname = "a/b",
-		.update_index = 1,
-		.value_type = REFTABLE_REF_SYMREF,
-		.value.symref = "master",
-	};
-	char *additions[] = { "a", "a/b/c" };
-
-	err = reftable_new_stack(&st, dir, cfg);
-	EXPECT_ERR(err);
-
-	err = reftable_stack_add(st, &write_test_ref, &ref);
-	EXPECT_ERR(err);
-
-	for (i = 0; i < ARRAY_SIZE(additions); i++) {
-		struct reftable_ref_record ref = {
-			.refname = additions[i],
-			.update_index = 1,
-			.value_type = REFTABLE_REF_SYMREF,
-			.value.symref = "master",
-		};
-
-		err = reftable_stack_add(st, &write_test_ref, &ref);
-		EXPECT(err == REFTABLE_NAME_CONFLICT);
-	}
-
-	reftable_stack_destroy(st);
-	clear_dir(dir);
-}
-
 static int write_error(struct reftable_writer *wr, void *arg)
 {
 	return *((int *)arg);
@@ -1097,7 +1059,6 @@ int stack_test_main(int argc, const char *argv[])
 	RUN_TEST(test_reftable_stack_transaction_api_performs_auto_compaction);
 	RUN_TEST(test_reftable_stack_update_index_check);
 	RUN_TEST(test_reftable_stack_uptodate);
-	RUN_TEST(test_reftable_stack_validate_refname);
 	RUN_TEST(test_sizes_to_segments);
 	RUN_TEST(test_sizes_to_segments_all_equal);
 	RUN_TEST(test_sizes_to_segments_empty);
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index 00237ef0d9..bae731669c 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -13,7 +13,6 @@ int cmd__reftable(int argc, const char **argv)
 	readwrite_test_main(argc, argv);
 	merged_test_main(argc, argv);
 	stack_test_main(argc, argv);
-	refname_test_main(argc, argv);
 	return 0;
 }
 
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]