On Mon, Apr 06, 2020 at 04:59:51PM +0000, Garima Singh via GitGitGadget wrote:
> From: Garima Singh <garima.singh@xxxxxxxxxxxxx>
>
> Add --changed-paths option to git commit-graph write. This option will
> allow users to compute information about the paths that have changed
> between a commit and its first parent, and write it into the commit graph
> file. If the option is passed to the write subcommand we set the
> COMMIT_GRAPH_WRITE_BLOOM_FILTERS flag and pass it down to the
> commit-graph logic.
>
> Helped-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> Signed-off-by: Garima Singh <garima.singh@xxxxxxxxxxxxx>
> ---
> Documentation/git-commit-graph.txt | 5 +++++
> builtin/commit-graph.c | 9 +++++++--
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
> index 28d1fee5053..f4b13c005b8 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -57,6 +57,11 @@ or `--stdin-packs`.)
> With the `--append` option, include all commits that are present in the
> existing commit-graph file.
> +
> +With the `--changed-paths` option, compute and write information about the
> +paths changed between a commit and it's first parent. This operation can
> +take a while on large repositories. It provides significant performance gains
> +for getting history of a directory or a file with `git log -- <path>`.
So 'git commit-graph write' only computes and writes changed path
Bloom filters if this option is specified. Though not mentioned in
the documentation or in the commit message, the negated
'--no-changed-paths' is supported as well, and it removes Bloom
filters from the commit-graph file. All this is quite reasonable.
However, the most important question is what happens when the
commit-graph file already contains Bloom filters and neither of these
options are specified on the command line. This isn't mentioned in
the docs or in the commit message, either, but as it is implemented in
this patch (i.e. COMMIT_GRAPH_WRITE_BLOOM_FILTERS is not passed from
the builtin to the commit-graph logic) all those existing Bloom
filters are removed from the commit-graph. Considering how expensive
it was to compute those Bloom filters this might not be the most
desirable behaviour.
This is important, because 'git commit-graph write' is not the only
command that writes the commit-graph file. 'git gc' does that by
default, too, and will wipe out any modified path Bloom filters while
doing so. Worse, the user doesn't even have to invoke 'git gc'
manually, because a lot of git commands invoke 'git gc --auto'.
$ git commit-graph write --reachable --changed-paths
$ ~/src/git/t/helper/test-tool read-graph |grep ^chunks
chunks: oid_fanout oid_lookup commit_metadata bloom_indexes bloom_data
$ git gc --quiet
$ ~/src/git/t/helper/test-tool read-graph |grep ^chunks
chunks: oid_fanout oid_lookup commit_metadata
Consequently, if users want to use modified path Bloom filters, then
they should avoid gc, both manual and auto, or they'll have to
re-generate the Bloom filters every once in a while. That is
definitely not the desired behaviour.
Now compare this e.g. to the behaviour of 'git update-index
--split-index' and '--untracked-cache': both of these options turn on
features that improve performance and write extra stuff to the index,
and after they did so all subsequent git commands updating the index
will keep writing that extra stuff, including 'git update-index'
itself even without those options, until it's finally invoked with the
corresponding '--no-...' option. I particularly like how
'--[no-]untracked-cache' and 'core.untrackedCache' work together and
warn when the given command line option goes against the configured
value, and I think the command line options and configuration
variables controlling modified path Bloom filters should behave
similarly.
> With the `--split` option, write the commit-graph as a chain of multiple
> commit-graph files stored in `<dir>/info/commit-graphs`. The new commits
> not already in the commit-graph are added in a new "tip" file. This file
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index d1ab6625f63..cacb5d04a80 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -9,7 +9,7 @@
>
> static char const * const builtin_commit_graph_usage[] = {
> N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
> - N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] <split options>"),
> + N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--changed-paths] [--[no-]progress] <split options>"),
> NULL
> };
>
> @@ -19,7 +19,7 @@ static const char * const builtin_commit_graph_verify_usage[] = {
> };
>
> static const char * const builtin_commit_graph_write_usage[] = {
> - N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] <split options>"),
> + N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--changed-paths] [--[no-]progress] <split options>"),
> NULL
> };
>
> @@ -32,6 +32,7 @@ static struct opts_commit_graph {
> int split;
> int shallow;
> int progress;
> + int enable_changed_paths;
> } opts;
>
> static struct object_directory *find_odb(struct repository *r,
> @@ -135,6 +136,8 @@ static int graph_write(int argc, const char **argv)
> N_("start walk at commits listed by stdin")),
> OPT_BOOL(0, "append", &opts.append,
> N_("include all commits already in the commit-graph file")),
> + OPT_BOOL(0, "changed-paths", &opts.enable_changed_paths,
> + N_("enable computation for changed paths")),
> OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
> OPT_BOOL(0, "split", &opts.split,
> N_("allow writing an incremental commit-graph file")),
> @@ -168,6 +171,8 @@ static int graph_write(int argc, const char **argv)
> flags |= COMMIT_GRAPH_WRITE_SPLIT;
> if (opts.progress)
> flags |= COMMIT_GRAPH_WRITE_PROGRESS;
> + if (opts.enable_changed_paths)
> + flags |= COMMIT_GRAPH_WRITE_BLOOM_FILTERS;
>
> read_replace_refs = 0;
> odb = find_odb(the_repository, opts.obj_dir);
> --
> gitgitgadget
>
