Re: [PATCH] merge-ort: initialize repo in index state

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 5, 2023, 20:56 UTC
Message-ID: <xmqqv8bkhjp1.fsf@gitster.g>
In-Reply-To: <pull.1583.git.git.1696519349407.gitgitgadget@gmail.com>

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 14 quoted lines

> From: John Cai <johncai86@gmail.com>
>
> initialize_attr_index() does not initialize the repo member of
> attr_index. Starting in 44451a2e5e (attr: teach "--attr-source=<tree>"
> global option to "git", 2023-05-06), this became a problem because
> istate->repo gets passed down the call chain starting in
> git_check_attr(). This gets passed all the way down to
> replace_refs_enabled(), which segfaults when accessing r->gitdir.
>
> Fix this by initializing the repository in the index state.
>
> Signed-off-by: John Cai <johncai86@gmail.com>
> Helped-by: Christian Couder <christian.couder@gmail.com>
> ---

Nice spotting.

Show 9 quoted lines

> +test_expect_success '3-way merge with --attr-source' '
> +	test_when_finished rm -rf 3-way &&
> +	git init 3-way &&
> +	(
> +		cd 3-way &&
> +		test_commit initial file1 foo &&
> +		base=$(git rev-parse HEAD) &&
> +		git checkout -b brancha &&
> +		echo bar>>file1 &&

We need a space before but not after ">>".

> +		git commit -am "adding bar" &&
> +		source=$(git rev-parse HEAD) &&
> +		echo baz>>file1 &&

Ditto.

> +		git commit -am "adding baz" &&
> +		merge=$(git rev-parse HEAD) &&

Sorry, but I got lost. We have the $base commit on the default initial branch, from which forked branch-A which we created two commits to add lines "bar" and "baz" to file1. We are calling the tip of this branch-A $merge, and the parent of $merge is called $source.

> +		test_must_fail git --attr-source=HEAD merge-tree -z --write-tree \
> +		--merge-base "$base" --end-of-options "$source" "$merge" >out &&

So, is this asking "merge-tree" to merge "$source" and "$merge", one of which is the direct parent of the other one? Aren't we missing a "checkout @{-1}" before we add "baz" or something?

If the attitude taken by this test is "we do not really care if the attempted merge is meaningless and would never happen in the real world, as the only thing we care is to see "git" not to segfault", then we probably shouldn't even check ...

> +		grep "Merge conflict in file1" out

... if we failed due to conflict with a "grep" like this. As "out" is a binary file (thanks to the use of "-z" on the command line of "merge-tree" invocation), I am not sure if you can rely on "grep" to find this error message in 'out', depending on your implementation of "grep".

On the other hand, the new test can do a more realistic merge between two commits, where having an attribute in some tree object (which preferrably is *not* HEAD and .gitattribute does not exist in the working tree) given to the --attr-source option does make a difference, and verify the contents of the "file1" recorded in the resulting tree. That way, the test can verify that the attributes are read from the right place without segfaulting.

Show 8 quoted lines

> +	)
> +'
> +
>  test_expect_success 'file change A, B (same)' '
>  	git reset --hard initial &&
>  	test_commit "change-a-b-same-A" "initial-file" "AAA" &&
>
> base-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09

Thanks.