git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v3 06/20] commit-graph: add 'verify' subcommand

From
Jakub Narebski <jnareb@gmail.com>
Date
May 27, 2018, 22:55 UTC
Message-ID
<86y3g4yahm.fsf@gmail.com>
In-Reply-To
<20180524162504.158394-7-dstolee@microsoft.com>
Derrick Stolee <dstolee@microsoft.com> writes:
Show 9 quoted lines
> If the commit-graph file becomes corrupt, we need a way to verify
> that its contents match the object database. In the manner of
> 'git fsck' we will implement a 'git commit-graph verify' subcommand
> to report all issues with the file.
>
> Add the 'verify' subcommand to the 'commit-graph' builtin and its
> documentation. The subcommand is currently a no-op except for
> loading the commit-graph into memory, which may trigger run-time
> errors that would be caught by normal use.

So this commit is simply getting the boilerplate out of the way for implementing 'git commit-graph verify' subcommand. Good.

>                                            Add a simple test that
> ensures the command returns a zero error code.
Nice.
>
> If no commit-graph file exists, this is an acceptable state. Do
> not report any errors.

All right. I assume that as it is explicit verification call, it does ignore core.commitGraph setting, isn't it?

Show 20 quoted lines
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/git-commit-graph.txt |  6 ++++++
>  builtin/commit-graph.c             | 38 ++++++++++++++++++++++++++++++++++++++
>  commit-graph.c                     | 26 ++++++++++++++++++++++++++
>  commit-graph.h                     |  2 ++
>  t/t5318-commit-graph.sh            | 10 ++++++++++
>  5 files changed, 82 insertions(+)
>
> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
> index 4c97b555cc..a222cfab08 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -10,6 +10,7 @@ SYNOPSIS
>  --------
>  [verse]
>  'git commit-graph read' [--object-dir <dir>]
> +'git commit-graph verify' [--object-dir <dir>]
>  'git commit-graph write' <options> [--object-dir <dir>]
In alphabetical order, good.
Show 11 quoted lines
>  
>  
> @@ -52,6 +53,11 @@ existing commit-graph file.
>  Read a graph file given by the commit-graph file and output basic
>  details about the graph file. Used for debugging purposes.
>  
> +'verify'::
> +
> +Read the commit-graph file and verify its contents against the object
> +database. Used to check for corrupted data.
> +
All right, good enough description.
Show 15 quoted lines
>  
>  EXAMPLES
>  --------
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index f0875b8bf3..0433dd6e20 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -8,10 +8,16 @@
>  static char const * const builtin_commit_graph_usage[] = {
>  	N_("git commit-graph [--object-dir <objdir>]"),
>  	N_("git commit-graph read [--object-dir <objdir>]"),
> +	N_("git commit-graph verify [--object-dir <objdir>]"),
>  	N_("git commit-graph write [--object-dir <objdir>] [--append] [--stdin-packs|--stdin-commits]"),
>  	NULL
>  };
In alphabetical order, same as in the manpage for git-commit-graph.
Show 32 quoted lines
>  
> +static const char * const builtin_commit_graph_verify_usage[] = {
> +	N_("git commit-graph verify [--object-dir <objdir>]"),
> +	NULL
> +};
> +
>  static const char * const builtin_commit_graph_read_usage[] = {
>  	N_("git commit-graph read [--object-dir <objdir>]"),
>  	NULL
> @@ -29,6 +35,36 @@ static struct opts_commit_graph {
>  	int append;
>  } opts;
>  
> +
> +static int graph_verify(int argc, const char **argv)
> +{
> +	struct commit_graph *graph = 0;
> +	char *graph_name;
> +
> +	static struct option builtin_commit_graph_verify_options[] = {
> +		OPT_STRING(0, "object-dir", &opts.obj_dir,
> +			   N_("dir"),
> +			   N_("The object directory to store the graph")),
> +		OPT_END(),
> +	};
> +
> +	argc = parse_options(argc, argv, NULL,
> +			     builtin_commit_graph_verify_options,
> +			     builtin_commit_graph_verify_usage, 0);
> +
> +	if (!opts.obj_dir)
> +		opts.obj_dir = get_object_directory();

Getting the boilerplate of implementing the command mostly out of the way. Good.

> +
> +	graph_name = get_commit_graph_filename(opts.obj_dir);
> +	graph = load_commit_graph_one(graph_name);

So we are verifying only the commit-graph file belonging directly to current repository, as I have expected. This is needed to for warnings and error messages from the 'verify' action, to be able to tell in which file there are problems.

This means that it is possible that there would be problems with commit-graph files that running 'git commit-graph verify' would not find, because they are in commit-graph file in one of the alternates.

It is very easy, though, to check all commit-graph files that would be read and its data concatenated when using commit-graph feature (e.g. 'git commit-graph read', IIRC):

  $ git commit-graph verify
  $ for obj_dir in $(cat .git/objects/info/alternates) do;
        git commit-graph --object-dir="$obj_dir";
    done
Note: I have not checked the above that it works.
> +	FREE_AND_NULL(graph_name);
Freeing the resources, always nice to have.
> +
> +	if (!graph)
> +		return 0;

DS> If no commit-graph file exists, this is an acceptable state. Do DS> not report any errors.

Right, non existant commit-graph file is certainly valid ;-)
> +
> +	return verify_commit_graph(graph);
> +}

