| Message ID | pull.820.v5.git.1608609498.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | negative-refspec: fix segfault on : refspec | expand |
"Nipunn Koorapati via GitGitGadget" <gitgitgadget@gmail.com> writes: > Update since Patch series V4 > > * Squashed in Junio's patch to handle non-master named branches > * Explicitly use test_unconfig > > Appreciate the reviews from Junio and Eric! Happy Holidays! > > Nipunn Koorapati (2): > negative-refspec: fix segfault on : refspec > negative-refspec: improve comment on query_matches_negative_refspec Thanks, will replace. Happy holidays to you, too.
Is this something we want to merge into the 2.29 maint branch? It is a segfault for anyone pushing with matching refspecs on 2.29 At what point does git stop patching bugs in the maint branch? --Nipunn --Nipunn On Tue, Dec 22, 2020 at 6:49 AM Junio C Hamano <gitster@pobox.com> wrote: > > "Nipunn Koorapati via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > Update since Patch series V4 > > > > * Squashed in Junio's patch to handle non-master named branches > > * Explicitly use test_unconfig > > > > Appreciate the reviews from Junio and Eric! Happy Holidays! > > > > Nipunn Koorapati (2): > > negative-refspec: fix segfault on : refspec > > negative-refspec: improve comment on query_matches_negative_refspec > > Thanks, will replace. Happy holidays to you, too.
Nipunn Koorapati <nipunn1313@gmail.com> writes:
> Is this something we want to merge into the 2.29 maint branch?
Eventually by backporting, but a fix typically goes to the current
development track first so it would happen after 2.30 is finished, I
would think.
> Eventually by backporting, but a fix typically goes to the current > development track first so it would happen after 2.30 is finished, I > would think. I wanted to bump this idea - now that it appears that 2.30 is complete and the new maint branch. Given that this patch makes matching-refspec unusable in 2.29, would it make sense to backport a fix to the 2.29 release? If that seems risky/unwanted, is there some practice of documenting known (serious) bugs in past releases? Thanks --Nipunn
Nipunn Koorapati <nipunn1313@gmail.com> writes: >> Eventually by backporting, but a fix typically goes to the current >> development track first so it would happen after 2.30 is finished, I >> would think. > > I wanted to bump this idea - now that it appears that 2.30 is complete > and the new maint branch. Given that this patch makes matching-refspec > unusable in 2.29, would it make sense to backport a fix to the 2.29 > release? Yes, it does make sense. If we were to spend engineering effort to cut a 2.29.1, however, we'd better make sure not just this fix but all the other fixes relevant to the 2.29 track that are already well tested are included in it. We just issued 2.30 and not many people are using it to exercise a relatively new negative pathspec feature yet, so it probably is a good idea to spend a weeks or two to enumerate what other things we want to be in the 2.29 maintenance track. I personally do not have time for doing that myself right now, but luckily it is something contributors like you can step in to help ;-) Thanks.
If remote.origin.push was set to ":", git segfaults during a push operation, due to bad parsing logic in query_matches_negative_refspec. Per bisect, the bug was introduced in: c0192df630 (refspec: add support for negative refspecs, 2020-09-30) We found this issue when rolling out git 2.29 at Dropbox - as several folks had "push = :" in their configuration. I based my diff off the master branch, but also confirmed that it patches cleanly onto maint - if the maintainers would like to also fix the segfault on 2.29 Update since Patch series V1: * Handled matching refspec explicitly * Added testing for "+:" case * Added comment explaining how the two loops work together Update since Patch series V2 * style suggestion in remote.c * Use test_config * Add test for a case with a matching refspec + negative refspec * Fix test_config to work with --add * Updated commit message to describe what git is told to do instead of segfaulting Update since Patch series V3 * Removed commit modifying test_config * Remove segfault-related comments in test * Consolidate the three tests to two tests (1st and 3rd test overlapped in functionality) * Base the patch series on the maint branch - since the bug affects 2.29.2 Update since Patch series V4 * Squashed in Junio's patch to handle non-master named branches * Explicitly use test_unconfig Appreciate the reviews from Junio and Eric! Happy Holidays! Nipunn Koorapati (2): negative-refspec: fix segfault on : refspec negative-refspec: improve comment on query_matches_negative_refspec remote.c | 16 ++++++++-- t/t5582-fetch-negative-refspec.sh | 51 +++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 3 deletions(-) base-commit: 898f80736c75878acc02dc55672317fcc0e0a5a6 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-820%2Fnipunn1313%2Fnk%2Fpush-refspec-segfault-v5 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-820/nipunn1313/nk/push-refspec-segfault-v5 Pull-Request: https://github.com/gitgitgadget/git/pull/820 Range-diff vs v4: 1: e59ff29bdef ! 1: 48c79dc3d84 negative-refspec: fix segfault on : refspec @@ t/t5582-fetch-negative-refspec.sh: test_expect_success "fetch --prune with negat ' +test_expect_success "push with matching : and negative refspec" ' -+ test_config -C two remote.one.push : && -+ # Fails to push master w/ tip behind counterpart ++ # Manually handle cleanup, since test_config is not ++ # prepared to take arbitrary options like --add ++ test_when_finished "test_unconfig -C two remote.one.push" && ++ ++ # For convenience, we use "master" to refer to the name of ++ # the branch created by default in the following. ++ # ++ # Repositories two and one have branches other than "master" ++ # but they have no overlap---"master" is the only one that ++ # is shared between them. And the master branch at two is ++ # behind the master branch at one by one commit. ++ git -C two config --add remote.one.push : && ++ ++ # A matching push tries to update master, fails due to non-ff + test_must_fail git -C two push one && + ++ # "master" may actually not be "master"---find it out. ++ current=$(git symbolic-ref HEAD) && ++ + # If master is in negative refspec, then the command will not attempt + # to push and succeed. -+ # We do not need test_config here as we are updating remote.one.push -+ # again. The teardown of the first test_config will do --unset-all -+ git -C two config --add remote.one.push ^refs/heads/master && -+ git -C two push one ++ git -C two config --add remote.one.push "^$current" && ++ ++ # With "master" excluded, this push is a no-op. Nothing gets ++ # pushed and it succeeds. ++ git -C two push -v one +' + +test_expect_success "push with matching +: and negative refspec" ' -+ test_config -C two remote.one.push +: && -+ # Fails to push master w/ tip behind counterpart ++ test_when_finished "test_unconfig -C two remote.one.push" && ++ ++ # The same set-up as above, whose side-effect was a no-op. ++ git -C two config --add remote.one.push +: && ++ ++ # The push refuses to update the "master" branch that is checked ++ # out in the "one" repository, even when it is forced with +: + test_must_fail git -C two push one && + ++ # "master" may actually not be "master"---find it out. ++ current=$(git symbolic-ref HEAD) && ++ + # If master is in negative refspec, then the command will not attempt + # to push and succeed -+ git -C two config --add remote.one.push ^refs/heads/master && -+ git -C two push one ++ git -C two config --add remote.one.push "^$current" && ++ ++ # With "master" excluded, this push is a no-op. Nothing gets ++ # pushed and it succeeds. ++ git -C two push -v one +' + test_done 2: 20575407cc0 = 2: 1f9af0e991c negative-refspec: improve comment on query_matches_negative_refspec