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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

> +	)
> +'
> +
>  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.