| Message ID | pull.1583.git.git.1696519349407.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | merge-ort: initialize repo in index state | expand |
"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.
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/. 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... > 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? > 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
Hi Elijah, On 6 Oct 2023, at 1:14, Elijah Newren wrote: > 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. > > 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. > > 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! > > >> 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
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" &&