threads / patch / 60306

patchmerge-ort: initialize repo in index state

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

## tl;dr

8 messages between Oct 5, 2023 and Oct 10, 2023. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

John Cai via GitGitGadget· Oct 5, 2023, 15:22 UTC · lore

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>
---
    merge-ort: initialize repo in index state
    
    initialize_attr_index() does not initialize the repo member of
    attr_index. Starting in 44451a2e5e (attr: teach "--attr-source=" 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.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1583%2Fjohn-cai%2Fjc%2Fpopulate-repo-when-init-attr-index-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1583/john-cai/jc/populate-repo-when-init-attr-index-v1
Pull-Request: https://github.com/git/git/pull/1583

 merge-ort.c           |  1 +
 t/t4300-merge-tree.sh | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

Show changes to 2 files +21 −0

merge-ort.c, t/t4300-merge-tree.sh

diff --git a/merge-ort.c b/merge-ort.c
index 7857ce9fbd1..172dc7d497d 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)
 	struct index_state *attr_index = &opt->priv->attr_index;
 	struct cache_entry *ce;
 
+	attr_index->repo = the_repository;
 	attr_index->initialized = 1;
 
 	if (!opt->renormalize)
diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
index 57c4f26e461..254453fff9c 100755
--- a/t/t4300-merge-tree.sh
+++ b/t/t4300-merge-tree.sh
@@ -86,6 +86,26 @@ EXPECTED
 	test_cmp expected actual
 '
 
+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 &&
+		git commit -am "adding bar" &&
+		source=$(git rev-parse HEAD) &&
+		echo baz>>file1 &&
+		git commit -am "adding baz" &&
+		merge=$(git rev-parse HEAD) &&
+		test_must_fail git --attr-source=HEAD merge-tree -z --write-tree \
+		--merge-base "$base" --end-of-options "$source" "$merge" >out &&
+		grep "Merge conflict in file1" out
+	)
+'
+
 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

-- 
gitgitgadget

Junio C Hamano· Oct 5, 2023, 20:56 UTC · re: John Cai via GitGitGadget · lore

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

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

Elijah Newren· Oct 6, 2023, 05:14 UTC · re: John Cai via GitGitGadget · lore

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

On Thu, Oct 5, 2023 at 9:14 AM John Cai via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 25 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>
> ---
>     merge-ort: initialize repo in index state
>
>     initialize_attr_index() does not initialize the repo member of
>     attr_index. Starting in 44451a2e5e (attr: teach "--attr-source=" 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.

Out of curiosity, are the changes in 44451a2e5e and its predecessors sufficient to allow us to gut this nasty initialize_attr_index() hack from merge-ort? See the comment at the top of the function and this old mailing list thread: https://lore.kernel.org/git/CABPp-BE1TvFJ1eOa8Ci5JTMET+dzZh3m3NxppqqWPyEp1UeAVg@mail.gmail.com/.

I never wanted to generate an index, Stolee didn't like it either, but the attribute code seemed hardcoded to require an index and I had gone down enough rabbit holes trying to get merge-ort into shape. But I still absolutely hate this awful hack.

If we do have to live with it still, then...

Show 17 quoted lines

> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1583%2Fjohn-cai%2Fjc%2Fpopulate-repo-when-init-attr-index-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1583/john-cai/jc/populate-repo-when-init-attr-index-v1
> Pull-Request: https://github.com/git/git/pull/1583
>
>  merge-ort.c           |  1 +
>  t/t4300-merge-tree.sh | 20 ++++++++++++++++++++
>  2 files changed, 21 insertions(+)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 7857ce9fbd1..172dc7d497d 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)
>         struct index_state *attr_index = &opt->priv->attr_index;
>         struct cache_entry *ce;
>
> +       attr_index->repo = the_repository;

Can we use opt->repo instead and reduce the number of places hardcoding the_repository?

Show 38 quoted lines

