[v3,2/8] test-ref-store: parse symbolic flag constants

Commit Message

Han-Wen Nienhuys Dec. 2, 2021, 6:39 p.m. UTC

From: Han-Wen Nienhuys <[e-mail]>

This lets tests use REF_XXXX constants instead of hardcoded integers. The flag
names should be separated by a ','.

Signed-off-by: Han-Wen Nienhuys <[e-mail]>
---
 t/helper/test-ref-store.c | 69 +++++++++++++++++++++++++++++++++++----
 t/t1405-main-ref-store.sh |  3 +-
 2 files changed, 63 insertions(+), 9 deletions(-)

Comments

Jeff King Dec. 3, 2021, 6:22 a.m. UTC | #1

On Thu, Dec 02, 2021 at 06:39:56PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:

> From: Han-Wen Nienhuys <[e-mail]>
> 
> This lets tests use REF_XXXX constants instead of hardcoded integers. The flag
> names should be separated by a ','.

So much nicer. Thank you for cleaning this up.

One small bug:

> +static unsigned int parse_flags(const char *str, struct flag_definition *defs)
> +{
> +	struct string_list masks = STRING_LIST_INIT_DUP;
> +	int i = 0;
> +	unsigned int result = 0;
> +
> +	if (!strcmp(str, "0"))
> +		return 0;
> +
> +	string_list_split(&masks, str, ',', 64);
> +	for (; i < masks.nr; i++) {
> +		const char *name = masks.items[i].string;
> +		struct flag_definition *def = defs;
> +		int found = 0;
> +		while (def->name) {
> +			if (!strcmp(def->name, name)) {
> +				result |= def->mask;
> +				found = 1;
> +				break;
> +			}
> +			def++;
> +		}

We assume defs ends with a NULL entry here...

> +static struct flag_definition empty_flags[] = {
> +	{ NULL, 0 },
> +};

...and this one does so...

> +static struct flag_definition pack_flags[] = {
> +	FLAG_DEF(PACK_REFS_PRUNE),
> +	FLAG_DEF(PACK_REFS_ALL),
> +};

...but this one does not. So passing an unknown flag will result in us
walking off the end of the list.

> +static struct flag_definition transaction_flags[] = {
> +	FLAG_DEF(REF_NO_DEREF),
> +	FLAG_DEF(REF_FORCE_CREATE_REFLOG),
> +	{ NULL, 0 },
> +};

This one is good. We might want to drop the trailing comma on the NULL
entries, to make it more clear that they have to come last.

The other option would be using ARRAY_SIZE() instead of a NULL
terminator, but of course that has to happen in the caller. Which means
either extra work there, or yet another macro. :)

-Peff

Patch

diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index b795a56eedf..cbf1b5f506d 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -5,6 +5,50 @@ 
 #include "object-store.h"
 #include "repository.h"
 
+struct flag_definition {
+	const char *name;
+	uint64_t mask;
+};
+
+#define FLAG_DEF(x)     \
+	{               \
+#x, (x) \
+	}
+
+static unsigned int parse_flags(const char *str, struct flag_definition *defs)
+{
+	struct string_list masks = STRING_LIST_INIT_DUP;
+	int i = 0;
+	unsigned int result = 0;
+
+	if (!strcmp(str, "0"))
+		return 0;
+
+	string_list_split(&masks, str, ',', 64);
+	for (; i < masks.nr; i++) {
+		const char *name = masks.items[i].string;
+		struct flag_definition *def = defs;
+		int found = 0;
+		while (def->name) {
+			if (!strcmp(def->name, name)) {
+				result |= def->mask;
+				found = 1;
+				break;
+			}
+			def++;
+		}
+		if (!found)
+			die("unknown flag \"%s\"", name);
+	}
+
+	string_list_clear(&masks, 0);
+	return result;
+}
+
+static struct flag_definition empty_flags[] = {
+	{ NULL, 0 },
+};
+
 static const char *notnull(const char *arg, const char *name)
 {
 	if (!arg)
@@ -12,9 +56,10 @@  static const char *notnull(const char *arg, const char *name)
 	return arg;
 }
 
-static unsigned int arg_flags(const char *arg, const char *name)
+static unsigned int arg_flags(const char *arg, const char *name,
+			      struct flag_definition *defs)
 {
-	return atoi(notnull(arg, name));
+	return parse_flags(notnull(arg, name), defs);
 }
 
 static const char **get_store(const char **argv, struct ref_store **refs)
@@ -64,10 +109,14 @@  static const char **get_store(const char **argv, struct ref_store **refs)
 	return argv + 1;
 }
 
