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

[Jakub Narebski <jnareb@gmail.com>]

Derrick Stolee <stolee@gmail.com> writes:
> On 4/30/2018 6:54 PM, Jakub Narebski wrote:
>> Derrick Stolee <dstolee@microsoft.com> writes:
>>
>>> 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.

[...]
>>> 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