On Fri, Oct 05 2018, Derrick Stolee wrote:
> On 10/4/2018 5:42 PM, Ævar Arnfjörð Bjarmason wrote:
>> I don't have time to polish this up for submission now, but here's a WIP
>> patch that implements this, highlights:
>>
>> * There's a gc.clone.autoDetach=false default setting which overrides
>> gc.autoDetach if 'git gc --auto' is run via git-clone (we just pass a
>> --cloning option to indicate this).
>
> I'll repeat that it could make sense to do the same thing on clone
> _and_ fetch. Perhaps a "--post-fetch" flag would be good here to
> communicate that we just downloaded a pack from a remote.
I don't think that makes sense, but let's talk about why, because maybe
I've missed something, you're certainly more familiar with the
commit-graph than I am.
The reason to do it on clone as a special-case or when the file is
missing, is because we know the file is desired (via the GC config), and
presumably is expected to help performance, and we have 0% of it. So by
going from 0% to 100% on clone we'll get fast --contains and other
goodies the graph helps with.
But when we're doing a fetch, or really anything else that runs "git gc
--auto" we can safely assume that we have a recent enough graph, because
it will have been run whenever auto-gc kicked in.
I.e.:
# Slow, if we assume background forked commit-graph generation
# (which I'm avoiding)
git clone x && cd x && git tag --contains
# Fast enough, since we have an existing commit-graph
cd x && git fetch && git tag --contains
I *do* think it might make sense to in general split off parts of "gc
--auto" that we'd like to be more aggressive about, simply because the
ratio of how long it takes to do, and how much it helps with performance
makes more sense than a full repack, which is what the current heuristic
is based on.
And maybe when we run in that mode we should run in the foreground, but
I don't see why git-fetch should be a special case there, and in this
regard, the gc.clone.autoDetach=false setting I've made doesn't make
much sence. I.e. maybe we should also skip forking to the background in
such a mode when we trigger such a "mini gc" via git-commit or whatever.
>> * A clone of say git.git with gc.writeCommitGraph=true looks like:
>>
>> [...]
>> Receiving objects: 100% (255262/255262), 100.49 MiB | 17.78 MiB/s, done.
>> Resolving deltas: 100% (188947/188947), done.
>> Computing commit graph generation numbers: 100% (55210/55210), done.
>
> This looks like good UX. Thanks for the progress here!
>
>> * The 'git gc --auto' command also knows to (only) run the commit-graph
>> (and space is left for future optimization steps) if general GC isn't
>> needed, but we need "optimization":
>>
>> $ rm .git/objects/info/commit-graph; ~/g/git/git --exec-path=$PWD -c gc.writeCommitGraph=true -c gc.autoDetach=false gc --auto;
>> Annotating commits in commit graph: 341229, done.
>> Computing commit graph generation numbers: 100% (165969/165969), done.
>> $
>
> Will this also trigger a full commit-graph rewrite on every 'git
> commit' command?
Nope, because "git commit" can safely be assumed to have some
commit-graph anyway, and I'm just special casing the case where it
doesn't exist.
But if it doesn't exist and you do a "git commit" then "gc --auto" will
be run, and we'll fork to the background and generate it...
> Or is there some way we can compute the staleness of
> the commit-graph in order to only update if we get too far ahead?
> Previously, this was solved by relying on the auto-GC threshold.
So re the "I don't think that makes sense..." at the start of my E-Mail,
isn't it fine to rely on the default thresholds here, or should we be
more aggressive?
>> * The patch to gc.c looks less scary with -w, most of it is indenting
>> the existing pack-refs etc. with a "!auto_gc || should_gc" condition.
>>
>> * I added a commit_graph_exists() exists function and only care if I
>> get ENOENT for the purposes of this gc mode. This would need to be
>> tweaked for the incremental mode Derrick talks about, but if we just
>> set "should_optimize" that'll also work as far as gc --auto is
>> concerned (e.g. on fetch, am etc.)
>
> The incremental mode would operate the same as split-index, which
> means we will still look for .git/objects/info/commit-graph. That file
> may point us to more files.
Ah!
>> +int commit_graph_exists(const char *graph_file)
>> +{
>> + struct stat st;
>> + if (stat(graph_file, &st)) {
>> + if (errno == ENOENT)
>> + return 0;
>> + else
>> + return -1;
>> + }
>> + return 1;
>> +}
>> +
>
> This method serves a very similar purpose to
> generation_numbers_enabled(), except your method only cares about the
> file existing. It ignores information like `core.commitGraph`, which
> should keep us from doing anything with the commit-graph file if
> false.
>
> Nothing about your method is specific to the commit-graph file, since
> you provide a filename as a parameter. It could easily be "int
> file_exists(const char *filename)".
I was being paranoid about not doing this if it didn't exist but it was
something else than ENOENT (e.g. permission error?), but in retrospect
that's silly. I'll drop this helper and just use file_exists().
