diff mbox series

[v2,4/5] test-tool: test refspec input/output

Message ID bf296282323aa74f7c5b1a49c74d5aaa4b6f37ae.1617734870.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Maintenance: adapt custom refspecs | expand

Commit Message

Derrick Stolee April 6, 2021, 6:47 p.m. UTC 
From: Derrick Stolee <dstolee@microsoft.com>

Add a new test-helper, 'test-tool refspec', that currently reads stdin
line-by-line and translates the refspecs using the parsing logic of
refspec_item_init() and writes them to output.

Create a test in t5511-refspec.sh that uses this helper to test several
known special cases. This includes all of the special properties of the
'struct refspec_item', including:

 * force: The refspec starts with '+'.
 * pattern: Each side of the refspec has a glob character ('*')
 * matching: The refspec is simply the string ":".
 * exact_sha1: The 'src' string is a 40-character hex string.
 * negative: The refspec starts with '^' and 'dst' is NULL.

While the exact_sha1 property doesn't require special logic in
refspec_item_format, it is still tested here for completeness.

There is also the special-case refspec "@" which translates to "HEAD".

Note that if a refspec does not start with "refs/", then that is not
incorporated as part of the 'struct refspec_item'. This behavior is
confirmed by these tests. These refspecs still work in the wild because
the refs layer interprets them appropriately as branches, prepending
"refs/" or "refs/heads/" as necessary. I spent some time attempting to
insert these prefixes explicitly in parse_refspec(), but these are
several subtleties I was unable to overcome. If such a change were to be
made, then this new test in t5511-refspec.sh will need to be updated
with new output. For example, the input lines ending with "translated"
are designed to demonstrate these subtleties.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Makefile                |  1 +
 t/helper/test-refspec.c | 44 +++++++++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c    |  1 +
 t/helper/test-tool.h    |  1 +
 t/t5511-refspec.sh      | 41 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 88 insertions(+)
 create mode 100644 t/helper/test-refspec.c

Comments

