On 4/10/2018 10:12 PM, Junio C Hamano wrote:
Derrick Stolee <dstolee@xxxxxxxxxxxxx> 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]
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
