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

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

quoted

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).

quoted

+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.

quoted

+
+	return strbuf_detach(&head_path, &len);
+}