Re: [PATCH v4 09/10] merge: check config before loading commits

Derrick Stolee [off-list ref] writes:

On 4/30/2018 6:54 PM, Jakub Narebski wrote:

quoted

Derrick Stolee [off-list ref] writes:

quoted

Now that we use generation numbers from the commit-graph, we must
ensure that all commits that exist in the commit-graph are loaded
from that file instead of from the object database. Since the
commit-graph file is only checked if core.commitGraph is true, we
must check the default config before we load any commits.

In the merge builtin, the config was checked after loading the HEAD
commit. This was due to the use of the global 'branch' when checking
merge-specific config settings.

Move the config load to be between the initialization of 'branch' and
the commit lookup.

Sidenote: I wonder why reading config was postponed to later in the
command lifetime... I guess it was to avoid having to read config if
HEAD was invalid.

The 'branch' does need to be loaded before the call to git_config (as
I found out after moving the config call too early), so I suppose it
was natural to pair that with resolving head_commit.

Right, so there was only a limited number of places where call to
git_config could be put correctly. Now I wonder no more.

[...]

quoted

quoted

diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index a380419b65..77d85aefe7 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh

@@ -221,4 +221,13 @@ test_expect_success 'write graph in bare repo' '
  graph_git_behavior 'bare repo with graph, commit 8 vs merge 1' bare commits/8 merge/1
  graph_git_behavior 'bare repo with graph, commit 8 vs merge 2' bare commits/8 merge/2
  +test_expect_success 'perform fast-forward merge in full repo' '
+	cd "$TRASH_DIRECTORY/full" &&
+	git checkout -b merge-5-to-8 commits/5 &&
+	git merge commits/8 &&
+	git show-ref -s merge-5-to-8 >output &&
+	git show-ref -s commits/8 >expect &&
+	test_cmp expect output
+'

All right.  (though I wonder if this tests catches all problems where
BUG("bad generation skip") could have been encountered.

We will never know until we have this series running in the wild (and
even then, some features are very obscure) and enough people turn on
the config setting.

One goal of the "fsck and gc" series is to get this feature running
during the rest of the test suite as much as possible, so we can get
additional coverage. Also to get more experience from the community
dogfooding the feature.

Sidenote: for two out of three features that change the view of history
we could also update commit-graph automatically:
* the shortening or deepening of shallow clone could also re-calculate
  the commit graph (or invalidate it)
* git-replace could check if the replacement modifies history, and if
  so, recalculate the commit graph (or invalidate it/check its validity)
* there is no such possibility for grafts, but they are deprecated anyway

-- 
Jakub Narębski