Re: [PATCH 6/8] commit-graph: not compatible with grafts

[Jakub Narebski <jnareb@gmail.com>]

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

> From: Derrick Stolee <dstolee@microsoft.com>
>
> Augment commit_graph_compatible(r) to return false when the given
> repository r has commit grafts or is a shallow clone. Test that in these
> situations we ignore existing commit-graph files and we do not write new
> commit-graph files.

Tests for grafts are wrong, as they test replace objects.

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  commit-graph.c          |  6 ++++++
>  commit.c                |  2 +-
>  commit.h                |  1 +
>  t/t5318-commit-graph.sh | 36 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 711099858..5097c7c12 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -60,6 +60,12 @@ static struct commit_graph *alloc_commit_graph(void)
>  
>  static int commit_graph_compatible(struct repository *r)
>  {
> +	prepare_commit_graft(r);
> +	if (r->parsed_objects && r->parsed_objects->grafts_nr)
> +		return 0;
> +	if (is_repository_shallow(r))
> +		return 0;

Do I assume correctly that is_repository_shallow(r) needs
prepare_commit_graph(r) done earlier?  If it is so, all right.

If it is not so, then why put two separate history-"altering" features,
namely grafts and shallow clones in single commit instead of in separate
commits?

> +
>  	prepare_replace_object(r);
>  	if (hashmap_get_size(&r->objects->replace_map->map))
>  		return 0;
> diff --git a/commit.c b/commit.c
> index 39b80bd21..609adaf8a 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -209,7 +209,7 @@ static int read_graft_file(struct repository *r, const char *graft_file)
>  	return 0;
>  }
>  
> -static void prepare_commit_graft(struct repository *r)
> +void prepare_commit_graft(struct repository *r)
>  {
>  	char *graft_file;
>  
> diff --git a/commit.h b/commit.h
> index da0db36eb..5459e279f 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -202,6 +202,7 @@ typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);
>  
>  struct commit_graft *read_graft_line(struct strbuf *line);
>  int register_commit_graft(struct repository *r, struct commit_graft *, int);
> +void prepare_commit_graft(struct repository *r);
>  struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);
>

I guess that prepare_commit_graft() was not needed outside commit.c
before.

>  extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2);
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index c90626f5d..1d9f49af5 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -281,6 +281,42 @@ test_expect_success 'replace-objects invalidates commit-graph' '
>  	)
>  '
>  
> +test_expect_success 'commit grafts invalidate commit-graph' '
> +	cd "$TRASH_DIRECTORY" &&
> +	test_when_finished rm -rf graft &&
> +	git clone full graft &&
> +	(
> +		cd graft &&
> +		git commit-graph write --reachable &&
> +		test_path_is_file .git/objects/info/commit-graph &&
> +		git replace --graft HEAD~1 HEAD~3 &&

Errr... this uses *replace objects* mechanism, not graft file!!  The
`--graft` option of git-remote command is here to make it easier to use
replace objects to alter the view of history, and make if easy to
convert from deprected grafts feature to replace objects.

Grafts file structure is described in gitrepository-layout(5) manpage.
You would need to do something like the following:

  +		H1=$(git rev-parse --verify HEAD~1) &&
  +		H3=$(git rev-parse --verify HEAD~3) &&
  +		echo '$H1 $H3' >.git/info/grafts &&


You could have used "git replace --graft HEAD~1 HEAD~3" in previous
commit test, like I wrote in reply to it.

> +		git -c core.commitGraph=false log >expect &&
> +		git -c core.commitGraph=true log >actual &&
> +		test_cmp expect actual &&

All right, we test that we get the same result (when graft-file is
written before altering the view of history) with and without the
commit-graph feature.

Sidenote: shouldn't we use rev-list instead?  Though here git-log is
actually safe to use...

> +		git commit-graph write --reachable &&
> +		git -c core.commitGraph=false --no-replace-objects log >expect &&
> +		git -c core.commitGraph=true --no-replace-objects log >actual &&

The `--no-replace-objects` does not affect the graph file!!  You would
want to remove graft file instead:

  +		git commit-graph write --reachable &&
  +		rm -f .git/info/grafts &&
  +		git -c core.commitGraph=false log >expect &&
  +		git -c core.commitGraph=true log >actual &&

And then check the results.

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

Though, if it is the same for grafts and for replacements... nah.
Adding a feature and deleting a feature is different, and it is only a
single repetition.

> +	)
> +'
> +
> +test_expect_success 'replace-objects invalidates commit-graph' '
> +	cd "$TRASH_DIRECTORY" &&
> +	test_when_finished rm -rf shallow &&
> +	git clone --depth 2 "file://$TRASH_DIRECTORY/full" shallow &&

Why do we need to use "file://..." URL, instead of simply "full"?

> +	(
> +		cd shallow &&
> +		git commit-graph write --reachable &&
> +		test_path_is_missing .git/objects/info/commit-graph &&
> +		git fetch origin --unshallow &&
> +		git commit-graph write --reachable &&
> +		test_path_is_file .git/objects/info/commit-graph

All right, so in this case because you need to start (clone) with
shallow feature, the test could be simplified.

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