[PATCH 0/1] Move 'git commit-graph read' subcommand to test helper

[PATCH 0/1] Move 'git commit-graph read' subcommand to test helper

[Derrick Stolee via GitGitGadget]

As discussed [1], the 'git commit-graph read' subcommand has little value to
users and should instead live in a test helper. The implementation was able
to move almost wholesale, with only some new code to find the default object
directory. This was necessary because of some tests that use a bare
repository.

Thanks, -Stolee

[1] 
https://public-inbox.org/git/CAGyf7-G3NDp--2nUbri_0EqvSLF21M0gsFCOKDCWMY+e68Htog@mail.gmail.com/

Derrick Stolee (1):
  test-tool: use 'read-graph' helper

 Documentation/git-commit-graph.txt | 12 ------
 Makefile                           |  1 +
 builtin/commit-graph.c             | 68 ------------------------------
 t/helper/test-read-graph.c         | 53 +++++++++++++++++++++++
 t/helper/test-tool.c               |  1 +
 t/helper/test-tool.h               |  1 +
 t/t5318-commit-graph.sh            |  2 +-
 t/t5324-split-commit-graph.sh      |  2 +-
 8 files changed, 58 insertions(+), 82 deletions(-)
 create mode 100644 t/helper/test-read-graph.c


base-commit: d9f6f3b6195a0ca35642561e530798ad1469bd41
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-460%2Fderrickstolee%2Fcommit-graph-read-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-460/derrickstolee/commit-graph-read-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/460
-- 
gitgitgadget

[PATCH 1/1] test-tool: use 'read-graph' helper

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The 'git commit-graph read' subcommand is used in test scripts to check
that the commit-graph contents match the expected data. Mostly, this
helps check the header information and the list of chunks. Users do not
need this information, so move the functionality to a test helper.

Reported-by: Bryan Turner <bturner@atlassian.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/git-commit-graph.txt | 12 ------
 Makefile                           |  1 +
 builtin/commit-graph.c             | 68 ------------------------------
 t/helper/test-read-graph.c         | 53 +++++++++++++++++++++++
 t/helper/test-tool.c               |  1 +
 t/helper/test-tool.h               |  1 +
 t/t5318-commit-graph.sh            |  2 +-
 t/t5324-split-commit-graph.sh      |  2 +-
 8 files changed, 58 insertions(+), 82 deletions(-)
 create mode 100644 t/helper/test-read-graph.c

diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
index 8c708a7a16..bcd85c1976 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -9,7 +9,6 @@ git-commit-graph - Write and verify Git commit-graph files
 SYNOPSIS
 --------
 [verse]
-'git commit-graph read' [--object-dir <dir>]
 'git commit-graph verify' [--object-dir <dir>] [--shallow] [--[no-]progress]
 'git commit-graph write' <options> [--object-dir <dir>] [--[no-]progress]
 
@@ -74,11 +73,6 @@ Finally, if `--expire-time=<datetime>` is not specified, let `datetime`
 be the current time. After writing the split commit-graph, delete all
 unused commit-graph whose modified times are older than `datetime`.
 
-'read'::
-
-Read the commit-graph file and output basic details about it.
-Used for debugging purposes.
-
 'verify'::
 
 Read the commit-graph file and verify its contents against the object
@@ -118,12 +112,6 @@ $ git show-ref -s | git commit-graph write --stdin-commits
 $ git rev-parse HEAD | git commit-graph write --stdin-commits --append
 ------------------------------------------------
 
-* Read basic information from the commit-graph file.
-+
-------------------------------------------------
-$ git commit-graph read
-------------------------------------------------
-
 
 GIT
 ---
diff --git a/Makefile b/Makefile
index 58b92af54b..44997f6f57 100644
--- a/Makefile
+++ b/Makefile
@@ -727,6 +727,7 @@ TEST_BUILTINS_OBJS += test-prio-queue.o
 TEST_BUILTINS_OBJS += test-progress.o
 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-ref-store.o
 TEST_BUILTINS_OBJS += test-regex.o
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index ec0fc93d39..e0c6fc4bbf 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -8,7 +8,6 @@
 #include "object-store.h"
 
 static char const * const builtin_commit_graph_usage[] = {
-	N_("git commit-graph read [--object-dir <objdir>]"),
 	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
 	N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] <split options>"),
 	NULL
@@ -19,11 +18,6 @@ static const char * const builtin_commit_graph_verify_usage[] = {
 	NULL
 };
 
-static const char * const builtin_commit_graph_read_usage[] = {
-	N_("git commit-graph read [--object-dir <objdir>]"),
-	NULL
-};
-
 static const char * const builtin_commit_graph_write_usage[] = {
 	N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] <split options>"),
 	NULL
@@ -93,66 +87,6 @@ static int graph_verify(int argc, const char **argv)
 	return verify_commit_graph(the_repository, graph, flags);
 }
 
