commit-graph idea: warn when disabled for incompatible repositories

commit-graph idea: warn when disabled for incompatible repositories

[Thomas Ferris Nicolaisen]

Hey there,

I recently started dabbling with commit-graph.

Accidentally, my first use-case was a local copy of a big repository
(chromium source) that another developer had cloned shallow (I wasn't
even aware of their clone being shallow at that point).

After spending a little time trying to figure out why no commit-graph
file was being created, I noticed that it worked just fine testing in
a fresh git repo.

Then I discovered the .git/shallow file in the big repo. So I did
fetch --unshallow, and commit-graph started working. Taking a 20
second log --graph operation down to less than a second (wooo!).

I saw some recent release notes that mentions that commit-graph is
disabled in incompatible repositories (graft, replace). I assume this
also be the case for shallow clones.

Here's the idea that may help others on the same path: Some warning
output when attempting to use commit-graph when it is disabled (either
by configuration or automatically).

I think others that come across commit-graph may have tried such
tricks (like shallow clones) to better work with their repositories,
and it could be frustrating that commit-graph has no apparent effect.

Apologies if this is a stupid/bad/old idea!

Re: commit-graph idea: warn when disabled for incompatible repositories

[Derrick Stolee]

On 12/18/2018 9:22 AM, Thomas Ferris Nicolaisen wrote:
> Hey there,

Hi, Thomas!

> Accidentally, my first use-case was a local copy of a big repository
> (chromium source) that another developer had cloned shallow (I wasn't
> even aware of their clone being shallow at that point).
>
> After spending a little time trying to figure out why no commit-graph
> file was being created, I noticed that it worked just fine testing in
> a fresh git repo.
>
> Then I discovered the .git/shallow file in the big repo. So I did
> fetch --unshallow, and commit-graph started working. Taking a 20
> second log --graph operation down to less than a second (wooo!).
>
> I saw some recent release notes that mentions that commit-graph is
> disabled in incompatible repositories (graft, replace). I assume this
> also be the case for shallow clones.

The commit-graph feature is not designed to work well with these 
features, and the "easy" fix we have so far is to completely avoid the 
interaction. The tricky bits come in when the commit parents can 
"change" according to these other features. The commit-graph would need 
to change at the same time, and this is actually very difficult to get 
correct.

> Here's the idea that may help others on the same path: Some warning
> output when attempting to use commit-graph when it is disabled (either
> by configuration or automatically).
>
> I think others that come across commit-graph may have tried such
> tricks (like shallow clones) to better work with their repositories,
> and it could be frustrating that commit-graph has no apparent effect.

This is a good idea, and I would happily review a patch that added such 
a warning.

The warning would want to be in builtin/commit-graph.c, and use the 
commit_graph_compatible() method from commit-graph.c. (You'll need to 
expose the method in the .h file.)

Thanks!

-Stolee

Re: commit-graph idea: warn when disabled for incompatible repositories

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 18 2018, Derrick Stolee wrote:

> On 12/18/2018 9:22 AM, Thomas Ferris Nicolaisen wrote:
>> Hey there,
>
> Hi, Thomas!
>
>> Accidentally, my first use-case was a local copy of a big repository
>> (chromium source) that another developer had cloned shallow (I wasn't
>> even aware of their clone being shallow at that point).
>>
>> After spending a little time trying to figure out why no commit-graph
>> file was being created, I noticed that it worked just fine testing in
>> a fresh git repo.
>>
>> Then I discovered the .git/shallow file in the big repo. So I did
>> fetch --unshallow, and commit-graph started working. Taking a 20
>> second log --graph operation down to less than a second (wooo!).
>>
>> I saw some recent release notes that mentions that commit-graph is
>> disabled in incompatible repositories (graft, replace). I assume this
>> also be the case for shallow clones.
>
> The commit-graph feature is not designed to work well with these
> features, and the "easy" fix we have so far is to completely avoid the
> interaction. The tricky bits come in when the commit parents can
> "change" according to these other features. The commit-graph would
> need to change at the same time, and this is actually very difficult
> to get correct.
>
>> Here's the idea that may help others on the same path: Some warning
>> output when attempting to use commit-graph when it is disabled (either
>> by configuration or automatically).
>>
>> I think others that come across commit-graph may have tried such
>> tricks (like shallow clones) to better work with their repositories,
>> and it could be frustrating that commit-graph has no apparent effect.
>
> This is a good idea, and I would happily review a patch that added
> such a warning.
>
> The warning would want to be in builtin/commit-graph.c, and use the
> commit_graph_compatible() method from commit-graph.c. (You'll need to
> expose the method in the .h file.)