>         attr_index->initialized = 1;
>
>         if (!opt->renormalize)
> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
> index 57c4f26e461..254453fff9c 100755
> --- a/t/t4300-merge-tree.sh
> +++ b/t/t4300-merge-tree.sh
> @@ -86,6 +86,26 @@ EXPECTED
>         test_cmp expected actual
>  '
>
> +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 &&
> +               git commit -am "adding bar" &&
> +               source=$(git rev-parse HEAD) &&
> +               echo baz>>file1 &&
> +               git commit -am "adding baz" &&
> +               merge=$(git rev-parse HEAD) &&
> +               test_must_fail git --attr-source=HEAD merge-tree -z --write-tree \
> +               --merge-base "$base" --end-of-options "$source" "$merge" >out &&
> +               grep "Merge conflict in file1" out
> +       )
> +'
> +
>  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
> --
> gitgitgadget

John Cai· Oct 8, 2023, 16:12 UTC · re: Elijah Newren · lore

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

Hi Elijah,

On 6 Oct 2023, at 1:14, Elijah Newren wrote:

Show 33 quoted lines

> On Thu, Oct 5, 2023 at 9:14 AM John Cai via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> 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>
>> ---
>>     merge-ort: initialize repo in index state
>>
>>     initialize_attr_index() does not initialize the repo member of
>>     attr_index. Starting in 44451a2e5e (attr: teach "--attr-source=" 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.
>
> Out of curiosity, are the changes in 44451a2e5e and its predecessors
> sufficient to allow us to gut this nasty initialize_attr_index() hack
> from merge-ort?  See the comment at the top of the function and this
> old mailing list thread:
> https://lore.kernel.org/git/CABPp-BE1TvFJ1eOa8Ci5JTMET+dzZh3m3NxppqqWPyEp1UeAVg@mail.gmail.com/.

Honestly, I'm not familiar enough with the attr code to know if the index can be taken out of the call chain. From first glance, it looks like that would take some additional work, which I agree would be nice to do.

Show 5 quoted lines

>
> I never wanted to generate an index, Stolee didn't like it either, but
> the attribute code seemed hardcoded to require an index and I had gone
> down enough rabbit holes trying to get merge-ort into shape.  But I
> still absolutely hate this awful hack.

Perhaps a follow up patch series could address this--will keep it in mind.

Show 23 quoted lines

>
> If we do have to live with it still, then...
>
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1583%2Fjohn-cai%2Fjc%2Fpopulate-repo-when-init-attr-index-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1583/john-cai/jc/populate-repo-when-init-attr-index-v1
>> Pull-Request: https://github.com/git/git/pull/1583
>>
>>  merge-ort.c           |  1 +
>>  t/t4300-merge-tree.sh | 20 ++++++++++++++++++++
>>  2 files changed, 21 insertions(+)
>>
>> diff --git a/merge-ort.c b/merge-ort.c
>> index 7857ce9fbd1..172dc7d497d 100644
>> --- a/merge-ort.c
>> +++ b/merge-ort.c
>> @@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)
>>         struct index_state *attr_index = &opt->priv->attr_index;
>>         struct cache_entry *ce;
>>
>> +       attr_index->repo = the_repository;
>
> Can we use opt->repo instead and reduce the number of places
> hardcoding the_repository?

sounds good!

Show 40 quoted lines

>
>
>>         attr_index->initialized = 1;
>>
>>         if (!opt->renormalize)
>> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
>> index 57c4f26e461..254453fff9c 100755
>> --- a/t/t4300-merge-tree.sh
>> +++ b/t/t4300-merge-tree.sh
>> @@ -86,6 +86,26 @@ EXPECTED
>>         test_cmp expected actual
>>  '
>>
>> +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 &&
>> +               git commit -am "adding bar" &&
>> +               source=$(git rev-parse HEAD) &&
>> +               echo baz>>file1 &&
>> +               git commit -am "adding baz" &&
>> +               merge=$(git rev-parse HEAD) &&
>> +               test_must_fail git --attr-source=HEAD merge-tree -z --write-tree \
>> +               --merge-base "$base" --end-of-options "$source" "$merge" >out &&
>> +               grep "Merge conflict in file1" out
>> +       )
>> +'
>> +
>>  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
>> --
>> gitgitgadget

thanks John

John Cai via GitGitGadget· Oct 8, 2023, 16:19 UTC · re: John Cai via GitGitGadget · lore

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