I guess that graph_verify() would not change much, if at all, in subsequent commits in this patch series.

Show 13 quoted lines
> +
>  static int graph_read(int argc, const char **argv)
>  {
>  	struct commit_graph *graph = NULL;
> @@ -163,6 +199,8 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)
>  			     PARSE_OPT_STOP_AT_NON_OPTION);
>  
>  	if (argc > 0) {
> +		if (!strcmp(argv[0], "verify"))
> +			return graph_verify(argc, argv);
>  		if (!strcmp(argv[0], "read"))
>  			return graph_read(argc, argv);
>  		if (!strcmp(argv[0], "write"))
Not in alphabetical order... is there a reason for that?
Show 23 quoted lines
> diff --git a/commit-graph.c b/commit-graph.c
> index 25893ec096..55b41664ee 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -836,3 +836,29 @@ void write_commit_graph(const char *obj_dir,
>  	oids.alloc = 0;
>  	oids.nr = 0;
>  }
> +
> +static int verify_commit_graph_error;
> +
> +static void graph_report(const char *fmt, ...)
> +{
> +	va_list ap;
> +	struct strbuf sb = STRBUF_INIT;
> +	verify_commit_graph_error = 1;
> +
> +	va_start(ap, fmt);
> +	strbuf_vaddf(&sb, fmt, ap);
> +
> +	fprintf(stderr, "%s\n", sb.buf);
> +	strbuf_release(&sb);
> +	va_end(ap);

Why do you use strbuf_vaddf + fprintf instead of straighforward vfprintf (or function instead of variable-level macro)?

Is it because of [string] safety?
Show 8 quoted lines
> +}
> +
> +int verify_commit_graph(struct commit_graph *g)
> +{
> +	if (!g) {
> +		graph_report("no commit-graph file loaded");
> +		return 1;
> +	}

All right, this is just a placeholder - we should not ever get this message because in this case we exit with error code of 0 (EXIT_SUCCESS) if there is no commit-graph file loaded before invoking verify_commit_graph().

> +
> +	return verify_commit_graph_error;
All right, this is for the future.  Good.
Show 11 quoted lines
> +}
> diff --git a/commit-graph.h b/commit-graph.h
> index 96cccb10f3..71a39c5a57 100644
> --- a/commit-graph.h
> +++ b/commit-graph.h
> @@ -53,4 +53,6 @@ void write_commit_graph(const char *obj_dir,
>  			int nr_commits,
>  			int append);
>  
> +int verify_commit_graph(struct commit_graph *g);
> +

Why does this need to be exported? I think it is not used outside of commit-graph.c, isn't it?

Show 11 quoted lines
>  #endif
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 77d85aefe7..6ca451dfd2 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -11,6 +11,11 @@ test_expect_success 'setup full repo' '
>  	objdir=".git/objects"
>  '
>  
> +test_expect_success 'verify graph with no graph file' '
> +	cd "$TRASH_DIRECTORY/full" &&

Is sich bare `cd`, without corresponding `cd` back or using subshell safe?

Show 14 quoted lines
> +	git commit-graph verify
> +'
> +
>  test_expect_success 'write graph with no packs' '
>  	cd "$TRASH_DIRECTORY/full" &&
>  	git commit-graph write --object-dir . &&
> @@ -230,4 +235,9 @@ test_expect_success 'perform fast-forward merge in full repo' '
>  	test_cmp expect output
>  '
>  
> +test_expect_success 'git commit-graph verify' '
> +	cd "$TRASH_DIRECTORY/full" &&
> +	git commit-graph verify >output
> +'

Those are tests with nearly the same code, but they are (by their descriptions) testing different things. This means that they rely on side effects of earlier tests.

This is suboptimal, as it means that it would be impossible or very difficult to run individual tests (e.g. with GIT_SKIP_TESTS environment variable, or with an individual test suite --run option), unless you know which tests setup the repository state for later tests.

It also means that running only failed tests with prove --state=failed,save or equivalently with

  $ make DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS='--state=failed,save' test
wouldn't work correctly.

As Johannes Schindelin (alias Dscho) said in latest Git Rev News interview: https://git.github.io/rev_news/2018/05/16/edition-39/

JS> We have a test suite where debugging a regression may mean that you JS> have to run 98 test cases before the failing one every single time in JS> the edit/compile/debug cycle, because the 99th test case may depend on JS> a side effect of at least one of the preceding test cases. Git’s test JS> suite is so not [21st century best practices][1]. JS> JS> [1]: https://www.slideshare.net/BuckHodges/lessons-learned-doing-devops-at-scale-at-microsoft

I think can be solved quite efficiently by creating and using shell function, or two shell functions, which would either:

 * rename commit-graph file to some other temporary name if it exists,
   and move it back after the test.
 * create commit-graph file if it does not exist.
For example (untested):
  prepare_no_commit_graph() {
  	mv .git/info/commit-graph .git/info/commit-graph.away &&
  	test_when_finished "mv .git/info/commit-graph.away .git/info/commit-graph"
  }
  prepare_commit_graph() {
  	if ! test -f ".git/info/commit-graph"
  	then
  		git commit-graph write
  	fi
  }
Or something like that.
> +
>  test_done
Regards,
-- 
Jakub Narębski
Previous: Derrick StoleeNext: Derrick Stolee
Message 91 of 149 in “Integrate commit-graph into 'fsck' and 'gc'”
  1. 00/12 Integrate commit-graph into 'fsck' and 'gc'Derrick Stolee, Apr 17, 2018
  2. 01/12 fixup! commit-graph: always load commit-graph informationDerrick Stolee, Apr 17, 2018
  3. 04/12 commit-graph: parse commit from chosen graphDerrick Stolee, Apr 17, 2018
  4. Jakub NarebskiApr 19, 2018
  5. 06/12 commit: force commit to parse from object databaseDerrick Stolee, Apr 17, 2018
  6. Jakub NarebskiApr 20, 2018
  7. 07/12 commit-graph: load a root tree from specific graphDerrick Stolee, Apr 17, 2018
  8. Jakub NarebskiApr 20, 2018
  9. 10/12 commit-graph: add '--reachable' optionDerrick Stolee, Apr 17, 2018
  10. Jakub NarebskiApr 20, 2018
  11. 12/12 commit-graph: update design documentDerrick Stolee, Apr 17, 2018
  12. Jakub NarebskiApr 20, 2018
  13. 11/12 gc: automatically write commit-graph filesDerrick Stolee, Apr 17, 2018
  14. Jakub NarebskiApr 20, 2018
  15. Ævar Arnfjörð BjarmasonApr 20, 2018
  16. 09/12 fsck: check commit-graphDerrick Stolee, Apr 17, 2018
  17. Jakub NarebskiApr 20, 2018
  18. 08/12 commit-graph: verify commit contents against odbDerrick Stolee, Apr 17, 2018
  19. Jakub NarebskiApr 20, 2018
  20. 02/12 commit-graph: add 'check' subcommandDerrick Stolee, Apr 17, 2018
  21. Jakub NarebskiApr 19, 2018
  22. 03/12 commit-graph: check file header informationDerrick Stolee, Apr 17, 2018
  23. Jakub NarebskiApr 19, 2018
  24. 05/12 commit-graph: check fanout and lookup tableDerrick Stolee, Apr 17, 2018
  25. Jakub NarebskiApr 20, 2018
  26. Derrick StoleeApr 17, 2018
  27. 00/12 Integrate commit-graph into fsck, gc, and fetchDerrick Stolee, May 10, 2018
  28. 01/12 commit-graph: add 'verify' subcommandDerrick Stolee, May 10, 2018
  29. Martin ÅgrenMay 10, 2018
  30. 02/12 commit-graph: verify file header informationDerrick Stolee, May 10, 2018
  31. Martin ÅgrenMay 10, 2018
  32. 04/12 commit-graph: verify fanout and lookup tableDerrick Stolee, May 10, 2018
  33. Martin ÅgrenMay 10, 2018
  34. Derrick StoleeMay 11, 2018
  35. 06/12 commit-graph: load a root tree from specific graphDerrick Stolee, May 10, 2018
  36. 08/12 fsck: verify commit-graphDerrick Stolee, May 10, 2018
  37. 12/12 commit-graph: update design documentDerrick Stolee, May 10, 2018
  38. 11/12 fetch: compute commit-graph by defaultDerrick Stolee, May 10, 2018
  39. 07/12 commit-graph: verify commit contents against odbDerrick Stolee, May 10, 2018
  40. 09/12 commit-graph: add '--reachable' optionDerrick Stolee, May 10, 2018
  41. 10/12 gc: automatically write commit-graph filesDerrick Stolee, May 10, 2018
  42. 05/12 commit: force commit to parse from object databaseDerrick Stolee, May 10, 2018
  43. 03/12 commit-graph: parse commit from chosen graphDerrick Stolee, May 10, 2018
  44. Martin ÅgrenMay 10, 2018
  45. Stefan BellerMay 10, 2018
  46. Derrick StoleeMay 11, 2018
  47. Martin ÅgrenMay 11, 2018
  48. Ævar Arnfjörð BjarmasonMay 10, 2018
  49. Derrick StoleeMay 11, 2018
  50. 00/12 Integrate commit-graph into fsck and gcDerrick Stolee, May 11, 2018
  51. 02/12 commit-graph: verify file header informationDerrick Stolee, May 11, 2018
  52. Martin ÅgrenMay 12, 2018
  53. Derrick StoleeMay 14, 2018
  54. Jakub NarebskiMay 20, 2018
  55. 01/12 commit-graph: add 'verify' subcommandDerrick Stolee, May 11, 2018
  56. Martin ÅgrenMay 12, 2018
  57. Derrick StoleeMay 14, 2018
  58. Jakub NarebskiMay 20, 2018
  59. 03/12 commit-graph: test that 'verify' finds corruptionDerrick Stolee, May 11, 2018
  60. Martin ÅgrenMay 12, 2018
  61. Jakub NarebskiMay 21, 2018
  62. Derrick StoleeMay 24, 2018
  63. 04/12 commit-graph: parse commit from chosen graphDerrick Stolee, May 11, 2018
  64. Martin ÅgrenMay 12, 2018
  65. 05/12 commit-graph: verify fanout and lookup tableDerrick Stolee, May 11, 2018
  66. 06/12 commit: force commit to parse from object databaseDerrick Stolee, May 11, 2018
  67. Martin ÅgrenMay 12, 2018
  68. 09/12 fsck: verify commit-graphDerrick Stolee, May 11, 2018
  69. Martin ÅgrenMay 17, 2018
  70. 11/12 gc: automatically write commit-graph filesDerrick Stolee, May 11, 2018
  71. Martin ÅgrenMay 17, 2018
  72. 12/12 commit-graph: update design documentDerrick Stolee, May 11, 2018
  73. 07/12 commit-graph: load a root tree from specific graphDerrick Stolee, May 11, 2018
  74. Martin ÅgrenMay 12, 2018
  75. 08/12 commit-graph: verify commit contents against odbDerrick Stolee, May 11, 2018
  76. Martin ÅgrenMay 12, 2018
  77. Derrick StoleeMay 14, 2018
  78. Martin ÅgrenMay 15, 2018
  79. 10/12 commit-graph: add '--reachable' optionDerrick Stolee, May 11, 2018
  80. Martin ÅgrenMay 17, 2018
  81. 00/20 Integrate commit-graph into 'fsck' and 'gc'Derrick Stolee, May 24, 2018
  82. 01/20 commit-graph: UNLEAK before die()Derrick Stolee, May 24, 2018
  83. Stefan BellerMay 24, 2018
  84. Derrick StoleeMay 25, 2018
  85. 04/20 commit: force commit to parse from object databaseDerrick Stolee, May 24, 2018
  86. Jakub NarebskiMay 27, 2018
  87. 03/20 commit-graph: parse commit from chosen graphDerrick Stolee, May 24, 2018
  88. Jakub NarebskiMay 27, 2018
  89. Derrick StoleeMay 29, 2018
  90. 06/20 commit-graph: add 'verify' subcommandDerrick Stolee, May 24, 2018
  91. Jakub NarebskiMay 27, 2018
  92. Derrick StoleeMay 30, 2018
  93. Jakub NarebskiJun 2, 2018
  94. Derrick StoleeJun 4, 2018
  95. 07/20 commit-graph: verify catches corrupt signatureDerrick Stolee, May 24, 2018
  96. Jakub NarebskiMay 28, 2018
  97. Derrick StoleeMay 29, 2018
  98. Jakub NarebskiJun 2, 2018
  99. 05/20 commit-graph: load a root tree from specific graphDerrick Stolee, May 24, 2018
  100. Jakub NarebskiMay 27, 2018
  101. 08/20 commit-graph: verify required chunks are presentDerrick Stolee, May 24, 2018
  102. Jakub NarebskiMay 28, 2018
  103. 02/20 commit-graph: fix GRAPH_MIN_SIZEDerrick Stolee, May 24, 2018
  104. Jakub NarebskiMay 26, 2018
  105. brian m. carlsonMay 26, 2018
  106. Jakub NarebskiJun 2, 2018
  107. 09/20 commit-graph: verify corrupt OID fanout and lookupDerrick Stolee, May 24, 2018
  108. Jakub NarebskiMay 30, 2018
  109. Derrick StoleeMay 30, 2018
  110. Duy NguyenJun 2, 2018
  111. Derrick StoleeJun 4, 2018
  112. Duy NguyenJun 4, 2018
  113. 10/20 commit-graph: verify objects existDerrick Stolee, May 24, 2018
  114. Jakub NarebskiMay 30, 2018
  115. Derrick StoleeMay 31, 2018
  116. 11/20 commit-graph: verify root tree OIDsDerrick Stolee, May 24, 2018
  117. Jakub NarebskiMay 30, 2018
  118. Derrick StoleeMay 31, 2018
  119. Jakub NarebskiJun 2, 2018
  120. 12/20 commit-graph: verify parent listDerrick Stolee, May 24, 2018
  121. Jakub NarebskiJun 1, 2018
  122. 14/20 commit-graph: verify commit dateDerrick Stolee, May 24, 2018
  123. Jakub NarebskiJun 2, 2018
  124. 16/20 commit-graph: verify contents match checksumDerrick Stolee, May 24, 2018
  125. SZEDER GáborMay 30, 2018
  126. Jakub NarebskiJun 2, 2018
  127. Derrick StoleeJun 4, 2018
  128. 19/20 gc: automatically write commit-graph filesDerrick Stolee, May 24, 2018
  129. Jakub NarebskiJun 2, 2018
  130. Derrick StoleeJun 4, 2018
  131. 20/20 commit-graph: update design documentDerrick Stolee, May 24, 2018
  132. Jakub NarebskiJun 2, 2018
  133. 17/20 fsck: verify commit-graphDerrick Stolee, May 24, 2018
  134. Jakub NarebskiJun 2, 2018
  135. Derrick StoleeJun 4, 2018
  136. 15/20 commit-graph: test for corrupted octopus edgeDerrick Stolee, May 24, 2018
  137. Jakub NarebskiJun 2, 2018
  138. Derrick StoleeJun 4, 2018
  139. 18/20 commit-graph: add '--reachable' optionDerrick Stolee, May 24, 2018
  140. Jakub NarebskiJun 2, 2018
  141. Derrick StoleeJun 4, 2018
  142. 13/20 commit-graph: verify generation numberDerrick Stolee, May 24, 2018
  143. Jakub NarebskiJun 2, 2018
  144. Derrick StoleeJun 4, 2018
  145. Ævar Arnfjörð BjarmasonMay 24, 2018
  146. Junio C HamanoMay 25, 2018
  147. Junio C HamanoMay 29, 2018
  148. Derrick StoleeMay 29, 2018
  149. Junio C HamanoMay 29, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.