This patch will work. Consider it to have my SOB. Just needs tests, and
b.t.w. I noticed that if I s/warning/die/ in this patch the commit graph
& GC tests pass, pointing to a blind spot in the test suite.

diff --git a/builtin/gc.c b/builtin/gc.c
index 871a56f1c5..702568b70d 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -662,9 +662,14 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	if (pack_garbage.nr > 0)
 		clean_pack_garbage();

-	if (gc_write_commit_graph)
+	if (gc_write_commit_graph) {
+		int verbose = !quiet && !daemonized;
+		if (verbose && !commit_graph_compatible(the_repository))
+			warning(_("The `gc.writeCommitGraph' setting is on, "
+				  "but commit_graph_compatible() = false"));
 		write_commit_graph_reachable(get_object_directory(), 0,
-					     !quiet && !daemonized);
+					     verbose);
+	}

 	if (auto_gc && too_many_loose_objects())
 		warning(_("There are too many unreachable loose objects; "
diff --git a/commit-graph.c b/commit-graph.c
index 40c855f185..60915bf9aa 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -61,7 +61,7 @@ static struct commit_graph *alloc_commit_graph(void)

 extern int read_replace_refs;

-static int commit_graph_compatible(struct repository *r)
+int commit_graph_compatible(struct repository *r)
 {
 	if (!r->gitdir)
 		return 0;
diff --git a/commit-graph.h b/commit-graph.h
index 9db40b4d3a..7c92d41a28 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -12,6 +12,8 @@ struct commit;

 char *get_commit_graph_filename(const char *obj_dir);

+int commit_graph_compatible(struct repository *r);
+
 /*
  * Given a commit struct, try to fill the commit struct info, including:
  *  1. tree object

Re: commit-graph idea: warn when disabled for incompatible repositories

[Derrick Stolee]

On 12/18/2018 1:21 PM, Ævar Arnfjörð Bjarmason wrote:
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 871a56f1c5..702568b70d 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -662,9 +662,14 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>   	if (pack_garbage.nr > 0)
>   		clean_pack_garbage();
>
> -	if (gc_write_commit_graph)
> +	if (gc_write_commit_graph) {
> +		int verbose = !quiet && !daemonized;
> +		if (verbose && !commit_graph_compatible(the_repository))
> +			warning(_("The `gc.writeCommitGraph' setting is on, "
> +				  "but commit_graph_compatible() = false"));
>   		write_commit_graph_reachable(get_object_directory(), 0,
> -					     !quiet && !daemonized);
> +					     verbose);
> +	}

I actually think this is the wrong place to put it. This will cause a 
warning for someone with 'gc.writeCommitGraph' enabled and running GC on 
a shallow clone.

I think the issue was someone running 'git commit-graph write' inside a 
shallow clone that succeeds without doing anything.

Also, I bet you would hit this block if you run the test suite with 
GIT_TEST_COMMIT_GRAPH=1.

Thanks,

-Stolee


>   	if (auto_gc && too_many_loose_objects())
>   		warning(_("There are too many unreachable loose objects; "
> diff --git a/commit-graph.c b/commit-graph.c
> index 40c855f185..60915bf9aa 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -61,7 +61,7 @@ static struct commit_graph *alloc_commit_graph(void)
>
>   extern int read_replace_refs;
>
> -static int commit_graph_compatible(struct repository *r)
> +int commit_graph_compatible(struct repository *r)
>   {
>   	if (!r->gitdir)
>   		return 0;
> diff --git a/commit-graph.h b/commit-graph.h
> index 9db40b4d3a..7c92d41a28 100644
> --- a/commit-graph.h
> +++ b/commit-graph.h
> @@ -12,6 +12,8 @@ struct commit;
>
>   char *get_commit_graph_filename(const char *obj_dir);
>
> +int commit_graph_compatible(struct repository *r);
> +
>   /*
>    * Given a commit struct, try to fill the commit struct info, including:
>    *  1. tree object

This part looks correct, and necessary for the warning in 
builtin/commit-graph.c

Thanks,

-Stolee