From: John Cai <johncai86@gmail.com>

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>
---
    merge-ort: initialize repo in index state
    
    initialize_attr_index() does not initialize the repo member of
    attr_index. Starting in 44451a2e5e (attr: teach "--attr-source=" 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.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1583%2Fjohn-cai%2Fjc%2Fpopulate-repo-when-init-attr-index-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1583/john-cai/jc/populate-repo-when-init-attr-index-v2
Pull-Request: https://github.com/git/git/pull/1583

Range-diff vs v1:

 1:  80a18252b30 ! 1:  e178236064a merge-ort: initialize repo in index state
     @@ merge-ort.c: static void initialize_attr_index(struct merge_options *opt)
       	struct index_state *attr_index = &opt->priv->attr_index;
       	struct cache_entry *ce;
       
     -+	attr_index->repo = the_repository;
     ++	attr_index->repo = opt->repo;
       	attr_index->initialized = 1;
       
       	if (!opt->renormalize)
     @@ t/t4300-merge-tree.sh: EXPECTED
      +		test_commit initial file1 foo &&
      +		base=$(git rev-parse HEAD) &&
      +		git checkout -b brancha &&
     -+		echo bar>>file1 &&
     ++		echo bar >>file1 &&
      +		git commit -am "adding bar" &&
      +		source=$(git rev-parse HEAD) &&
     -+		echo baz>>file1 &&
     ++		git checkout @{-1} &&
     ++		git checkout -b branchb &&
     ++		echo baz >>file1 &&
      +		git commit -am "adding baz" &&
      +		merge=$(git rev-parse HEAD) &&
     -+		test_must_fail git --attr-source=HEAD merge-tree -z --write-tree \
     -+		--merge-base "$base" --end-of-options "$source" "$merge" >out &&
     -+		grep "Merge conflict in file1" out
     ++		git checkout -b gitattributes &&
     ++		test_commit "gitattributes" .gitattributes "file1 merge=union" &&
     ++		git checkout @{-1} &&
     ++		tree=$(git --attr-source=gitattributes merge-tree --write-tree \
     ++		--merge-base "$base" --end-of-options "$source" "$merge") &&
     ++		echo "foo\nbar\nbaz" >expect &&
     ++		git cat-file -p "$tree:file1" >actual &&
     ++		test_cmp expect actual
      +	)
      +'
      +

 merge-ort.c           |  1 +
 t/t4300-merge-tree.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

Show changes to 2 files +28 −0

merge-ort.c, t/t4300-merge-tree.sh

diff --git a/merge-ort.c b/merge-ort.c
index 7857ce9fbd1..36537256613 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)
 	struct index_state *attr_index = &opt->priv->attr_index;
 	struct cache_entry *ce;
 
+	attr_index->repo = opt->repo;
 	attr_index->initialized = 1;
 
 	if (!opt->renormalize)
diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
index 57c4f26e461..2929ce3bf16 100755
--- a/t/t4300-merge-tree.sh
+++ b/t/t4300-merge-tree.sh
@@ -86,6 +86,33 @@ EXPECTED
 	test_cmp expected actual
 '
 
+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 &&
+		git commit -am "adding bar" &&
+		source=$(git rev-parse HEAD) &&
+		git checkout @{-1} &&
+		git checkout -b branchb &&
+		echo baz >>file1 &&
+		git commit -am "adding baz" &&
+		merge=$(git rev-parse HEAD) &&
+		git checkout -b gitattributes &&
+		test_commit "gitattributes" .gitattributes "file1 merge=union" &&
+		git checkout @{-1} &&
+		tree=$(git --attr-source=gitattributes merge-tree --write-tree \
+		--merge-base "$base" --end-of-options "$source" "$merge") &&
+		echo "foo\nbar\nbaz" >expect &&
+		git cat-file -p "$tree:file1" >actual &&
+		test_cmp expect actual
+	)
+'
+
 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

-- 
gitgitgadget

John Cai via GitGitGadget· Oct 9, 2023, 13:21 UTC · re: John Cai via GitGitGadget · lore

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

