Re: [PATCH v2 04/14] commit-graph: implement construct_commit_graph()

From: Derrick Stolee <stolee@gmail.com>
Date: Feb 5, 2018, 16:06 UTC
Message-ID: <b7f45961-35c8-6e13-646c-9574fb5d56da@gmail.com>
In-Reply-To: <20180202153212.29746-1-szeder.dev@gmail.com>

On 2/2/2018 10:32 AM, SZEDER Gábor wrote:

Show 25 quoted lines

>> Teach Git to write a commit graph file by checking all packed objects
>> to see if they are commits, then store the file in the given pack
>> directory.
> I'm afraid that scanning all packed objects is a bit of a roundabout
> way to approach this.
>
> In my git repo, with 9 pack files at the moment, i.e. not that big a
> repo and not that many pack files:
>
>    $ time ./git commit-graph --write --update-head
>    4df41a3d1cc408b7ad34bea87b51ec4ccbf4b803
>
>    real    0m27.550s
>    user    0m27.113s
>    sys     0m0.376s
>
> In comparison, performing a good old revision walk to gather all the
> info that is written into the graph file:
>
>    $ time git log --all --topo-order --format='%H %T %P %cd' |wc -l
>    52954
>
>    real    0m0.903s
>    user    0m0.972s
>    sys     0m0.058s

Two reasons this is in here:

(1) It's easier to get the write implemented this way and add the reachable closure later (which I do).

(2) For GVFS, we want to add all commits that arrived in a "prefetch pack" to the graph even if we do not have a ref that points to the commit yet. We expect many commits to become reachable soon and having them in the graph saves a lot of time in merge-base calculations.

So, (1) is for patch simplicity, and (2) is why I want it to be an option in the final version. See the --stdin-packs argument later for a way to do this incrementally.

I expect almost all users to use the reachable closure method with --stdin-commits (and that's how I will integrate automatic updates with 'fetch', 'repack', and 'gc' in a later patch).

Show 16 quoted lines

>
>> +char* get_commit_graph_filename_hash(const char *pack_dir,
>> +				     struct object_id *hash)
>> +{
>> +	size_t len;
>> +	struct strbuf head_path = STRBUF_INIT;
>> +	strbuf_addstr(&head_path, pack_dir);
>> +	strbuf_addstr(&head_path, "/graph-");
>> +	strbuf_addstr(&head_path, oid_to_hex(hash));
>> +	strbuf_addstr(&head_path, ".graph");
> Nit: this is assembling the path of a graph file, not that of a
> graph-head, so the strbuf should be renamed accordingly.
>
>> +
>> +	return strbuf_detach(&head_path, &len);
>> +}