+static struct flag_definition pack_flags[] = {
+	FLAG_DEF(PACK_REFS_PRUNE),
+	FLAG_DEF(PACK_REFS_ALL),
+};
 
 static int cmd_pack_refs(struct ref_store *refs, const char **argv)
 {
-	unsigned int flags = arg_flags(*argv++, "flags");
+	unsigned int flags = arg_flags(*argv++, "flags", pack_flags);
 
 	return refs_pack_refs(refs, flags);
 }
@@ -81,9 +130,15 @@  static int cmd_create_symref(struct ref_store *refs, const char **argv)
 	return refs_create_symref(refs, refname, target, logmsg);
 }
 
+static struct flag_definition transaction_flags[] = {
+	FLAG_DEF(REF_NO_DEREF),
+	FLAG_DEF(REF_FORCE_CREATE_REFLOG),
+	{ NULL, 0 },
+};
+
 static int cmd_delete_refs(struct ref_store *refs, const char **argv)
 {
-	unsigned int flags = arg_flags(*argv++, "flags");
+	unsigned int flags = arg_flags(*argv++, "flags", transaction_flags);
 	const char *msg = *argv++;
 	struct string_list refnames = STRING_LIST_INIT_NODUP;
 
@@ -120,7 +175,7 @@  static int cmd_resolve_ref(struct ref_store *refs, const char **argv)
 {
 	struct object_id oid = *null_oid();
 	const char *refname = notnull(*argv++, "refname");
-	int resolve_flags = arg_flags(*argv++, "resolve-flags");
+	int resolve_flags = arg_flags(*argv++, "resolve-flags", empty_flags);
 	int flags;
 	const char *ref;
 	int ignore_errno;
@@ -209,7 +264,7 @@  static int cmd_delete_ref(struct ref_store *refs, const char **argv)
 	const char *msg = notnull(*argv++, "msg");
 	const char *refname = notnull(*argv++, "refname");
 	const char *sha1_buf = notnull(*argv++, "old-sha1");
-	unsigned int flags = arg_flags(*argv++, "flags");
+	unsigned int flags = arg_flags(*argv++, "flags", transaction_flags);
 	struct object_id old_oid;
 
 	if (get_oid_hex(sha1_buf, &old_oid))
@@ -224,7 +279,7 @@  static int cmd_update_ref(struct ref_store *refs, const char **argv)
 	const char *refname = notnull(*argv++, "refname");
 	const char *new_sha1_buf = notnull(*argv++, "new-sha1");
 	const char *old_sha1_buf = notnull(*argv++, "old-sha1");
-	unsigned int flags = arg_flags(*argv++, "flags");
+	unsigned int flags = arg_flags(*argv++, "flags", transaction_flags);
 	struct object_id old_oid;
 	struct object_id new_oid;
 
diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
index 5e0f7073286..63e0ae82bdf 100755
--- a/t/t1405-main-ref-store.sh
+++ b/t/t1405-main-ref-store.sh
@@ -17,8 +17,7 @@  test_expect_success 'setup' '
 test_expect_success REFFILES 'pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE)' '
 	N=`find .git/refs -type f | wc -l` &&
 	test "$N" != 0 &&
-	ALL_OR_PRUNE_FLAG=3 &&
-	$RUN pack-refs ${ALL_OR_PRUNE_FLAG} &&
+	$RUN pack-refs PACK_REFS_PRUNE,PACK_REFS_ALL &&
 	N=`find .git/refs -type f` &&
 	test -z "$N"
 '