From: John Cai <johncai86@gmail.com>

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>
---
    merge-ort: initialize repo in index state
    
    initialize_attr_index() does not initialize the repo member of
    attr_index. Starting in 44451a2e5e (attr: teach "--attr-source=" 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.
    
    Changes since V2:
    
     * fixed test by using printf instead of echo
    
    Changes since v1:
    
     * using opt->repo to avoid hardcoding another the_repository
     * clarified test

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1583%2Fjohn-cai%2Fjc%2Fpopulate-repo-when-init-attr-index-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1583/john-cai/jc/populate-repo-when-init-attr-index-v3
Pull-Request: https://github.com/git/git/pull/1583

Range-diff vs v2:

 1:  e178236064a ! 1:  792b01fa616 merge-ort: initialize repo in index state
     @@ t/t4300-merge-tree.sh: EXPECTED
      +		git checkout @{-1} &&
      +		tree=$(git --attr-source=gitattributes merge-tree --write-tree \
      +		--merge-base "$base" --end-of-options "$source" "$merge") &&
     -+		echo "foo\nbar\nbaz" >expect &&
     ++		printf "foo\nbar\nbaz\n" >expect &&
      +		git cat-file -p "$tree:file1" >actual &&
      +		test_cmp expect actual
      +	)

 merge-ort.c           |  1 +
 t/t4300-merge-tree.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

Show changes to 2 files +28 −0

merge-ort.c, t/t4300-merge-tree.sh

diff --git a/merge-ort.c b/merge-ort.c
index 7857ce9fbd1..36537256613 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)
 	struct index_state *attr_index = &opt->priv->attr_index;
 	struct cache_entry *ce;
 
+	attr_index->repo = opt->repo;
 	attr_index->initialized = 1;
 
 	if (!opt->renormalize)
diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
index 57c4f26e461..c3a03e54187 100755
--- a/t/t4300-merge-tree.sh
+++ b/t/t4300-merge-tree.sh
@@ -86,6 +86,33 @@ EXPECTED
 	test_cmp expected actual
 '
 
+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 &&
+		git commit -am "adding bar" &&
+		source=$(git rev-parse HEAD) &&
+		git checkout @{-1} &&
+		git checkout -b branchb &&
+		echo baz >>file1 &&
+		git commit -am "adding baz" &&
+		merge=$(git rev-parse HEAD) &&
+		git checkout -b gitattributes &&
+		test_commit "gitattributes" .gitattributes "file1 merge=union" &&
+		git checkout @{-1} &&
+		tree=$(git --attr-source=gitattributes merge-tree --write-tree \
+		--merge-base "$base" --end-of-options "$source" "$merge") &&
+		printf "foo\nbar\nbaz\n" >expect &&
+		git cat-file -p "$tree:file1" >actual &&
+		test_cmp expect actual
+	)
+'
+
 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

-- 
gitgitgadget

Junio C Hamano· Oct 9, 2023, 21:41 UTC · re: John Cai via GitGitGadget · lore

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

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

Show 5 quoted lines

>     Fix this by initializing the repository in the index state.
>     
>     Changes since V2:
>     
>      * fixed test by using printf instead of echo

Much better than using unportable \n with echo.

>      -+		echo "foo\nbar\nbaz" >expect &&
>      ++		printf "foo\nbar\nbaz\n" >expect &&

But if we are using printf, it would be easier to read lines separately, which would look more like

	printf "%s\n" foo bar baz >expect

And we have

	test_write_lines foo bar baz >expect

to make it even more discoverable.

Show 47 quoted lines

>       +		git cat-file -p "$tree:file1" >actual &&
>       +		test_cmp expect actual
>       +	)
>
>
>  merge-ort.c           |  1 +
>  t/t4300-merge-tree.sh | 27 +++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 7857ce9fbd1..36537256613 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)
>  	struct index_state *attr_index = &opt->priv->attr_index;
>  	struct cache_entry *ce;
>  
> +	attr_index->repo = opt->repo;
>  	attr_index->initialized = 1;
>  
>  	if (!opt->renormalize)
> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
> index 57c4f26e461..c3a03e54187 100755
> --- a/t/t4300-merge-tree.sh
> +++ b/t/t4300-merge-tree.sh
> @@ -86,6 +86,33 @@ EXPECTED
>  	test_cmp expected actual
>  '
>  
> +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 &&
> +		git commit -am "adding bar" &&
> +		source=$(git rev-parse HEAD) &&
> +		git checkout @{-1} &&
> +		git checkout -b branchb &&
> +		echo baz >>file1 &&
> +		git commit -am "adding baz" &&
> +		merge=$(git rev-parse HEAD) &&
> +		git checkout -b gitattributes &&
> +		test_commit "gitattributes" .gitattributes "file1 merge=union" &&