-static int graph_read(int argc, const char **argv)
-{
-	struct commit_graph *graph = NULL;
-	char *graph_name;
-	int open_ok;
-	int fd;
-	struct stat st;
-
-	static struct option builtin_commit_graph_read_options[] = {
-		OPT_STRING(0, "object-dir", &opts.obj_dir,
-			N_("dir"),
-			N_("The object directory to store the graph")),
-		OPT_END(),
-	};
-
-	trace2_cmd_mode("read");
-
-	argc = parse_options(argc, argv, NULL,
-			     builtin_commit_graph_read_options,
-			     builtin_commit_graph_read_usage, 0);
-
-	if (!opts.obj_dir)
-		opts.obj_dir = get_object_directory();
-
-	graph_name = get_commit_graph_filename(opts.obj_dir);
-
-	open_ok = open_commit_graph(graph_name, &fd, &st);
-	if (!open_ok)
-		die_errno(_("Could not open commit-graph '%s'"), graph_name);
-
-	graph = load_commit_graph_one_fd_st(fd, &st);
-	if (!graph)
-		return 1;
-
-	FREE_AND_NULL(graph_name);
-
-	printf("header: %08x %d %d %d %d\n",
-		ntohl(*(uint32_t*)graph->data),
-		*(unsigned char*)(graph->data + 4),
-		*(unsigned char*)(graph->data + 5),
-		*(unsigned char*)(graph->data + 6),
-		*(unsigned char*)(graph->data + 7));
-	printf("num_commits: %u\n", graph->num_commits);
-	printf("chunks:");
-
-	if (graph->chunk_oid_fanout)
-		printf(" oid_fanout");
-	if (graph->chunk_oid_lookup)
-		printf(" oid_lookup");
-	if (graph->chunk_commit_data)
-		printf(" commit_metadata");
-	if (graph->chunk_extra_edges)
-		printf(" extra_edges");
-	printf("\n");
-
-	UNLEAK(graph);
-
-	return 0;
-}
-
 extern int read_replace_refs;
 static struct split_commit_graph_opts split_opts;
 
@@ -268,8 +202,6 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)
 	save_commit_buffer = 0;
 
 	if (argc > 0) {
-		if (!strcmp(argv[0], "read"))
-			return graph_read(argc, argv);
 		if (!strcmp(argv[0], "verify"))
 			return graph_verify(argc, argv);
 		if (!strcmp(argv[0], "write"))
diff --git a/t/helper/test-read-graph.c b/t/helper/test-read-graph.c
new file mode 100644
index 0000000000..d2884efe0a
--- /dev/null
+++ b/t/helper/test-read-graph.c
@@ -0,0 +1,53 @@
+#include "test-tool.h"
+#include "cache.h"
+#include "commit-graph.h"
+#include "repository.h"
+#include "object-store.h"
+
+int cmd__read_graph(int argc, const char **argv)
+{
+	struct commit_graph *graph = NULL;
+	char *graph_name;
+	int open_ok;
+	int fd;
+	struct stat st;
+	const char *object_dir;
+
+	setup_git_directory();
+	object_dir = get_object_directory();
+
+	graph_name = get_commit_graph_filename(object_dir);
+
+	open_ok = open_commit_graph(graph_name, &fd, &st);
+	if (!open_ok)
+		die_errno(_("Could not open commit-graph '%s'"), graph_name);
+
+	graph = load_commit_graph_one_fd_st(fd, &st);
+	if (!graph)
+		return 1;
+
+	FREE_AND_NULL(graph_name);
+
+	printf("header: %08x %d %d %d %d\n",
+		ntohl(*(uint32_t*)graph->data),
+		*(unsigned char*)(graph->data + 4),
+		*(unsigned char*)(graph->data + 5),
+		*(unsigned char*)(graph->data + 6),
+		*(unsigned char*)(graph->data + 7));
+	printf("num_commits: %u\n", graph->num_commits);
+	printf("chunks:");
+
+	if (graph->chunk_oid_fanout)
+		printf(" oid_fanout");
+	if (graph->chunk_oid_lookup)
+		printf(" oid_lookup");
+	if (graph->chunk_commit_data)
+		printf(" commit_metadata");
+	if (graph->chunk_extra_edges)
+		printf(" extra_edges");
+	printf("\n");
+
+	UNLEAK(graph);
+
+	return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 19ee26d931..f20989d449 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -45,6 +45,7 @@ static struct test_cmd cmds[] = {
 	{ "progress", cmd__progress },
 	{ "reach", cmd__reach },
 	{ "read-cache", cmd__read_cache },
+	{ "read-graph", cmd__read_graph },
 	{ "read-midx", cmd__read_midx },
 	{ "ref-store", cmd__ref_store },
 	{ "regex", cmd__regex },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index c2aa56ef50..8ed2af71d1 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -35,6 +35,7 @@ int cmd__prio_queue(int argc, const char **argv);
 int cmd__progress(int argc, const char **argv);
 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__ref_store(int argc, const char **argv);
 int cmd__regex(int argc, const char **argv);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index d42b3efe39..798968374f 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -85,7 +85,7 @@ graph_read_expect() {
 	num_commits: $1
 	chunks: oid_fanout oid_lookup commit_metadata$OPTIONAL
 	EOF
-	git commit-graph read >output &&
+	test-tool read-graph >output &&
 	test_cmp expect output
 }
 
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 115aabd141..c24823431f 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -25,7 +25,7 @@ graph_read_expect() {
 	num_commits: $1
 	chunks: oid_fanout oid_lookup commit_metadata
 	EOF
-	git commit-graph read >output &&
+	test-tool read-graph >output &&
 	test_cmp expect output
 }
 
-- 
gitgitgadget

Re: [PATCH 1/1] test-tool: use 'read-graph' helper

[Jeff King]

On Tue, Nov 12, 2019 at 04:58:20PM +0000, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <dstolee@microsoft.com>
> 
> The 'git commit-graph read' subcommand is used in test scripts to check
> that the commit-graph contents match the expected data. Mostly, this
> helps check the header information and the list of chunks. Users do not
> need this information, so move the functionality to a test helper.

This looks good to me.

>  t/helper/test-read-graph.c         | 53 +++++++++++++++++++++++

I wondered if we might want a commit-graph test-tool with sub-commands.
But it probably doesn't matter. This would be the only thing in it. And
there's less need these days to keep the number of tools down, since
they're all linked into the same binary. And we can always change it
later, since it's not a public interface.

So this is the simplest thing, which is probably best for now.

-Peff