Re: [PATCH v2 02/10] merge: check config before loading commits

From: Derrick Stolee <stolee@gmail.com>
Date: Apr 11, 2018, 12:49 UTC
Message-ID: <197f4c77-ab2a-cf5b-463f-471bd5f8966f@gmail.com>
In-Reply-To: <xmqqo9iqze1g.fsf@gitster-ct.c.googlers.com>

On 4/10/2018 10:12 PM, Junio C Hamano wrote:

Show 25 quoted lines

> Derrick Stolee <dstolee@microsoft.com> writes:
>
>> diff --git a/builtin/merge.c b/builtin/merge.c
>> index ee050a47f3..20897f8223 100644
>> --- a/builtin/merge.c
>> +++ b/builtin/merge.c
>> @@ -1183,13 +1183,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>>   	branch = branch_to_free = resolve_refdup("HEAD", 0, &head_oid, NULL);
>>   	if (branch)
>>   		skip_prefix(branch, "refs/heads/", &branch);
>> +	init_diff_ui_defaults();
>> +	git_config(git_merge_config, NULL);
>> +
>>   	if (!branch || is_null_oid(&head_oid))
>>   		head_commit = NULL;
>>   	else
>>   		head_commit = lookup_commit_or_die(&head_oid, "HEAD");
>>   
>> -	init_diff_ui_defaults();
>> -	git_config(git_merge_config, NULL);
> Wow, that's tricky.  git_merge_config() wants to know which "branch"
> we are on, and this place is as early as we can move the call to
> without breaking things.  Is this to allow parse_object() called
> in lookup_commit_reference_gently() to know if we can rely on the
> data cached in the commit-graph data?

When I saw the bug on my machine, I tracked the issue down to a call to parse_commit_in_graph() that skipped the graph check since core_commit_graph was not set. The call stack from this call is as follows:

* lookup_commit_or_die()
* lookup_commit_reference()
* lookup_commit_reference_gently()
* parse_object()
* parse_object_buffer()
* parse_commit_in_graph() [as introduced in PATCH 01/10]

Show 7 quoted lines

>
>> Move the config load to be between the initialization of 'branch'
>> and the commit lookup. Also add a test to t5318-commit-graph.sh
>> that exercises this code path to prevent a regression.
> It is not clear to me how a successful merge of commits/8
> demonstrates that reading the config earlier than before is
> regression free.

I didn't want to introduce commits in an order that led to a commit failing tests, but if you drop the change to builtin/merge.c from this series, the tip commit will fail this test with "BUG: bad generation skip".

The reason for this failure is that commits/5 is loaded from HEAD from the object database, so its generation is marked as GENERATION_NUMBER_INFINITY, and the commit is marked as parsed. Later, the commit at merges/3 is loaded from the graph with generation 4. This triggers the BUG statement in paint_down_to_common(). That is why it is important to check a fast-forward merge.

In the 'graph_git_behavior' steps of t5318-commit-graph.sh, we were already testing 'git merge-base' to check the commit walk logic.

Thanks, -Stolee