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

From: Elijah Newren <newren@gmail.com>
Date: Oct 6, 2023, 05:14 UTC
Message-ID: <CABPp-BG_cQ011a15HSVRtJDq7MxgqY2_tD8aZm0Qc4F6ZU0NPA@mail.gmail.com>
In-Reply-To: <pull.1583.git.git.1696519349407.gitgitgadget@gmail.com>

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