{"thread":{"id":"60306","subject":"[PATCH] merge-ort: initialize repo in index state","startedAt":"2023-10-05T16:12:21Z","lastAt":"2023-10-10T15:06:51Z","messageCount":8,"participants":["John Cai via GitGitGadget","Junio C Hamano","Elijah Newren","John Cai"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"482689","messageId":"pull.1583.git.git.1696519349407.gitgitgadget@gmail.com","threadId":"60306","inReplyTo":null,"subject":"[PATCH] merge-ort: initialize repo in index state","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-05T15:22:29Z","receivedAt":"2023-10-05T16:12:21Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\ninitialize_attr_index() does not initialize the repo member of\nattr_index. Starting in 44451a2e5e (attr: teach \"--attr-source=<tree>\"\nglobal option to \"git\", 2023-05-06), this became a problem because\nistate->repo gets passed down the call chain starting in\ngit_check_attr(). This gets passed all the way down to\nreplace_refs_enabled(), which segfaults when accessing r->gitdir.\n\nFix this by initializing the repository in the index state.\n\nSigned-off-by: John Cai <johncai86@gmail.com>\nHelped-by: Christian Couder <christian.couder@gmail.com>\n---\n    merge-ort: initialize repo in index state\n    \n    initialize_attr_index() does not initialize the repo member of\n    attr_index. Starting in 44451a2e5e (attr: teach \"--attr-source=\" global\n    option to \"git\", 2023-05-06), this became a problem because istate->repo\n    gets passed down the call chain starting in git_check_attr(). This gets\n    passed all the way down to replace_refs_enabled(), which segfaults when\n    accessing r->gitdir.\n    \n    Fix this by initializing the repository in the index state.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1583%2Fjohn-cai%2Fjc%2Fpopulate-repo-when-init-attr-index-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1583/john-cai/jc/populate-repo-when-init-attr-index-v1\nPull-Request: https://github.com/git/git/pull/1583\n\n merge-ort.c           |  1 +\n t/t4300-merge-tree.sh | 20 ++++++++++++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 7857ce9fbd1..172dc7d497d 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)\n \tstruct index_state *attr_index = &opt->priv->attr_index;\n \tstruct cache_entry *ce;\n \n+\tattr_index->repo = the_repository;\n \tattr_index->initialized = 1;\n \n \tif (!opt->renormalize)\ndiff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\nindex 57c4f26e461..254453fff9c 100755\n--- a/t/t4300-merge-tree.sh\n+++ b/t/t4300-merge-tree.sh\n@@ -86,6 +86,26 @@ EXPECTED\n \ttest_cmp expected actual\n '\n \n+test_expect_success '3-way merge with --attr-source' '\n+\ttest_when_finished rm -rf 3-way &&\n+\tgit init 3-way &&\n+\t(\n+\t\tcd 3-way &&\n+\t\ttest_commit initial file1 foo &&\n+\t\tbase=$(git rev-parse HEAD) &&\n+\t\tgit checkout -b brancha &&\n+\t\techo bar>>file1 &&\n+\t\tgit commit -am \"adding bar\" &&\n+\t\tsource=$(git rev-parse HEAD) &&\n+\t\techo baz>>file1 &&\n+\t\tgit commit -am \"adding baz\" &&\n+\t\tmerge=$(git rev-parse HEAD) &&\n+\t\ttest_must_fail git --attr-source=HEAD merge-tree -z --write-tree \\\n+\t\t--merge-base \"$base\" --end-of-options \"$source\" \"$merge\" >out &&\n+\t\tgrep \"Merge conflict in file1\" out\n+\t)\n+'\n+\n test_expect_success 'file change A, B (same)' '\n \tgit reset --hard initial &&\n \ttest_commit \"change-a-b-same-A\" \"initial-file\" \"AAA\" &&\n\nbase-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09\n-- \ngitgitgadget\n"},{"id":"482714","messageId":"xmqqv8bkhjp1.fsf@gitster.g","threadId":"60306","inReplyTo":"pull.1583.git.git.1696519349407.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-ort: initialize repo in index state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-05T20:56:42Z","receivedAt":"2023-10-05T20:57:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: John Cai <johncai86@gmail.com>\n>\n> initialize_attr_index() does not initialize the repo member of\n> attr_index. Starting in 44451a2e5e (attr: teach \"--attr-source=<tree>\"\n> global option to \"git\", 2023-05-06), this became a problem because\n> istate->repo gets passed down the call chain starting in\n> git_check_attr(). This gets passed all the way down to\n> replace_refs_enabled(), which segfaults when accessing r->gitdir.\n>\n> Fix this by initializing the repository in the index state.\n>\n> Signed-off-by: John Cai <johncai86@gmail.com>\n> Helped-by: Christian Couder <christian.couder@gmail.com>\n> ---\n\nNice spotting.\n\n> +test_expect_success '3-way merge with --attr-source' '\n> +\ttest_when_finished rm -rf 3-way &&\n> +\tgit init 3-way &&\n> +\t(\n> +\t\tcd 3-way &&\n> +\t\ttest_commit initial file1 foo &&\n> +\t\tbase=$(git rev-parse HEAD) &&\n> +\t\tgit checkout -b brancha &&\n> +\t\techo bar>>file1 &&\n\nWe need a space before but not after \">>\".\n\n> +\t\tgit commit -am \"adding bar\" &&\n> +\t\tsource=$(git rev-parse HEAD) &&\n> +\t\techo baz>>file1 &&\n\nDitto.\n\n> +\t\tgit commit -am \"adding baz\" &&\n> +\t\tmerge=$(git rev-parse HEAD) &&\n\nSorry, but I got lost.  We have the $base commit on the default\ninitial branch, from which forked branch-A which we created two\ncommits to add lines \"bar\" and \"baz\" to file1.  We are calling the\ntip of this branch-A $merge, and the parent of $merge is called\n$source.\n\n> +\t\ttest_must_fail git --attr-source=HEAD merge-tree -z --write-tree \\\n> +\t\t--merge-base \"$base\" --end-of-options \"$source\" \"$merge\" >out &&\n\nSo, is this asking \"merge-tree\" to merge \"$source\" and \"$merge\", one\nof which is the direct parent of the other one?  Aren't we missing a\n\"checkout @{-1}\" before we add \"baz\" or something?\n\nIf the attitude taken by this test is \"we do not really care if the\nattempted merge is meaningless and would never happen in the real\nworld, as the only thing we care is to see \"git\" not to segfault\",\nthen we probably shouldn't even check ...\n\n> +\t\tgrep \"Merge conflict in file1\" out\n\n... if we failed due to conflict with a \"grep\" like this.  As \"out\"\nis a binary file (thanks to the use of \"-z\" on the command line of\n\"merge-tree\" invocation), I am not sure if you can rely on \"grep\" to\nfind this error message in 'out', depending on your implementation\nof \"grep\".\n\nOn the other hand, the new test can do a more realistic merge\nbetween two commits, where having an attribute in some tree object\n(which preferrably is *not* HEAD and .gitattribute does not exist in\nthe working tree) given to the --attr-source option does make a\ndifference, and verify the contents of the \"file1\" recorded in the\nresulting tree.  That way, the test can verify that the attributes\nare read from the right place without segfaulting.\n\n> +\t)\n> +'\n> +\n>  test_expect_success 'file change A, B (same)' '\n>  \tgit reset --hard initial &&\n>  \ttest_commit \"change-a-b-same-A\" \"initial-file\" \"AAA\" &&\n>\n> base-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09\n\nThanks.\n"},{"id":"482735","messageId":"CABPp-BG_cQ011a15HSVRtJDq7MxgqY2_tD8aZm0Qc4F6ZU0NPA@mail.gmail.com","threadId":"60306","inReplyTo":"pull.1583.git.git.1696519349407.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-ort: initialize repo in index state","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-10-06T05:14:38Z","receivedAt":"2023-10-06T05:15:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Oct 5, 2023 at 9:14 AM John Cai via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: John Cai <johncai86@gmail.com>\n>\n> initialize_attr_index() does not initialize the repo member of\n> attr_index. Starting in 44451a2e5e (attr: teach \"--attr-source=<tree>\"\n> global option to \"git\", 2023-05-06), this became a problem because\n> istate->repo gets passed down the call chain starting in\n> git_check_attr(). This gets passed all the way down to\n> replace_refs_enabled(), which segfaults when accessing r->gitdir.\n>\n> Fix this by initializing the repository in the index state.\n>\n> Signed-off-by: John Cai <johncai86@gmail.com>\n> Helped-by: Christian Couder <christian.couder@gmail.com>\n> ---\n>     merge-ort: initialize repo in index state\n>\n>     initialize_attr_index() does not initialize the repo member of\n>     attr_index. Starting in 44451a2e5e (attr: teach \"--attr-source=\" global\n>     option to \"git\", 2023-05-06), this became a problem because istate->repo\n>     gets passed down the call chain starting in git_check_attr(). This gets\n>     passed all the way down to replace_refs_enabled(), which segfaults when\n>     accessing r->gitdir.\n>\n>     Fix this by initializing the repository in the index state.\n\nOut of curiosity, are the changes in 44451a2e5e and its predecessors\nsufficient to allow us to gut this nasty initialize_attr_index() hack\nfrom merge-ort?  See the comment at the top of the function and this\nold mailing list thread:\nhttps://lore.kernel.org/git/CABPp-BE1TvFJ1eOa8Ci5JTMET+dzZh3m3NxppqqWPyEp1UeAVg@mail.gmail.com/.\n\nI never wanted to generate an index, Stolee didn't like it either, but\nthe attribute code seemed hardcoded to require an index and I had gone\ndown enough rabbit holes trying to get merge-ort into shape.  But I\nstill absolutely hate this awful hack.\n\nIf we do have to live with it still, then...\n\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1583%2Fjohn-cai%2Fjc%2Fpopulate-repo-when-init-attr-index-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1583/john-cai/jc/populate-repo-when-init-attr-index-v1\n> Pull-Request: https://github.com/git/git/pull/1583\n>\n>  merge-ort.c           |  1 +\n>  t/t4300-merge-tree.sh | 20 ++++++++++++++++++++\n>  2 files changed, 21 insertions(+)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 7857ce9fbd1..172dc7d497d 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)\n>         struct index_state *attr_index = &opt->priv->attr_index;\n>         struct cache_entry *ce;\n>\n> +       attr_index->repo = the_repository;\n\nCan we use opt->repo instead and reduce the number of places\nhardcoding the_repository?\n\n\n>         attr_index->initialized = 1;\n>\n>         if (!opt->renormalize)\n> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\n> index 57c4f26e461..254453fff9c 100755\n> --- a/t/t4300-merge-tree.sh\n> +++ b/t/t4300-merge-tree.sh\n> @@ -86,6 +86,26 @@ EXPECTED\n>         test_cmp expected actual\n>  '\n>\n> +test_expect_success '3-way merge with --attr-source' '\n> +       test_when_finished rm -rf 3-way &&\n> +       git init 3-way &&\n> +       (\n> +               cd 3-way &&\n> +               test_commit initial file1 foo &&\n> +               base=$(git rev-parse HEAD) &&\n> +               git checkout -b brancha &&\n> +               echo bar>>file1 &&\n> +               git commit -am \"adding bar\" &&\n> +               source=$(git rev-parse HEAD) &&\n> +               echo baz>>file1 &&\n> +               git commit -am \"adding baz\" &&\n> +               merge=$(git rev-parse HEAD) &&\n> +               test_must_fail git --attr-source=HEAD merge-tree -z --write-tree \\\n> +               --merge-base \"$base\" --end-of-options \"$source\" \"$merge\" >out &&\n> +               grep \"Merge conflict in file1\" out\n> +       )\n> +'\n> +\n>  test_expect_success 'file change A, B (same)' '\n>         git reset --hard initial &&\n>         test_commit \"change-a-b-same-A\" \"initial-file\" \"AAA\" &&\n>\n> base-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09\n> --\n> gitgitgadget\n"},{"id":"482825","messageId":"BA926591-DC68-4929-AC20-54B019A43183@gmail.com","threadId":"60306","inReplyTo":"CABPp-BG_cQ011a15HSVRtJDq7MxgqY2_tD8aZm0Qc4F6ZU0NPA@mail.gmail.com","subject":"Re: [PATCH] merge-ort: initialize repo in index state","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-10-08T16:12:32Z","receivedAt":"2023-10-08T16:12:38Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Elijah,\n\nOn 6 Oct 2023, at 1:14, Elijah Newren wrote:\n\n> On Thu, Oct 5, 2023 at 9:14 AM John Cai via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>>\n>> From: John Cai <johncai86@gmail.com>\n>>\n>> initialize_attr_index() does not initialize the repo member of\n>> attr_index. Starting in 44451a2e5e (attr: teach \"--attr-source=<tree>\"\n>> global option to \"git\", 2023-05-06), this became a problem because\n>> istate->repo gets passed down the call chain starting in\n>> git_check_attr(). This gets passed all the way down to\n>> replace_refs_enabled(), which segfaults when accessing r->gitdir.\n>>\n>> Fix this by initializing the repository in the index state.\n>>\n>> Signed-off-by: John Cai <johncai86@gmail.com>\n>> Helped-by: Christian Couder <christian.couder@gmail.com>\n>> ---\n>>     merge-ort: initialize repo in index state\n>>\n>>     initialize_attr_index() does not initialize the repo member of\n>>     attr_index. Starting in 44451a2e5e (attr: teach \"--attr-source=\" global\n>>     option to \"git\", 2023-05-06), this became a problem because istate->repo\n>>     gets passed down the call chain starting in git_check_attr(). This gets\n>>     passed all the way down to replace_refs_enabled(), which segfaults when\n>>     accessing r->gitdir.\n>>\n>>     Fix this by initializing the repository in the index state.\n>\n> Out of curiosity, are the changes in 44451a2e5e and its predecessors\n> sufficient to allow us to gut this nasty initialize_attr_index() hack\n> from merge-ort?  See the comment at the top of the function and this\n> old mailing list thread:\n> https://lore.kernel.org/git/CABPp-BE1TvFJ1eOa8Ci5JTMET+dzZh3m3NxppqqWPyEp1UeAVg@mail.gmail.com/.\n\nHonestly, I'm not familiar enough with the attr code to know if the index can be\ntaken out of the call chain. From first glance, it looks like that would take\nsome additional work, which I agree would be nice to do.\n\n>\n> I never wanted to generate an index, Stolee didn't like it either, but\n> the attribute code seemed hardcoded to require an index and I had gone\n> down enough rabbit holes trying to get merge-ort into shape.  But I\n> still absolutely hate this awful hack.\n\nPerhaps a follow up patch series could address this--will keep it in mind.\n\n>\n> If we do have to live with it still, then...\n>\n>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1583%2Fjohn-cai%2Fjc%2Fpopulate-repo-when-init-attr-index-v1\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1583/john-cai/jc/populate-repo-when-init-attr-index-v1\n>> Pull-Request: https://github.com/git/git/pull/1583\n>>\n>>  merge-ort.c           |  1 +\n>>  t/t4300-merge-tree.sh | 20 ++++++++++++++++++++\n>>  2 files changed, 21 insertions(+)\n>>\n>> diff --git a/merge-ort.c b/merge-ort.c\n>> index 7857ce9fbd1..172dc7d497d 100644\n>> --- a/merge-ort.c\n>> +++ b/merge-ort.c\n>> @@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)\n>>         struct index_state *attr_index = &opt->priv->attr_index;\n>>         struct cache_entry *ce;\n>>\n>> +       attr_index->repo = the_repository;\n>\n> Can we use opt->repo instead and reduce the number of places\n> hardcoding the_repository?\n\nsounds good!\n\n>\n>\n>>         attr_index->initialized = 1;\n>>\n>>         if (!opt->renormalize)\n>> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\n>> index 57c4f26e461..254453fff9c 100755\n>> --- a/t/t4300-merge-tree.sh\n>> +++ b/t/t4300-merge-tree.sh\n>> @@ -86,6 +86,26 @@ EXPECTED\n>>         test_cmp expected actual\n>>  '\n>>\n>> +test_expect_success '3-way merge with --attr-source' '\n>> +       test_when_finished rm -rf 3-way &&\n>> +       git init 3-way &&\n>> +       (\n>> +               cd 3-way &&\n>> +               test_commit initial file1 foo &&\n>> +               base=$(git rev-parse HEAD) &&\n>> +               git checkout -b brancha &&\n>> +               echo bar>>file1 &&\n>> +               git commit -am \"adding bar\" &&\n>> +               source=$(git rev-parse HEAD) &&\n>> +               echo baz>>file1 &&\n>> +               git commit -am \"adding baz\" &&\n>> +               merge=$(git rev-parse HEAD) &&\n>> +               test_must_fail git --attr-source=HEAD merge-tree -z --write-tree \\\n>> +               --merge-base \"$base\" --end-of-options \"$source\" \"$merge\" >out &&\n>> +               grep \"Merge conflict in file1\" out\n>> +       )\n>> +'\n>> +\n>>  test_expect_success 'file change A, B (same)' '\n>>         git reset --hard initial &&\n>>         test_commit \"change-a-b-same-A\" \"initial-file\" \"AAA\" &&\n>>\n>> base-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09\n>> --\n>> gitgitgadget\n\nthanks\nJohn\n"},{"id":"482826","messageId":"pull.1583.v2.git.git.1696781998420.gitgitgadget@gmail.com","threadId":"60306","inReplyTo":"pull.1583.git.git.1696519349407.gitgitgadget@gmail.com","subject":"[PATCH v2] merge-ort: initialize repo in index state","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-08T16:19:58Z","receivedAt":"2023-10-08T16:20:11Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\ninitialize_attr_index() does not initialize the repo member of\nattr_index. Starting in 44451a2e5e (attr: teach \"--attr-source=<tree>\"\nglobal option to \"git\", 2023-05-06), this became a problem because\nistate->repo gets passed down the call chain starting in\ngit_check_attr(). This gets passed all the way down to\nreplace_refs_enabled(), which segfaults when accessing r->gitdir.\n\nFix this by initializing the repository in the index state.\n\nSigned-off-by: John Cai <johncai86@gmail.com>\nHelped-by: Christian Couder <christian.couder@gmail.com>\n---\n    merge-ort: initialize repo in index state\n    \n    initialize_attr_index() does not initialize the repo member of\n    attr_index. Starting in 44451a2e5e (attr: teach \"--attr-source=\" global\n    option to \"git\", 2023-05-06), this became a problem because istate->repo\n    gets passed down the call chain starting in git_check_attr(). This gets\n    passed all the way down to replace_refs_enabled(), which segfaults when\n    accessing r->gitdir.\n    \n    Fix this by initializing the repository in the index state.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1583%2Fjohn-cai%2Fjc%2Fpopulate-repo-when-init-attr-index-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1583/john-cai/jc/populate-repo-when-init-attr-index-v2\nPull-Request: https://github.com/git/git/pull/1583\n\nRange-diff vs v1:\n\n 1:  80a18252b30 ! 1:  e178236064a merge-ort: initialize repo in index state\n     @@ merge-ort.c: static void initialize_attr_index(struct merge_options *opt)\n       \tstruct index_state *attr_index = &opt->priv->attr_index;\n       \tstruct cache_entry *ce;\n       \n     -+\tattr_index->repo = the_repository;\n     ++\tattr_index->repo = opt->repo;\n       \tattr_index->initialized = 1;\n       \n       \tif (!opt->renormalize)\n     @@ t/t4300-merge-tree.sh: EXPECTED\n      +\t\ttest_commit initial file1 foo &&\n      +\t\tbase=$(git rev-parse HEAD) &&\n      +\t\tgit checkout -b brancha &&\n     -+\t\techo bar>>file1 &&\n     ++\t\techo bar >>file1 &&\n      +\t\tgit commit -am \"adding bar\" &&\n      +\t\tsource=$(git rev-parse HEAD) &&\n     -+\t\techo baz>>file1 &&\n     ++\t\tgit checkout @{-1} &&\n     ++\t\tgit checkout -b branchb &&\n     ++\t\techo baz >>file1 &&\n      +\t\tgit commit -am \"adding baz\" &&\n      +\t\tmerge=$(git rev-parse HEAD) &&\n     -+\t\ttest_must_fail git --attr-source=HEAD merge-tree -z --write-tree \\\n     -+\t\t--merge-base \"$base\" --end-of-options \"$source\" \"$merge\" >out &&\n     -+\t\tgrep \"Merge conflict in file1\" out\n     ++\t\tgit checkout -b gitattributes &&\n     ++\t\ttest_commit \"gitattributes\" .gitattributes \"file1 merge=union\" &&\n     ++\t\tgit checkout @{-1} &&\n     ++\t\ttree=$(git --attr-source=gitattributes merge-tree --write-tree \\\n     ++\t\t--merge-base \"$base\" --end-of-options \"$source\" \"$merge\") &&\n     ++\t\techo \"foo\\nbar\\nbaz\" >expect &&\n     ++\t\tgit cat-file -p \"$tree:file1\" >actual &&\n     ++\t\ttest_cmp expect actual\n      +\t)\n      +'\n      +\n\n\n merge-ort.c           |  1 +\n t/t4300-merge-tree.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 7857ce9fbd1..36537256613 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)\n \tstruct index_state *attr_index = &opt->priv->attr_index;\n \tstruct cache_entry *ce;\n \n+\tattr_index->repo = opt->repo;\n \tattr_index->initialized = 1;\n \n \tif (!opt->renormalize)\ndiff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\nindex 57c4f26e461..2929ce3bf16 100755\n--- a/t/t4300-merge-tree.sh\n+++ b/t/t4300-merge-tree.sh\n@@ -86,6 +86,33 @@ EXPECTED\n \ttest_cmp expected actual\n '\n \n+test_expect_success '3-way merge with --attr-source' '\n+\ttest_when_finished rm -rf 3-way &&\n+\tgit init 3-way &&\n+\t(\n+\t\tcd 3-way &&\n+\t\ttest_commit initial file1 foo &&\n+\t\tbase=$(git rev-parse HEAD) &&\n+\t\tgit checkout -b brancha &&\n+\t\techo bar >>file1 &&\n+\t\tgit commit -am \"adding bar\" &&\n+\t\tsource=$(git rev-parse HEAD) &&\n+\t\tgit checkout @{-1} &&\n+\t\tgit checkout -b branchb &&\n+\t\techo baz >>file1 &&\n+\t\tgit commit -am \"adding baz\" &&\n+\t\tmerge=$(git rev-parse HEAD) &&\n+\t\tgit checkout -b gitattributes &&\n+\t\ttest_commit \"gitattributes\" .gitattributes \"file1 merge=union\" &&\n+\t\tgit checkout @{-1} &&\n+\t\ttree=$(git --attr-source=gitattributes merge-tree --write-tree \\\n+\t\t--merge-base \"$base\" --end-of-options \"$source\" \"$merge\") &&\n+\t\techo \"foo\\nbar\\nbaz\" >expect &&\n+\t\tgit cat-file -p \"$tree:file1\" >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'file change A, B (same)' '\n \tgit reset --hard initial &&\n \ttest_commit \"change-a-b-same-A\" \"initial-file\" \"AAA\" &&\n\nbase-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09\n-- \ngitgitgadget\n"},{"id":"482861","messageId":"pull.1583.v3.git.git.1696857660374.gitgitgadget@gmail.com","threadId":"60306","inReplyTo":"pull.1583.v2.git.git.1696781998420.gitgitgadget@gmail.com","subject":"[PATCH v3] merge-ort: initialize repo in index state","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-09T13:21:00Z","receivedAt":"2023-10-09T13:21:20Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\ninitialize_attr_index() does not initialize the repo member of\nattr_index. Starting in 44451a2e5e (attr: teach \"--attr-source=<tree>\"\nglobal option to \"git\", 2023-05-06), this became a problem because\nistate->repo gets passed down the call chain starting in\ngit_check_attr(). This gets passed all the way down to\nreplace_refs_enabled(), which segfaults when accessing r->gitdir.\n\nFix this by initializing the repository in the index state.\n\nSigned-off-by: John Cai <johncai86@gmail.com>\nHelped-by: Christian Couder <christian.couder@gmail.com>\n---\n    merge-ort: initialize repo in index state\n    \n    initialize_attr_index() does not initialize the repo member of\n    attr_index. Starting in 44451a2e5e (attr: teach \"--attr-source=\" global\n    option to \"git\", 2023-05-06), this became a problem because istate->repo\n    gets passed down the call chain starting in git_check_attr(). This gets\n    passed all the way down to replace_refs_enabled(), which segfaults when\n    accessing r->gitdir.\n    \n    Fix this by initializing the repository in the index state.\n    \n    Changes since V2:\n    \n     * fixed test by using printf instead of echo\n    \n    Changes since v1:\n    \n     * using opt->repo to avoid hardcoding another the_repository\n     * clarified test\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1583%2Fjohn-cai%2Fjc%2Fpopulate-repo-when-init-attr-index-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1583/john-cai/jc/populate-repo-when-init-attr-index-v3\nPull-Request: https://github.com/git/git/pull/1583\n\nRange-diff vs v2:\n\n 1:  e178236064a ! 1:  792b01fa616 merge-ort: initialize repo in index state\n     @@ t/t4300-merge-tree.sh: EXPECTED\n      +\t\tgit checkout @{-1} &&\n      +\t\ttree=$(git --attr-source=gitattributes merge-tree --write-tree \\\n      +\t\t--merge-base \"$base\" --end-of-options \"$source\" \"$merge\") &&\n     -+\t\techo \"foo\\nbar\\nbaz\" >expect &&\n     ++\t\tprintf \"foo\\nbar\\nbaz\\n\" >expect &&\n      +\t\tgit cat-file -p \"$tree:file1\" >actual &&\n      +\t\ttest_cmp expect actual\n      +\t)\n\n\n merge-ort.c           |  1 +\n t/t4300-merge-tree.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 7857ce9fbd1..36537256613 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)\n \tstruct index_state *attr_index = &opt->priv->attr_index;\n \tstruct cache_entry *ce;\n \n+\tattr_index->repo = opt->repo;\n \tattr_index->initialized = 1;\n \n \tif (!opt->renormalize)\ndiff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\nindex 57c4f26e461..c3a03e54187 100755\n--- a/t/t4300-merge-tree.sh\n+++ b/t/t4300-merge-tree.sh\n@@ -86,6 +86,33 @@ EXPECTED\n \ttest_cmp expected actual\n '\n \n+test_expect_success '3-way merge with --attr-source' '\n+\ttest_when_finished rm -rf 3-way &&\n+\tgit init 3-way &&\n+\t(\n+\t\tcd 3-way &&\n+\t\ttest_commit initial file1 foo &&\n+\t\tbase=$(git rev-parse HEAD) &&\n+\t\tgit checkout -b brancha &&\n+\t\techo bar >>file1 &&\n+\t\tgit commit -am \"adding bar\" &&\n+\t\tsource=$(git rev-parse HEAD) &&\n+\t\tgit checkout @{-1} &&\n+\t\tgit checkout -b branchb &&\n+\t\techo baz >>file1 &&\n+\t\tgit commit -am \"adding baz\" &&\n+\t\tmerge=$(git rev-parse HEAD) &&\n+\t\tgit checkout -b gitattributes &&\n+\t\ttest_commit \"gitattributes\" .gitattributes \"file1 merge=union\" &&\n+\t\tgit checkout @{-1} &&\n+\t\ttree=$(git --attr-source=gitattributes merge-tree --write-tree \\\n+\t\t--merge-base \"$base\" --end-of-options \"$source\" \"$merge\") &&\n+\t\tprintf \"foo\\nbar\\nbaz\\n\" >expect &&\n+\t\tgit cat-file -p \"$tree:file1\" >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'file change A, B (same)' '\n \tgit reset --hard initial &&\n \ttest_commit \"change-a-b-same-A\" \"initial-file\" \"AAA\" &&\n\nbase-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09\n-- \ngitgitgadget\n"},{"id":"482926","messageId":"xmqq4jizxyla.fsf@gitster.g","threadId":"60306","inReplyTo":"pull.1583.v3.git.git.1696857660374.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] merge-ort: initialize repo in index state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-09T21:41:53Z","receivedAt":"2023-10-09T21:42:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>     Fix this by initializing the repository in the index state.\n>     \n>     Changes since V2:\n>     \n>      * fixed test by using printf instead of echo\n\nMuch better than using unportable \\n with echo.\n\n>      -+\t\techo \"foo\\nbar\\nbaz\" >expect &&\n>      ++\t\tprintf \"foo\\nbar\\nbaz\\n\" >expect &&\n\nBut if we are using printf, it would be easier to read lines\nseparately, which would look more like\n\n\tprintf \"%s\\n\" foo bar baz >expect\n\nAnd we have\n\n\ttest_write_lines foo bar baz >expect\n\nto make it even more discoverable.\n\n>       +\t\tgit cat-file -p \"$tree:file1\" >actual &&\n>       +\t\ttest_cmp expect actual\n>       +\t)\n>\n>\n>  merge-ort.c           |  1 +\n>  t/t4300-merge-tree.sh | 27 +++++++++++++++++++++++++++\n>  2 files changed, 28 insertions(+)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 7857ce9fbd1..36537256613 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)\n>  \tstruct index_state *attr_index = &opt->priv->attr_index;\n>  \tstruct cache_entry *ce;\n>  \n> +\tattr_index->repo = opt->repo;\n>  \tattr_index->initialized = 1;\n>  \n>  \tif (!opt->renormalize)\n> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\n> index 57c4f26e461..c3a03e54187 100755\n> --- a/t/t4300-merge-tree.sh\n> +++ b/t/t4300-merge-tree.sh\n> @@ -86,6 +86,33 @@ EXPECTED\n>  \ttest_cmp expected actual\n>  '\n>  \n> +test_expect_success '3-way merge with --attr-source' '\n> +\ttest_when_finished rm -rf 3-way &&\n> +\tgit init 3-way &&\n> +\t(\n> +\t\tcd 3-way &&\n> +\t\ttest_commit initial file1 foo &&\n> +\t\tbase=$(git rev-parse HEAD) &&\n> +\t\tgit checkout -b brancha &&\n> +\t\techo bar >>file1 &&\n> +\t\tgit commit -am \"adding bar\" &&\n> +\t\tsource=$(git rev-parse HEAD) &&\n> +\t\tgit checkout @{-1} &&\n> +\t\tgit checkout -b branchb &&\n> +\t\techo baz >>file1 &&\n> +\t\tgit commit -am \"adding baz\" &&\n> +\t\tmerge=$(git rev-parse HEAD) &&\n> +\t\tgit checkout -b gitattributes &&\n> +\t\ttest_commit \"gitattributes\" .gitattributes \"file1 merge=union\" &&\n\nOK, the branch \"gitattributes\" will be used to drive merge of file1\nusing the union merge to avoid conflicting.\n\n> +\t\tgit checkout @{-1} &&\n\nBut such attribute will only be available in that branch, not in the\nchecked out working tree.  And then\n\n> +\t\ttree=$(git --attr-source=gitattributes merge-tree --write-tree \\\n> +\t\t--merge-base \"$base\" --end-of-options \"$source\" \"$merge\") &&\n\nwe use the gitattributes branch as the tree-ish to take the\nattribute information from.  Makes sense.\n\n> +\t\tprintf \"foo\\nbar\\nbaz\\n\" >expect &&\n\nI'll squash in the \"test_write_lines\" change while queuing.\n\n> +\t\tgit cat-file -p \"$tree:file1\" >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n>  test_expect_success 'file change A, B (same)' '\n>  \tgit reset --hard initial &&\n>  \ttest_commit \"change-a-b-same-A\" \"initial-file\" \"AAA\" &&\n>\n> base-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09\n\nThanks.  Looking good.\n"},{"id":"482986","messageId":"7E296F4F-82E1-4DA2-94C5-EFBBD2B05BB3@gmail.com","threadId":"60306","inReplyTo":"xmqq4jizxyla.fsf@gitster.g","subject":"Re: [PATCH v3] merge-ort: initialize repo in index state","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-10-10T15:06:44Z","receivedAt":"2023-10-10T15:06:51Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio\n\nOn 9 Oct 2023, at 17:41, Junio C Hamano wrote:\n\n> \"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n>>     Fix this by initializing the repository in the index state.\n>>\n>>     Changes since V2:\n>>\n>>      * fixed test by using printf instead of echo\n>\n> Much better than using unportable \\n with echo.\n>\n>>      -+\t\techo \"foo\\nbar\\nbaz\" >expect &&\n>>      ++\t\tprintf \"foo\\nbar\\nbaz\\n\" >expect &&\n>\n> But if we are using printf, it would be easier to read lines\n> separately, which would look more like\n>\n> \tprintf \"%s\\n\" foo bar baz >expect\n>\n> And we have\n>\n> \ttest_write_lines foo bar baz >expect\n>\n> to make it even more discoverable.\n\nwasn't aware of test_write_lines, thanks.\n\n>\n>>       +\t\tgit cat-file -p \"$tree:file1\" >actual &&\n>>       +\t\ttest_cmp expect actual\n>>       +\t)\n>>\n>>\n>>  merge-ort.c           |  1 +\n>>  t/t4300-merge-tree.sh | 27 +++++++++++++++++++++++++++\n>>  2 files changed, 28 insertions(+)\n>>\n>> diff --git a/merge-ort.c b/merge-ort.c\n>> index 7857ce9fbd1..36537256613 100644\n>> --- a/merge-ort.c\n>> +++ b/merge-ort.c\n>> @@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)\n>>  \tstruct index_state *attr_index = &opt->priv->attr_index;\n>>  \tstruct cache_entry *ce;\n>>\n>> +\tattr_index->repo = opt->repo;\n>>  \tattr_index->initialized = 1;\n>>\n>>  \tif (!opt->renormalize)\n>> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\n>> index 57c4f26e461..c3a03e54187 100755\n>> --- a/t/t4300-merge-tree.sh\n>> +++ b/t/t4300-merge-tree.sh\n>> @@ -86,6 +86,33 @@ EXPECTED\n>>  \ttest_cmp expected actual\n>>  '\n>>\n>> +test_expect_success '3-way merge with --attr-source' '\n>> +\ttest_when_finished rm -rf 3-way &&\n>> +\tgit init 3-way &&\n>> +\t(\n>> +\t\tcd 3-way &&\n>> +\t\ttest_commit initial file1 foo &&\n>> +\t\tbase=$(git rev-parse HEAD) &&\n>> +\t\tgit checkout -b brancha &&\n>> +\t\techo bar >>file1 &&\n>> +\t\tgit commit -am \"adding bar\" &&\n>> +\t\tsource=$(git rev-parse HEAD) &&\n>> +\t\tgit checkout @{-1} &&\n>> +\t\tgit checkout -b branchb &&\n>> +\t\techo baz >>file1 &&\n>> +\t\tgit commit -am \"adding baz\" &&\n>> +\t\tmerge=$(git rev-parse HEAD) &&\n>> +\t\tgit checkout -b gitattributes &&\n>> +\t\ttest_commit \"gitattributes\" .gitattributes \"file1 merge=union\" &&\n>\n> OK, the branch \"gitattributes\" will be used to drive merge of file1\n> using the union merge to avoid conflicting.\n>\n>> +\t\tgit checkout @{-1} &&\n>\n> But such attribute will only be available in that branch, not in the\n> checked out working tree.  And then\n>\n>> +\t\ttree=$(git --attr-source=gitattributes merge-tree --write-tree \\\n>> +\t\t--merge-base \"$base\" --end-of-options \"$source\" \"$merge\") &&\n>\n> we use the gitattributes branch as the tree-ish to take the\n> attribute information from.  Makes sense.\n>\n>> +\t\tprintf \"foo\\nbar\\nbaz\\n\" >expect &&\n>\n> I'll squash in the \"test_write_lines\" change while queuing.\n\nthank you!\nJohn\n\n>\n>> +\t\tgit cat-file -p \"$tree:file1\" >actual &&\n>> +\t\ttest_cmp expect actual\n>> +\t)\n>> +'\n>> +\n>>  test_expect_success 'file change A, B (same)' '\n>>  \tgit reset --hard initial &&\n>>  \ttest_commit \"change-a-b-same-A\" \"initial-file\" \"AAA\" &&\n>>\n>> base-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09\n>\n> Thanks.  Looking good.\n"}]}