Re: [PATCH 6/8] commit-graph: not compatible with grafts
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Jul 29, 2018, 22:04 UTC
- Message-ID
- <86bmap7l7a.fsf@gmail.com>
- In-Reply-To
- <94dd91ac35006ebee3c8808af1fce4ad69975234.1531926932.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 6 quoted lines
> 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.
Show 22 quoted lines
>
> 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?
Show 28 quoted lines
> +
> 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.
Show 18 quoted lines
> 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.
Show 7 quoted lines
> + ) > +' > + > +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"?
Show 7 quoted lines
> + ( > + 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.
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