Question about get_cached_commit_buffer()

From: Derrick Stolee <stolee@gmail.com>
Date: Feb 20, 2018, 22:12 UTC
Message-ID: <ecbbe515-b7a8-3dc8-7d14-32412e7b12c3@gmail.com>

While working on my commit-graph patch [1] and using a local build in my usual workflows, I found a bug in my branch.

Essentially, when calling `git rev-list --header`, the header information is actually missing for many commits except the first. This was not caught in my testing since t6000-rev-list-misc.sh creates a test repo with only one commit.

The root cause is that the serialized commit graph gets its speedup by not loading buffers for every commit. For many use-cases (merge bases, --topo-order, etc.) we do not need the buffer for most of the commits. In the rev-list example, the buffer is loaded due to a side-effect of being referenced by HEAD or a branch. A similar effect happened in `git log`, hence the following change in my patch:

diff --git a/log-tree.c b/log-tree.c
index 580b3a9..14735d4 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -647,8 +647,7 @@ void show_log(struct rev_info *opt)
  		show_mergetag(opt, commit);
  	}
  
-	if (!get_cached_commit_buffer(commit, NULL))
-		return;
+	get_commit_buffer(commit, NULL);
  
  	if (opt->show_notes) {
  		int raw;


In rev-list, the "--header" option outputs a value and expects the 
buffer to be cached. It outputs the header info only if 
get_cached_commit_buffer() returns a non-null buffer, giving incorrect 
output. If it called get_commit_buffer() instead, it would immediately 
call get_cached_commit_buffer() and on failure actually load the buffer.

This has not been a problem before, since the buffer was always loaded 
at some point for each commit (and saved because of the 
save_commit_buffer global).

I propose to make get_cached_commit_buffer() static to commit.c and 
convert all callers to get_commit_buffer(). Is there any reason to _not_ 
do this? It seems that there is no functional or performance change.

After the serialized commit graph exists and is used, the only change is 
that we delay loading the buffer until we need to read the commit 
metadata that is not stored in the graph (message, author/committer).

Thanks,
-Stolee

[1] 
https://public-inbox.org/git/4d1ee202-7d79-d73c-6e05-d0fc85db943c@gmail.com/T/#m381bfd3f2eafbd254e35a5147cd198bc35055e92
     [Patch v4 00/14] Serialized Git Commit Graph

[2] 
https://public-inbox.org/git/20140610214039.GJ19147@sigill.intra.peff.net/
     [Patch 10/17] provide helpers to access the commit buffer