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

[Derrick Stolee <stolee@gmail.com>]

On 2/2/2018 10:32 AM, SZEDER Gábor wrote:
>> 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).

>
>> +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);
>> +}