OK, the branch "gitattributes" will be used to drive merge of file1 using the union merge to avoid conflicting.

> +		git checkout @{-1} &&

But such attribute will only be available in that branch, not in the checked out working tree. And then

> +		tree=$(git --attr-source=gitattributes merge-tree --write-tree \
> +		--merge-base "$base" --end-of-options "$source" "$merge") &&

we use the gitattributes branch as the tree-ish to take the attribute information from. Makes sense.

> +		printf "foo\nbar\nbaz\n" >expect &&

I'll squash in the "test_write_lines" change while queuing.

Show 10 quoted lines

> +		git cat-file -p "$tree:file1" >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
>  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.  Looking good.

John Cai· Oct 10, 2023, 15:06 UTC · re: Junio C Hamano · lore

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

Hi Junio

On 9 Oct 2023, at 17:41, Junio C Hamano wrote:

Show 23 quoted lines

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>>     Fix this by initializing the repository in the index state.
>>
>>     Changes since V2:
>>
>>      * fixed test by using printf instead of echo
>
> Much better than using unportable \n with echo.
>
>>      -+		echo "foo\nbar\nbaz" >expect &&
>>      ++		printf "foo\nbar\nbaz\n" >expect &&
>
> But if we are using printf, it would be easier to read lines
> separately, which would look more like
>
> 	printf "%s\n" foo bar baz >expect
>
> And we have
>
> 	test_write_lines foo bar baz >expect
>
> to make it even more discoverable.

wasn't aware of test_write_lines, thanks.

Show 66 quoted lines

>
>>       +		git cat-file -p "$tree:file1" >actual &&
>>       +		test_cmp expect actual
>>       +	)
>>
>>
>>  merge-ort.c           |  1 +
>>  t/t4300-merge-tree.sh | 27 +++++++++++++++++++++++++++
>>  2 files changed, 28 insertions(+)
>>
>> diff --git a/merge-ort.c b/merge-ort.c
>> index 7857ce9fbd1..36537256613 100644
>> --- a/merge-ort.c
>> +++ b/merge-ort.c
>> @@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)
>>  	struct index_state *attr_index = &opt->priv->attr_index;
>>  	struct cache_entry *ce;
>>
>> +	attr_index->repo = opt->repo;
>>  	attr_index->initialized = 1;
>>
>>  	if (!opt->renormalize)
>> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
>> index 57c4f26e461..c3a03e54187 100755
>> --- a/t/t4300-merge-tree.sh
>> +++ b/t/t4300-merge-tree.sh
>> @@ -86,6 +86,33 @@ EXPECTED
>>  	test_cmp expected actual
>>  '
>>
>> +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 &&
>> +		git commit -am "adding bar" &&
>> +		source=$(git rev-parse HEAD) &&
>> +		git checkout @{-1} &&
>> +		git checkout -b branchb &&
>> +		echo baz >>file1 &&
>> +		git commit -am "adding baz" &&
>> +		merge=$(git rev-parse HEAD) &&
>> +		git checkout -b gitattributes &&
>> +		test_commit "gitattributes" .gitattributes "file1 merge=union" &&
>
> OK, the branch "gitattributes" will be used to drive merge of file1
> using the union merge to avoid conflicting.
>
>> +		git checkout @{-1} &&
>
> But such attribute will only be available in that branch, not in the
> checked out working tree.  And then
>
>> +		tree=$(git --attr-source=gitattributes merge-tree --write-tree \
>> +		--merge-base "$base" --end-of-options "$source" "$merge") &&
>
> we use the gitattributes branch as the tree-ish to take the
> attribute information from.  Makes sense.
>
>> +		printf "foo\nbar\nbaz\n" >expect &&
>
> I'll squash in the "test_write_lines" change while queuing.

thank you! John

Show 13 quoted lines

>
>> +		git cat-file -p "$tree:file1" >actual &&
>> +		test_cmp expect actual
>> +	)
>> +'
>> +
>>  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.  Looking good.