Derrick Stolee <dsto...@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 <dsto...@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...
> 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
