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

Re: [PATCH 5/8] commit-graph: not compatible with replace objects

From
Jakub Narebski <jnareb@gmail.com>
Date
Jul 29, 2018, 21:00 UTC
Message-ID
<86wotd7o64.fsf@gmail.com>
In-Reply-To
<7f596c1718d35539f02828edbf933c8e660f123b.1531926932.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Create new method commit_graph_compatible(r) to check if a given
> repository r is compatible with the commit-graph feature. Fill the
> method with a check to see if replace-objects exist.

All right, looks sensible. Did you start with the history-"altering" feature with simplest detection mechanism?

>                                                       Test this
> interaction succeeds, including ignoring an existing commit-graph and
> failing to write a new commit-graph.

I wonder if we should test the implementation, or just that you get correct answer when history view is altered after writing commit-graph file... oh, wait, you do that (well, except for testing that `git commit-graph write` does not create commit-graph file instead of testing that the commit-graph file stores correct information even after the feature is removed (with `git replace --delete`) or turned off (with `--no-replace-objects` or equivalent). Good.

Sidenote: the inconsistency between command options and
subcommands... ehh... compare e.g. git-replace and git-remote.
Show 30 quoted lines
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  commit-graph.c          | 17 +++++++++++++++++
>  replace-object.c        |  2 +-
>  replace-object.h        |  2 ++
>  t/t5318-commit-graph.sh | 22 ++++++++++++++++++++++
>  4 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index b0a55ad12..711099858 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -13,6 +13,8 @@
>  #include "commit-graph.h"
>  #include "object-store.h"
>  #include "alloc.h"
> +#include "hashmap.h"
> +#include "replace-object.h"
>  
>  #define GRAPH_SIGNATURE 0x43475048 /* "CGPH" */
>  #define GRAPH_CHUNKID_OIDFANOUT 0x4f494446 /* "OIDF" */
> @@ -56,6 +58,15 @@ static struct commit_graph *alloc_commit_graph(void)
>  	return g;
>  }
>  
> +static int commit_graph_compatible(struct repository *r)
> +{
> +	prepare_replace_object(r);
> +	if (hashmap_get_size(&r->objects->replace_map->map))

A very minor nitpick, feel free to ignore. I think that the condition would be more readable written as

  +	if (hashmap_get_size(&r->objects->replace_map->map) > 0)

Another issue is that the condition may be too strict (but it is also the easiest to test for). You might have replacement objects for non-commit objects (e.g. replacing contents of configuration files with one with API keys and/or other secrets - not that I know any tool that does that), or replacing commit-objects without changing their history (e.g. to fix some annoying error in demo repository, where you want to show reflog). But I think that would be rare, so easier if maybe over-eager test is fine for me.

Show 25 quoted lines
> +		return 0;
> +
> +	return 1;
> +}
> +
>  struct commit_graph *load_commit_graph_one(const char *graph_file)
>  {
>  	void *graph_map;
> @@ -223,6 +234,9 @@ static int prepare_commit_graph(struct repository *r)
>  		 */
>  		return 0;
>  
> +	if (!commit_graph_compatible(r))
> +		return 0;
> +
>  	obj_dir = r->objects->objectdir;
>  	prepare_commit_graph_one(r, obj_dir);
>  	prepare_alt_odb(r);
> @@ -693,6 +707,9 @@ void write_commit_graph(const char *obj_dir,
>  	int num_extra_edges;
>  	struct commit_list *parent;
>  
> +	if (!commit_graph_compatible(the_repository))
> +		return;
> +

Hmmm... we might want to write commit-graph for immutable history; but not all history-"altering" feature can be easily turned off. So if it is to stay universal, without aadditional complications, it is good enough.

Show 26 quoted lines
>  	oids.nr = 0;
>  	oids.alloc = approximate_object_count() / 4;
>  
> diff --git a/replace-object.c b/replace-object.c
> index 017f02f8e..4d2d84cf4 100644
> --- a/replace-object.c
> +++ b/replace-object.c
> @@ -32,7 +32,7 @@ static int register_replace_ref(struct repository *r,
>  	return 0;
>  }
>  
> -static void prepare_replace_object(struct repository *r)
> +void prepare_replace_object(struct repository *r)
>  {
>  	if (r->objects->replace_map)
>  		return;
> diff --git a/replace-object.h b/replace-object.h
> index f996de3d6..c7a99fc35 100644
> --- a/replace-object.h
> +++ b/replace-object.h
> @@ -10,6 +10,8 @@ struct replace_object {
>  	struct object_id replacement;
>  };
>  
> +void prepare_replace_object(struct repository *r);
> +

Hmmm... how it is that this function didn't need to be exported before, I do wonder?

Show 19 quoted lines
>  /*
>   * This internal function is only declared here for the benefit of
>   * lookup_replace_object().  Please do not call it directly.
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 4f17d7701..c90626f5d 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -259,6 +259,28 @@ test_expect_success 'check that gc computes commit-graph' '
>  	test_cmp commit-graph-after-gc $objdir/info/commit-graph
>  '
>  
> +test_expect_success 'replace-objects invalidates commit-graph' '
> +	cd "$TRASH_DIRECTORY" &&
> +	test_when_finished rm -rf replace &&
> +	git clone full replace &&
> +	(
> +		cd replace &&
> +		git commit-graph write --reachable &&
> +		test_path_is_file .git/objects/info/commit-graph &&

All right, this is preparation / setup; create commit-graph file, and check that it was created.

> +		git replace HEAD~1 HEAD~2 &&
All right, so before replacement the history looks like this:
     ... <--- A <--- B <--- C <--- D     <--- master  <=== HEAD
             ~3     ~2     ~1    HEAD

While after the replacement the history view looks like this, if I understand it correctly:

     ... <--- A <--- B
               \
                \---[B] <--- D    <--- master  <=== HEAD

Wouldn't it be better (and more similar to the grafts test) to use `git replace --graft`, e.g.:

  +		git replace --graft HEAD~1 HEAD~3 &&
But that might be just a matter of taste.
> +		git -c core.commitGraph=false log >expect &&
> +		git -c core.commitGraph=true log >actual &&
> +		test_cmp expect actual &&

All right, so instead of testing if the output is what is expected, we test that the commit-graph feature did not change it. Good.

> +		git commit-graph write --reachable &&
Is this necessary [see below]?
> +		git -c core.commitGraph=false --no-replace-objects log >expect &&
> +		git -c core.commitGraph=true --no-replace-objects log >actual &&

All right, and here we test the same for `--no-replace-objects` case. Good.

By the way, is this `git commit-graph write --reachable` preceding this part of test necessary?

> +		test_cmp expect actual &&
> +		rm -rf .git/objects/info/commit-graph &&
> +		git commit-graph write --reachable &&
> +		test_path_is_missing .git/objects/info/commit-graph

All right, so here we test that we wont write commit-graph file with altered view of history; which would store wrong information if the replace feature is turned off or if replace object is deleted (or if commit graph gets transferred in the furure, but the replacement refs do not).

Checking that the commit-graph file ws not created might be seen as testing implementation details, but full test would require testing with `--no-replace-objects` and after `git replace -d`; the above is simpler, so good enough.

Show 6 quoted lines
> +	)
> +'
> +
>  # the verify tests below expect the commit-graph to contain
>  # exactly the commits reachable from the commits/8 branch.
>  # If the file changes the set of commits in the list, then the
Previous: Derrick StoleeNext: Derrick Stolee via GitGitGadget
Message 18 of 47 in “Clarify commit-graph and grafts/replace/shallow incompatibilities”
  1. 0/8 Clarify commit-graph and grafts/replace/shallow incompatibilitiesDerrick Stolee via GitGitGadget, Jul 18, 2018
  2. 1/8 refs.c: migrate internal ref iteration to pass thru repository argumentStefan Beller via GitGitGadget, Jul 18, 2018
  3. 2/8 refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callbackStefan Beller via GitGitGadget, Jul 18, 2018
  4. Junio C HamanoJul 18, 2018
  5. Stefan BellerJul 18, 2018
  6. Junio C HamanoJul 18, 2018
  7. 3/8 commit-graph: update design documentDerrick Stolee via GitGitGadget, Jul 18, 2018
  8. Jakub NarebskiJul 29, 2018
  9. 4/8 test-repository: properly init repoDerrick Stolee via GitGitGadget, Jul 18, 2018
  10. Jakub NarebskiJul 29, 2018
  11. 5/8 commit-graph: not compatible with replace objectsDerrick Stolee via GitGitGadget, Jul 18, 2018
  12. Jeff KingJul 18, 2018
  13. Jeff KingJul 18, 2018
  14. Derrick StoleeJul 18, 2018
  15. Derrick StoleeJul 20, 2018
  16. Stefan BellerJul 20, 2018
  17. Derrick StoleeJul 20, 2018
  18. Jakub NarebskiJul 29, 2018
  19. 6/8 commit-graph: not compatible with graftsDerrick Stolee via GitGitGadget, Jul 18, 2018
  20. Jakub NarebskiJul 29, 2018
  21. 7/8 commit-graph: not compatible with uninitialized repoDerrick Stolee via GitGitGadget, Jul 18, 2018
  22. Jakub NarebskiJul 29, 2018
  23. 8/8 commit-graph: close_commit_graph before shallow walkDerrick Stolee via GitGitGadget, Jul 18, 2018
  24. Jakub NarebskiJul 30, 2018
  25. DO-NOT-MERGE: write and read commit-graph alwaysDerrick Stolee, Jul 18, 2018
  26. Jakub NarebskiJul 30, 2018
  27. Stefan BellerJul 30, 2018
  28. Jakub NarebskiJul 31, 2018
  29. Elijah NewrenJul 31, 2018
  30. Jakub NarebskiJul 29, 2018
  31. Eric SunshineJul 29, 2018
  32. 0/8 Clarify commit-graph and grafts/replace/shallow incompatibilitiesDerrick Stolee, Aug 20, 2018
  33. 6/8 commit-graph: not compatible with graftsDerrick Stolee, Aug 20, 2018
  34. 7/8 commit-graph: not compatible with uninitialized repoDerrick Stolee, Aug 20, 2018
  35. 2/8 refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callbackDerrick Stolee, Aug 20, 2018
  36. 3/8 commit-graph: update design documentDerrick Stolee, Aug 20, 2018
  37. 4/8 test-repository: properly init repoDerrick Stolee, Aug 20, 2018
  38. 5/8 commit-graph: not compatible with replace objectsDerrick Stolee, Aug 20, 2018
  39. Junio C HamanoAug 21, 2018
  40. Derrick StoleeAug 21, 2018
  41. 8/8 commit-graph: close_commit_graph before shallow walkDerrick Stolee, Aug 20, 2018
  42. 1/8 refs.c: migrate internal ref iteration to pass thru repository argumentDerrick Stolee, Aug 20, 2018
  43. Stefan BellerAug 20, 2018
  44. Junio C HamanoAug 21, 2018
  45. Derrick StoleeAug 21, 2018
  46. Stefan BellerAug 20, 2018
  47. Derrick StoleeAug 20, 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.