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

[Jakub Narebski <jnareb@gmail.com>]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

>
> 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.

> +		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.

>  	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?

>  /*
>   * 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.

> +	)
> +'
> +
>  # 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