Re: [PATCH v3 19/20] gc: automatically write commit-graph files

[Derrick Stolee <stolee@gmail.com>]

On 6/2/2018 2:03 PM, Jakub Narebski wrote:
> Derrick Stolee <dstolee@microsoft.com> writes:
>
>> The commit-graph file is a very helpful feature for speeding up git
>> operations. In order to make it more useful, write the commit-graph file
>> by default during standard garbage collection operations.
> I think you meant here "make it possible to write the commit-graph file
> during standard garbage collection operations." (i.e. add "make it
> possible" because it hides behind new config option, and remove "by
> default" because currently it is not turned on by default).
>
>> Add a 'gc.commitGraph' config setting that triggers writing a
>> commit-graph file after any non-trivial 'git gc' command. Defaults to
>> false while the commit-graph feature matures. We specifically do not
>> want to turn this on by default until the commit-graph feature is fully
> s/turn this on/have this on/  I think.
>
>> integrated with history-modifying features like shallow clones.
> Two things.
>
> First, shallow clones, replacement mechanims (git-replace) and grafts
> are not "history-modifying" features; this name is in my opinion
> reserved for history-rewriting features such as interactive rebase, the
> `git filter-branch` feature or out-of-tree BFG Repo Cleaner or
> reposurgeon tools.  They alter the _view_ of history; they should be
> IMVHO named "history-view-altering" features -- though I agree this is
> mouthful.
>
> Second, shouldn't we, as Martin Ågren said, warn about the issue in the
> manpage for git-commit-graph?
>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>   Documentation/config.txt |  6 ++++++
>>   Documentation/git-gc.txt |  4 ++++
>>   builtin/gc.c             |  6 ++++++
>>   t/t5318-commit-graph.sh  | 14 ++++++++++++++
>>   4 files changed, 30 insertions(+)
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 11f027194e..9a3abd87e7 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1553,6 +1553,12 @@ gc.autoDetach::
>>   	Make `git gc --auto` return immediately and run in background
>>   	if the system supports it. Default is true.
>>   
>> +gc.commitGraph::
>> +	If true, then gc will rewrite the commit-graph file after any
>> +	change to the object database. If '--auto' is used, then the
>> +	commit-graph will not be updated unless the threshold is met.
> What threshold?  Ah, thresholds defined for `git gc --auto` (gc.auto,
> gc.autoPackLimit, gc.logExpiry,...).
>
>> +	See linkgit:git-commit-graph[1] for details.
> You missed declaring the default value for this config option.
>
>> +
>>   gc.logExpiry::
>>   	If the file gc.log exists, then `git gc --auto` won't run
>>   	unless that file is more than 'gc.logExpiry' old.  Default is
>> diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
>> index 571b5a7e3c..17dd654a59 100644
>> --- a/Documentation/git-gc.txt
>> +++ b/Documentation/git-gc.txt
>> @@ -119,6 +119,10 @@ The optional configuration variable `gc.packRefs` determines if
>>   it within all non-bare repos or it can be set to a boolean value.
>>   This defaults to true.
>>   
>> +The optional configuration variable 'gc.commitGraph' determines if
>> +'git gc' runs 'git commit-graph write'. This can be set to a boolean
> Should it be "runs" or "should run"?
>
>> +value. This defaults to false.
> Should it be '...' or `...`?  Below we have `gc.aggresiveWindow`, above
> we have 'gc.commitGraph', for example.
>
>> +
>>   The optional configuration variable `gc.aggressiveWindow` controls how
>>   much time is spent optimizing the delta compression of the objects in
>>   the repository when the --aggressive option is specified.  The larger
>> diff --git a/builtin/gc.c b/builtin/gc.c
>> index 77fa720bd0..efd214a59f 100644
>> --- a/builtin/gc.c
>> +++ b/builtin/gc.c
>> @@ -20,6 +20,7 @@
>>   #include "argv-array.h"
>>   #include "commit.h"
>>   #include "packfile.h"
>> +#include "commit-graph.h"
>>   
>>   #define FAILED_RUN "failed to run %s"
>>   
>> @@ -34,6 +35,7 @@ static int aggressive_depth = 50;
>>   static int aggressive_window = 250;
>>   static int gc_auto_threshold = 6700;
>>   static int gc_auto_pack_limit = 50;
>> +static int gc_commit_graph = 0;
>>   static int detach_auto = 1;
>>   static timestamp_t gc_log_expire_time;
>>   static const char *gc_log_expire = "1.day.ago";
>> @@ -121,6 +123,7 @@ static void gc_config(void)
>>   	git_config_get_int("gc.aggressivedepth", &aggressive_depth);
>>   	git_config_get_int("gc.auto", &gc_auto_threshold);
>>   	git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit);
>> +	git_config_get_bool("gc.commitgraph", &gc_commit_graph);
>>   	git_config_get_bool("gc.autodetach", &detach_auto);
>>   	git_config_get_expiry("gc.pruneexpire", &prune_expire);
>>   	git_config_get_expiry("gc.worktreepruneexpire", &prune_worktrees_expire);
>> @@ -480,6 +483,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>>   	if (pack_garbage.nr > 0)
>>   		clean_pack_garbage();
>>   
>> +	if (gc_commit_graph)
>> +		write_commit_graph_reachable(get_object_directory(), 0);
>> +
> Nice.
>
> Though now I wonder when appending should be used...

Appending is probably useless in the 'reachable' case, but is valuable 
in the '--stdin-packs' case (which is what we use in GVFS to maintain 
the commit-graph).

>
>>   	if (auto_gc && too_many_loose_objects())
>>   		warning(_("There are too many unreachable loose objects; "
>>   			"run 'git prune' to remove them."));
>> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
>> index a659620332..d20b17586f 100755
>> --- a/t/t5318-commit-graph.sh
>> +++ b/t/t5318-commit-graph.sh
>> @@ -245,6 +245,20 @@ test_expect_success 'perform fast-forward merge in full repo' '
>>   	test_cmp expect output
>>   '
>>   
>> +test_expect_success 'check that gc clears commit-graph' '
> I wouldn't use the word "clears" here...
>
>> +	cd "$TRASH_DIRECTORY/full" &&
>> +	git commit --allow-empty -m "blank" &&
>> +	git commit-graph write --reachable &&
>> +	cp $objdir/info/commit-graph commit-graph-before-gc &&
>> +	git reset --hard HEAD~1 &&
>> +	git config gc.commitGraph true &&
>> +	git gc &&
>> +	cp $objdir/info/commit-graph commit-graph-after-gc &&
>> +	! test_cmp commit-graph-before-gc commit-graph-after-gc &&
>> +	git commit-graph write --reachable &&
>> +	test_cmp commit-graph-after-gc $objdir/info/commit-graph
>> +'
> ...but otherwise, nice test: it checks that git-gc after rewriting
> history changes commit-graph file, and that the changed file is what we
> expect it to be (note: here we compare commit-graph files directly, and
> not just check the features via 'git commit-graph read').
>
>> +
>>   # 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