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

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 30, 2018, 22:54 UTC
Message-ID: <86po2gfgpp.fsf@gmail.com>
In-Reply-To: <20180425143735.240183-10-dstolee@microsoft.com>

Derrick Stolee <dstolee@microsoft.com> writes:

Show 12 quoted lines

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

Show 6 quoted lines

>
> Without this change, a fast-forward merge would hit a BUG("bad
> generation skip") statement in commit.c during paint_down_to_common().
> This is because the HEAD commit would be loaded with "infinite"
> generation but then reached by commits with "finite" generation
> numbers.

I guess this is because we avoid re-parsing objects at all costs; we want to avoid re-reading commit graph too.

>
> Add a test to t5318-commit-graph.sh that exercises this code path to
> prevent a regression.

I would prefer if this commit was put earlier in the series, to avoid having broken Git (and thus a possibility of problems when bisecting) in between those two commits.

Show 27 quoted lines

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/merge.c         | 7 ++++---
>  t/t5318-commit-graph.sh | 9 +++++++++
>  2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 5e5e4497e3..b819756946 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1148,14 +1148,15 @@ 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);
> -

Good.

Show 19 quoted lines

>  	if (branch_mergeoptions)
>  		parse_branch_merge_options(branch_mergeoptions);
>  	argc = parse_options(argc, argv, prefix, builtin_merge_options,
> 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.

> +
>  test_done

Best,

-- 
Jakub Narębski