Josh Steadmon April 7, 2021, 11:08 p.m. UTC | #1 
On 2021.04.06 18:47, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> Add a new test-helper, 'test-tool refspec', that currently reads stdin
> line-by-line and translates the refspecs using the parsing logic of
> refspec_item_init() and writes them to output.
> 
> Create a test in t5511-refspec.sh that uses this helper to test several
> known special cases. This includes all of the special properties of the
> 'struct refspec_item', including:
> 
>  * force: The refspec starts with '+'.
>  * pattern: Each side of the refspec has a glob character ('*')
>  * matching: The refspec is simply the string ":".
>  * exact_sha1: The 'src' string is a 40-character hex string.
>  * negative: The refspec starts with '^' and 'dst' is NULL.
> 
> While the exact_sha1 property doesn't require special logic in
> refspec_item_format, it is still tested here for completeness.
> 
> There is also the special-case refspec "@" which translates to "HEAD".
> 
> Note that if a refspec does not start with "refs/", then that is not
> incorporated as part of the 'struct refspec_item'. This behavior is
> confirmed by these tests. These refspecs still work in the wild because
> the refs layer interprets them appropriately as branches, prepending
> "refs/" or "refs/heads/" as necessary. I spent some time attempting to
> insert these prefixes explicitly in parse_refspec(), but these are
> several subtleties I was unable to overcome. If such a change were to be
> made, then this new test in t5511-refspec.sh will need to be updated
> with new output. For example, the input lines ending with "translated"
> are designed to demonstrate these subtleties.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Makefile                |  1 +
>  t/helper/test-refspec.c | 44 +++++++++++++++++++++++++++++++++++++++++
>  t/helper/test-tool.c    |  1 +
>  t/helper/test-tool.h    |  1 +
>  t/t5511-refspec.sh      | 41 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 88 insertions(+)
>  create mode 100644 t/helper/test-refspec.c
> 
> diff --git a/Makefile b/Makefile
> index a6a73c574191..f858c9f25976 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -734,6 +734,7 @@ TEST_BUILTINS_OBJS += test-reach.o
>  TEST_BUILTINS_OBJS += test-read-cache.o
>  TEST_BUILTINS_OBJS += test-read-graph.o
>  TEST_BUILTINS_OBJS += test-read-midx.o
> +TEST_BUILTINS_OBJS += test-refspec.o
>  TEST_BUILTINS_OBJS += test-ref-store.o
>  TEST_BUILTINS_OBJS += test-regex.o
>  TEST_BUILTINS_OBJS += test-repository.o
> diff --git a/t/helper/test-refspec.c b/t/helper/test-refspec.c
> new file mode 100644
> index 000000000000..b06735ded208
> --- /dev/null
> +++ b/t/helper/test-refspec.c
> @@ -0,0 +1,44 @@
> +#include "cache.h"
> +#include "parse-options.h"
> +#include "refspec.h"
> +#include "strbuf.h"
> +#include "test-tool.h"
> +
> +static const char * const refspec_usage[] = {
> +	N_("test-tool refspec [--fetch]"),
> +	NULL
> +};
> +
> +int cmd__refspec(int argc, const char **argv)
> +{
> +	struct strbuf line = STRBUF_INIT;
> +	int fetch = 0;
> +
> +	struct option refspec_options [] = {
> +		OPT_BOOL(0, "fetch", &fetch,
> +			 N_("enable the 'fetch' option for parsing refpecs")),

Typo here: s/refpecs/refspecs/


> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, NULL, refspec_options,
> +			     refspec_usage, 0);
> +
> +	while (strbuf_getline(&line, stdin) != EOF) {
> +		struct refspec_item rsi;
> +		char *buf;
> +
> +		if (!refspec_item_init(&rsi, line.buf, fetch)) {
> +			printf("failed to parse %s\n", line.buf);
> +			continue;
> +		}
> +
> +		buf = refspec_item_format(&rsi);
> +		printf("%s\n", buf);
> +		free(buf);
> +
> +		refspec_item_clear(&rsi);
> +	}
> +
> +	strbuf_release(&line);
> +	return 0;
> +}
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 287aa6002307..f534ad1731a9 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -55,6 +55,7 @@ static struct test_cmd cmds[] = {
>  	{ "read-cache", cmd__read_cache },
>  	{ "read-graph", cmd__read_graph },
>  	{ "read-midx", cmd__read_midx },
> +	{ "refspec", cmd__refspec },
>  	{ "ref-store", cmd__ref_store },
>  	{ "regex", cmd__regex },
>  	{ "repository", cmd__repository },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index 9ea4b31011dd..46a0b8850f17 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -44,6 +44,7 @@ int cmd__reach(int argc, const char **argv);
>  int cmd__read_cache(int argc, const char **argv);
>  int cmd__read_graph(int argc, const char **argv);
>  int cmd__read_midx(int argc, const char **argv);
> +int cmd__refspec(int argc, const char **argv);
>  int cmd__ref_store(int argc, const char **argv);
>  int cmd__regex(int argc, const char **argv);
>  int cmd__repository(int argc, const char **argv);
> diff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh
> index be025b90f989..489bec08d570 100755
> --- a/t/t5511-refspec.sh
> +++ b/t/t5511-refspec.sh
> @@ -93,4 +93,45 @@ test_refspec fetch "refs/heads/${good}"
>  bad=$(printf '\011tab')
>  test_refspec fetch "refs/heads/${bad}"				invalid
>  
> +test_expect_success 'test input/output round trip' '
> +	cat >input <<-\EOF &&
> +	+refs/heads/*:refs/remotes/origin/*
> +	refs/heads/*:refs/remotes/origin/*
> +	refs/heads/main:refs/remotes/frotz/xyzzy
> +	:refs/remotes/frotz/deleteme
> +	^refs/heads/secrets
> +	refs/heads/secret:refs/heads/translated
> +	refs/heads/secret:heads/translated
> +	refs/heads/secret:remotes/translated
> +	secret:translated
> +	refs/heads/*:remotes/xxy/*
> +	refs/heads*/for-linus:refs/remotes/mine/*
> +	2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48:refs/heads/fixed
> +	HEAD
> +	@
> +	:
> +	EOF
> +	cat >expect <<-\EOF &&
> +	+refs/heads/*:refs/remotes/origin/*
> +	refs/heads/*:refs/remotes/origin/*
> +	refs/heads/main:refs/remotes/frotz/xyzzy
> +	:refs/remotes/frotz/deleteme
> +	^refs/heads/secrets
> +	refs/heads/secret:refs/heads/translated
> +	refs/heads/secret:heads/translated
> +	refs/heads/secret:remotes/translated
> +	secret:translated
> +	refs/heads/*:remotes/xxy/*
> +	refs/heads*/for-linus:refs/remotes/mine/*
> +	2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48:refs/heads/fixed
> +	HEAD
> +	HEAD
> +	:
> +	EOF
> +	test-tool refspec <input >output &&
> +	test_cmp expect output &&
> +	test-tool refspec --fetch <input >output &&
> +	test_cmp expect output
> +'
> +
>  test_done
> -- 
> gitgitgadget
>
Emily Shaffer April 7, 2021, 11:26 p.m. UTC | #2 
On Tue, Apr 06, 2021 at 06:47:49PM +0000, Derrick Stolee via GitGitGadget wrote:
> diff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh
> index be025b90f989..489bec08d570 100755
> --- a/t/t5511-refspec.sh
> +++ b/t/t5511-refspec.sh
> @@ -93,4 +93,45 @@ test_refspec fetch "refs/heads/${good}"
>  bad=$(printf '\011tab')
>  test_refspec fetch "refs/heads/${bad}"				invalid
>  
> +test_expect_success 'test input/output round trip' '
> +	cat >input <<-\EOF &&
> +	+refs/heads/*:refs/remotes/origin/*
> +	refs/heads/*:refs/remotes/origin/*
> +	refs/heads/main:refs/remotes/frotz/xyzzy
> +	:refs/remotes/frotz/deleteme
> +	^refs/heads/secrets
> +	refs/heads/secret:refs/heads/translated
> +	refs/heads/secret:heads/translated
> +	refs/heads/secret:remotes/translated
> +	secret:translated
> +	refs/heads/*:remotes/xxy/*
> +	refs/heads*/for-linus:refs/remotes/mine/*
> +	2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48:refs/heads/fixed
> +	HEAD
> +	@
> +	:
> +	EOF
> +	cat >expect <<-\EOF &&
> +	+refs/heads/*:refs/remotes/origin/*
> +	refs/heads/*:refs/remotes/origin/*
> +	refs/heads/main:refs/remotes/frotz/xyzzy
> +	:refs/remotes/frotz/deleteme
> +	^refs/heads/secrets
> +	refs/heads/secret:refs/heads/translated
> +	refs/heads/secret:heads/translated
> +	refs/heads/secret:remotes/translated
> +	secret:translated
> +	refs/heads/*:remotes/xxy/*
> +	refs/heads*/for-linus:refs/remotes/mine/*
> +	2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48:refs/heads/fixed
> +	HEAD
> +	HEAD
> +	:
> +	EOF

I don't like these expect/actual here. They are almost entirely
identical, which means that the reader either A) spends a toilsome few
minutes checking every single line to be sure they are not identical, or
B) reads the first three lines, decides they're the same, and misses the
@->HEAD special case.

Why not instead run the test once for all the lines which should be the
same before and after the parse, and again for all the lines which
should differ, to reduce burden on the reader?

 - Emily
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index a6a73c574191..f858c9f25976 100644
--- a/Makefile
+++ b/Makefile
@@ -734,6 +734,7 @@  TEST_BUILTINS_OBJS += test-reach.o
 TEST_BUILTINS_OBJS += test-read-cache.o
 TEST_BUILTINS_OBJS += test-read-graph.o
 TEST_BUILTINS_OBJS += test-read-midx.o
+TEST_BUILTINS_OBJS += test-refspec.o
 TEST_BUILTINS_OBJS += test-ref-store.o
 TEST_BUILTINS_OBJS += test-regex.o
 TEST_BUILTINS_OBJS += test-repository.o
diff --git a/t/helper/test-refspec.c b/t/helper/test-refspec.c
new file mode 100644
index 000000000000..b06735ded208
--- /dev/null
+++ b/t/helper/test-refspec.c
@@ -0,0 +1,44 @@ 
+#include "cache.h"
+#include "parse-options.h"
+#include "refspec.h"
+#include "strbuf.h"
+#include "test-tool.h"
+
+static const char * const refspec_usage[] = {
+	N_("test-tool refspec [--fetch]"),
+	NULL
+};
+
+int cmd__refspec(int argc, const char **argv)
+{
+	struct strbuf line = STRBUF_INIT;
+	int fetch = 0;
+
+	struct option refspec_options [] = {
+		OPT_BOOL(0, "fetch", &fetch,
+			 N_("enable the 'fetch' option for parsing refpecs")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, NULL, refspec_options,
+			     refspec_usage, 0);
+
+	while (strbuf_getline(&line, stdin) != EOF) {
+		struct refspec_item rsi;
+		char *buf;
+
+		if (!refspec_item_init(&rsi, line.buf, fetch)) {
+			printf("failed to parse %s\n", line.buf);
+			continue;
+		}
+
+		buf = refspec_item_format(&rsi);
+		printf("%s\n", buf);
+		free(buf);
+
+		refspec_item_clear(&rsi);
+	}
+
+	strbuf_release(&line);
+	return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 287aa6002307..f534ad1731a9 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -55,6 +55,7 @@  static struct test_cmd cmds[] = {
 	{ "read-cache", cmd__read_cache },
 	{ "read-graph", cmd__read_graph },
 	{ "read-midx", cmd__read_midx },
+	{ "refspec", cmd__refspec },
 	{ "ref-store", cmd__ref_store },
 	{ "regex", cmd__regex },
 	{ "repository", cmd__repository },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 9ea4b31011dd..46a0b8850f17 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -44,6 +44,7 @@  int cmd__reach(int argc, const char **argv);
 int cmd__read_cache(int argc, const char **argv);
 int cmd__read_graph(int argc, const char **argv);
 int cmd__read_midx(int argc, const char **argv);
+int cmd__refspec(int argc, const char **argv);
 int cmd__ref_store(int argc, const char **argv);
 int cmd__regex(int argc, const char **argv);
 int cmd__repository(int argc, const char **argv);
diff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh
index be025b90f989..489bec08d570 100755
--- a/t/t5511-refspec.sh
+++ b/t/t5511-refspec.sh
@@ -93,4 +93,45 @@  test_refspec fetch "refs/heads/${good}"
 bad=$(printf '\011tab')
 test_refspec fetch "refs/heads/${bad}"				invalid
 
+test_expect_success 'test input/output round trip' '
+	cat >input <<-\EOF &&
+	+refs/heads/*:refs/remotes/origin/*
+	refs/heads/*:refs/remotes/origin/*
+	refs/heads/main:refs/remotes/frotz/xyzzy
+	:refs/remotes/frotz/deleteme
+	^refs/heads/secrets
+	refs/heads/secret:refs/heads/translated
+	refs/heads/secret:heads/translated
+	refs/heads/secret:remotes/translated
+	secret:translated
+	refs/heads/*:remotes/xxy/*
+	refs/heads*/for-linus:refs/remotes/mine/*
+	2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48:refs/heads/fixed
+	HEAD
+	@
+	:
+	EOF
+	cat >expect <<-\EOF &&
+	+refs/heads/*:refs/remotes/origin/*
+	refs/heads/*:refs/remotes/origin/*
+	refs/heads/main:refs/remotes/frotz/xyzzy
+	:refs/remotes/frotz/deleteme
+	^refs/heads/secrets
+	refs/heads/secret:refs/heads/translated
+	refs/heads/secret:heads/translated
+	refs/heads/secret:remotes/translated
+	secret:translated
+	refs/heads/*:remotes/xxy/*
+	refs/heads*/for-linus:refs/remotes/mine/*
+	2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48:refs/heads/fixed
+	HEAD
+	HEAD
+	:
+	EOF
+	test-tool refspec <input >output &&
+	test_cmp expect output &&
+	test-tool refspec --fetch <input >output &&
+	test_cmp expect output
+'
+
 test_done