negative-refspec: fix segfault on : refspec #820
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/preview |
|
Preview email sent as pull.820.git.1608397544481.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.820.git.1608398598893.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this):
Our local convention in this project is to write about the
status-quo without the patch under discussion in the present tense,
and describe the fix as if we are giving orders to the codebase to
become like so (or giving orders to the monkeys sitting in front of
the keyboard to update the code). I'd explain the "problem
description" part of the above perhaps like so:
The logic added to check for negative pathspec match by
c0192df (refspec: add support for negative refspecs,
2020-09-30) looks at refspec->src assuming it never is NULL,
but when remote.origin.push is set to ":" (i.e. "matching"),
refspec->src is NULL, causing a segfauilt.
Thank you! Will update.
But stepping back a bit, a "matching" push is saying "if we have
branch 'hello', and they also have branch 'hello', push ours to
theirs". So if the query is asking about 'hello' (e.g. needle is
'hello'), shouldn't a refspec ":" have the same effect as a refspec
"hello:hello", instead of getting ignored like this patch does?
Original author of the feature (Jacob) cc'ed for insight.
- Can we have refspec->src==NULL in cases other than where
refspec->matching is true? If not, then perhaps the patch should
insert, before the problematic "else if" clause, something like
if (match_name_with_pattern(...))
string_list_append_nodup(...);
+ } else if (refspec->matching) {
+ ... behaviour for the matching case ...
+ } else if (refspec->src == NULL) {
+ BUG("refspec->src cannot be null here");
} else {
if (!strcmp(needle, refspec->src))
- We'd need to decide if ignoring is the right behaviour for the
matching refspec. I do not recall what we decided the logic of
the function should be offhand.
According to documentation in refspec.h:
src and dest are the two sides (including '*'
characters if present); if there is only one side, it is src, and dst is
NULL; if sides exist but are empty (i.e., the refspec either starts or ends
with ':'), the corresponding side is "".
Good thought - would appreciate insight from Jacob. From my readthrough, this
first loop intends to expand patterns, and the second loop checks for omissions.
In the special "matching" case, it's somewhat similar to the *:* pattern - and
the query string should directly be added to "&reversed" - per Junio's
suggestion.
I see some comments describing that we should apply pattern refspecs in reverse,
but it's not obvious to me if the `reversed` variable is being
reversed at any point.
I may be misunderstanding the intention here.
I'll wait for input from Jacob, and can follow up the diff afterward.
I'm also happy
to surrender the diff over to Jacob if there's something more complex needed.
|
nipunn1313
force-pushed
the
nk/push-refspec-segfault
branch
from
743e848
to
8da8d9c
Compare
|
/preview |
|
Preview email sent as pull.820.v2.git.1608412091.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.820.v2.git.1608415117.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -751,9 +751,13 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Sat, Dec 19, 2020 at 5:00 PM Nipunn Koorapati via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The logic added to check for negative pathspec match by c0192df630
> (refspec: add support for negative refspecs, 2020-09-30) looks at
> refspec->src assuming it is never NULL, however when
> remote.origin.push is set to ":", then refspec->src is NULL,
> causing a segfault within strcmp
>
> Added testing for this case in fetch-negative-refspec
A couple minor comments below...
> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
> ---
> diff --git a/remote.c b/remote.c
> @@ -751,9 +751,13 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite
> + } else if (refspec->matching) {
> + /* For the special matching refspec, any query should match */
> + string_list_append(&reversed, needle);
> + } else if (refspec->src == NULL) {
> + BUG("refspec->src should not be null here");
I realize that you copied Junio's example, but style on this project
is to write this as:
} else if (!refspec->src) {
...
> diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
> @@ -186,4 +186,19 @@ test_expect_success "fetch --prune with negative refspec" '
> +test_expect_success "push with matching ':' refspec" '
> + (
> + cd two &&
> + git config remote.one.push : &&
> + # Fails w/ tip behind counterpart - but should not segfault
> + test_must_fail git push one master &&
> +
> + git config remote.one.push +: &&
> + # Fails w/ tip behind counterpart - but should not segfault
> + test_must_fail git push one master &&
> +
> + git config --unset remote.one.push
> + )
> +'
If anything in this test fails prior to the final `git config
--unset`, then that cleanup command won't be executed, which might
negatively impact tests which follow. To ensure cleanup whether the
test succeeds or fails, use test_config(). Unfortunately,
test_config() has the limitation that it can't be used in subshells,
so you may have to restructure the test a bit, perhaps like this:
test_config remote.one.push : &&
(
cd two &&
test_must_fail git push one master &&
git config remote.one.push +: &&
test_must_fail git push one master
)
Driving the test with a for-loop and taking advantage of -C to avoid
the subshell is also an option:
for v in : +:
do
test_config -C two remote.one.push $v &&
test_must_fail git -C two push one master || return 1
done
|
User |
nipunn1313
force-pushed
the
nk/push-refspec-segfault
branch
2 times, most recently
from
9161e93
to
0fd4e9f
Compare
|
/submit |
|
Submitted as pull.820.v3.git.1608516320.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
t/test-lib-functions.sh
Outdated
| @@ -381,6 +381,7 @@ test_unconfig () { | |||
| config_dir=$1 | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Sun, Dec 20, 2020 at 9:05 PM Nipunn Koorapati via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> test_config fails to unset the configuration variable when
> using --add, as it tries to run git config --unset-all --add
>
> Tell test_config to invoke test_unconfig with the arg $2 when
> the arg $1 is --add
>
> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -381,6 +381,7 @@ test_unconfig () {
> config_dir=$1
> shift
> fi
> + echo git ${config_dir:+-C "$config_dir"} config --unset-all "$@"
Stray debugging gunk?
> @@ -400,7 +401,13 @@ test_config () {
> - test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} '$1'" &&
> +
> + first_arg=$1
> + if test "$1" = --add; then
> + first_arg=$2
> + fi
> +
> + test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} '$first_arg'" &&
Several comments...
Style on this project is to place `then` on its own line (as seen a
few lines above this change):
if test "$1" = --add
then
...
This logic would be easier to understand if the variable was named
`varname` or `cfgvar` (or something), which better conveys intention,
rather than `first_arg`.
It feels odd to single out `--add` when there are other similar
options, such as `--replace-all`, `--fixed-value`, or even `--type`
which people might try using in the future.
This new option parsing is somewhat brittle. If a caller uses
`test_config --add -C <dir> ...`, it won't work as expected. Perhaps
that's not likely to happen, but it would be easy enough to fix by
unifying and generalizing option parsing a bit. Doing so would also
make it easy for the other options mentioned above to be added later
if ever needed. For instance:
options=
while test $# != 0
do
case "$1" in
-C)
config_dir=$2
shift
;;
--add)
options="$options $1"
;;
*)
break
;;
esac
shift
done
Finally, as this is a one-off case, it might be simpler just to drop
this patch altogether and open-code the cleanup in the test itself in
patch [2/3] rather than bothering with test_config() in that
particular case. For example:
test_when_finished "test_unconfig -C two remote.one.push" &&
git config -C two --add remote.one.push : &&
test_must_fail git -C two push one &&
git config -C two --add remote.one.push ^refs/heads/master &&
git -C two push one
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Eric Sunshine <sunshine@sunshineco.com> writes:
> Finally, as this is a one-off case, it might be simpler just to drop
> this patch altogether and open-code the cleanup in the test itself in
> patch [2/3] rather than bothering with test_config() in that
> particular case. For example:
>
> test_when_finished "test_unconfig -C two remote.one.push" &&
> git config -C two --add remote.one.push : &&
> test_must_fail git -C two push one &&
> git config -C two --add remote.one.push ^refs/heads/master &&
> git -C two push one
That would be my preference, too. Thanks for carefully and
patiently reviewing.
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Mon, Dec 21, 2020 at 2:00 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > Finally, as this is a one-off case, it might be simpler just to drop
> > this patch altogether and open-code the cleanup in the test itself in
> > patch [2/3] rather than bothering with test_config() in that
> > particular case. For example:
> >
> > test_when_finished "test_unconfig -C two remote.one.push" &&
> > git config -C two --add remote.one.push : &&
> > test_must_fail git -C two push one &&
> > git config -C two --add remote.one.push ^refs/heads/master &&
> > git -C two push one
>
> That would be my preference, too. Thanks for carefully and
> patiently reviewing.
I forgot to mention that it likely would be a good idea to at least
mention in the commit message why test_config() is not being used for
that particular case. Perhaps saying something along the lines of "one
test handles config cleanup manually since test_config() is not
prepared to take arbitrary options such as --add" -- or something
along those lines -- would be sufficient. Alternatively, an in-code
comment within the test explaining the open-coding might be more
helpful to people reading the code in the future.
There was a problem hiding this comment.
On the Git mailing list, Nipunn Koorapati wrote (reply to this):
> I forgot to mention that it likely would be a good idea to at least
> mention in the commit message why test_config() is not being used for
> that particular case. Perhaps saying something along the lines of "one
> test handles config cleanup manually since test_config() is not
> prepared to take arbitrary options such as --add" -- or something
> along those lines -- would be sufficient. Alternatively, an in-code
> comment within the test explaining the open-coding might be more
> helpful to people reading the code in the future.
I found that since test_unconfig uses --unset-all, I can write a test as such
test_config -C two remote.one.push +: &&
test_must_fail git -C two push one &&
git -C two config --add remote.one.push ^refs/heads/master &&
git -C two push one
The unconfig of the test_config will --unset-all remote.one.push. I can
use this technique and add a comment to that extent.
--Nipunn
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Mon, Dec 21, 2020 at 7:00 PM Nipunn Koorapati <nipunn1313@gmail.com> wrote:
> I found that since test_unconfig uses --unset-all, I can write a test as such
>
> test_config -C two remote.one.push +: &&
> test_must_fail git -C two push one &&
> git -C two config --add remote.one.push ^refs/heads/master &&
> git -C two push one
>
> The unconfig of the test_config will --unset-all remote.one.push. I can
> use this technique and add a comment to that extent.
Yes, you could do that, though it is somewhat subtle and increases
cognitive load since the reader has to reason about it a bit more --
and perhaps study the internal implementation of test_config() -- to
convince himself or herself that the different methods of setting
configuration (test_config() vs. `git config`) used in the same test
is intentional and works as intended.
The example presented earlier, on the other hand, in which cleanup is
explicit via `test_when_finished "test_unconfig ..."` does not suffer
from such increased cognitive load since it uses `git config`
consistently to set configuration rather than a mix of `git config`
and test_config(). This sort of consideration is important not just
for reviewers, but for people who need to understand the code down the
road. For this reason, I think I favor the version in which the
cleanup is explicit. (But that's just my opinion...)
There was a problem hiding this comment.
On the Git mailing list, Nipunn Koorapati wrote (reply to this):
> Yes, you could do that, though it is somewhat subtle and increases
> cognitive load since the reader has to reason about it a bit more --
> and perhaps study the internal implementation of test_config() -- to
> convince himself or herself that the different methods of setting
> configuration (test_config() vs. `git config`) used in the same test
> is intentional and works as intended.
>
> The example presented earlier, on the other hand, in which cleanup is
> explicit via `test_when_finished "test_unconfig ..."` does not suffer
> from such increased cognitive load since it uses `git config`
> consistently to set configuration rather than a mix of `git config`
> and test_config(). This sort of consideration is important not just
> for reviewers, but for people who need to understand the code down the
> road. For this reason, I think I favor the version in which the
> cleanup is explicit. (But that's just my opinion...)
Totally sympathize with wanting to reduce the cognitive load of
reading the test suite.
As a reader of the test, I found both implementation choices
nonobvious - and requiring
some diving into test_config and test_unconfig (namely the --unset-all
behavior).
A comment on the `git config` stating that it's there because
`test_config` doesn't yet support
`--add` seems equally clarifying as inserting a comment next to the
test_unconfig usage.
That being said, I suspect anyone in the future poking around with
this will have to sourcedive
through test_config and test_unconfig to make sense of it.
I'll switch it over to the test_unconfig option on the next reroll if requested.
--Nipunn
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Mon, Dec 21, 2020 at 9:25 PM Nipunn Koorapati <nipunn1313@gmail.com> wrote:
> That being said, I suspect anyone in the future poking around with
> this will have to sourcedive
> through test_config and test_unconfig to make sense of it.
>
> I'll switch it over to the test_unconfig option on the next reroll if requested.
Indeed, it's not entirely uncommon to have to dig into the test
functions when writing tests. My bigger concern was someone coming
along and thinking that the mixed use of test_config() and manual `git
config` in the same test was a mistake, and want to fix that mistake,
which would lead the person down the same rabbit hole. Explicit
cleanup via test_unconfig() and consistent use of `git config` within
the test, on the other hand, does not look accidental, so the reader
would be less likely to want to "fix the mistake". The comment you
added in the re-roll above the manual cleanup saves the reader the
trouble of having to dive into the implementation of test_config(),
which is a nice bonus.
| @@ -751,9 +751,13 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Sun, Dec 20, 2020 at 9:05 PM Nipunn Koorapati via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The logic added to check for negative pathspec match by c0192df630
> (refspec: add support for negative refspecs, 2020-09-30) looks at
> refspec->src assuming it is never NULL, however when
> remote.origin.push is set to ":", then refspec->src is NULL,
> causing a segfault within strcmp
>
> Tell git to handle matching refspec by adding the needle to the
> set of positively matched refspecs, since matching ":" refspecs
> match anything as src.
>
> Added testing for matching refspec pushes fetch-negative-refspec
s/Added testing/Add test/
> both individually and in combination with a negative refspec
>
> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
> ---
> diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
> @@ -186,4 +186,26 @@ test_expect_success "fetch --prune with negative refspec" '
> +test_expect_success "push with matching ':' refspec" '
> + test_config -C two remote.one.push : &&
> + # Fails w/ tip behind counterpart - but should not segfault
> + test_must_fail git -C two push one
> +'
Nit: It is understood implicitly that Git should not segfault (or
indeed any software). That's also implied by use of test_must_fail()
which explicitly distinguishes expected failures from unexpected
failures (where segfault falls in the category of unexpected failure).
Therefore, it doesn't really add value to say "but should not
segfault" in the comment.
Same observation applies to the other similarly-worded comments in
this patch. Not alone worth a re-roll, but perhaps worth changing if
you do re-roll.
25199ff
to
2057540
Compare
|
/submit |
|
Submitted as pull.820.v4.git.1608599513.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
| @@ -751,9 +751,13 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Nipunn Koorapati via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +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_must_fail git -C two push one &&
I offhand do not know where the master branch of two and one
repositories are, but I presume that one's master is not an ancestor
of two's master here, and the reason why this fails is because we
would prevent such a non-ff push unless forced? Are there other
matching refs between one and two that are subject to the push
operation here, or is the 'master' the only thing that exists?
> + # 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
... and the idea of the test is that by adding a "we do not want to
push out our master" configuration, we no longer attempt to push out
the 'master' branch from two that is not a descendant of the master
branch of one, so "push" would "succeed". Is there other branches
involved, or this is essentially a no-op as there is only 'master'
branch involved in the operation?
> +'
> +
> +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_must_fail git -C two push one &&
Assuming that the successful case from the previous test was a
no-op, we start from the same condition from the previous one. THe
only difference is that the matching push is now configured to force.
So, how would this one fail, exactly? Aren't we forcing? Shouldn't
we succeed in such a case?
I think the test still fails to push but for a different reason. It
is not because the tip being pushed is not ahead of the counterpart
at the receiving repository. +: (i.e. force-push matching refs)
takes care of the "must fast-forward" requirement that causes the
previous one to fail.
It is because the receiving repository is not a bare repository, and
the push attempts to update its current branch. It cannot be forced
with + prefix, and that is why it fails.
So, the comment above is wrong. Perhaps
# Fail to update the branch currently checked out.
or something.
> + # 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
And this succeeds for the same reason, i.e. it becomes no-op because
there is no other branches involved?
> +'
> +
> test_done
Ideally, we should make sure that the next person who reads "git
show" output of the commit that would result from the patch would
not have to ask any of the "?" asked in the review above. Let me
see if I can come up with a suggestion to get us closer to that
goal.
... goes and hacks ...
Perhaps squash the following into this step?
Thanks.
t/t5582-fetch-negative-refspec.sh | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git c/t/t5582-fetch-negative-refspec.sh w/t/t5582-fetch-negative-refspec.sh
index a4960c586b..bed67cf92d 100755
--- c/t/t5582-fetch-negative-refspec.sh
+++ w/t/t5582-fetch-negative-refspec.sh
@@ -187,8 +187,13 @@ test_expect_success "fetch --prune with negative refspec" '
'
test_expect_success "push with matching : and negative refspec" '
+ # 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.
test_config -C two remote.one.push : &&
- # Fails to push master w/ tip behind counterpart
+
+ # A matching push tries to update master, fails due to non-ff
test_must_fail git -C two push one &&
# If master is in negative refspec, then the command will not attempt
@@ -196,18 +201,27 @@ test_expect_success "push with matching : and negative refspec" '
# 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
+
+ # 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" '
+ # The same set-up as above, whose side-effect was a no-op.
test_config -C two remote.one.push +: &&
- # Fails to push master w/ tip behind counterpart
+
+ # 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 &&
# 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
+
+ # With "master" excluded, this push is a no-op. Nothing gets
+ # pushed and it succeeds.
+ git -C two push -v one
'
test_done
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Junio C Hamano <gitster@pobox.com> writes:
> @@ -196,18 +201,27 @@ test_expect_success "push with matching : and negative refspec" '
> # 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
> +
> + # With "master" excluded, this push is a no-op. Nothing gets
> + # pushed and it succeeds.
> + git -C two push -v one
> '
Another obvious thing is that these tests will not work without
tweaking when merged to 'seen', as over there the name given by
default to the initial branch might not be 'master'. The negative
refspec specification must be written in a way not to depend on
a particular name, I think.
Here is another try (disregard the previous one and squash this one
on top of your 1/2).
Thanks.
t/t5582-fetch-negative-refspec.sh | 35 +++++++++++++++++++++++++++++------
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git c/t/t5582-fetch-negative-refspec.sh w/t/t5582-fetch-negative-refspec.sh
index a4960c586b..83a3c58c0c 100755
--- c/t/t5582-fetch-negative-refspec.sh
+++ w/t/t5582-fetch-negative-refspec.sh
@@ -187,27 +187,50 @@ test_expect_success "fetch --prune with negative refspec" '
'
test_expect_success "push with matching : and negative refspec" '
+ # 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.
test_config -C two remote.one.push : &&
- # Fails to push master w/ tip behind counterpart
+
+ # 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" '
+ # The same set-up as above, whose side-effect was a no-op.
test_config -C two remote.one.push +: &&
- # Fails to push master w/ tip behind counterpart
+
+ # 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
|
This patch series was integrated into seen via git@f2101f5. |
nipunn1313
force-pushed
the
nk/push-refspec-segfault
branch
from
2057540
to
1f9af0e
Compare
The logic added to check for negative pathspec match by c0192df (refspec: add support for negative refspecs, 2020-09-30) looks at refspec->src assuming it is never NULL, however when remote.origin.push is set to ":", then refspec->src is NULL, causing a segfault within strcmp. Tell git to handle matching refspec by adding the needle to the set of positively matched refspecs, since matching ":" refspecs match anything as src. Add test for matching refspec pushes fetch-negative-refspec both individually and in combination with a negative refspec. Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
Comment did not adequately explain how the two loops work together to achieve the goal of querying for matching of any negative refspec. Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
|
/submit |
|
Submitted as pull.820.v5.git.1608609498.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
This branch is now known as |
|
This patch series was integrated into seen via git@85d5f46. |
|
This patch series was integrated into next via git@7d88169. |
|
This patch series was integrated into seen via git@16467e7. |
|
This patch series was integrated into seen via git@edf9e4f. |
|
This patch series was integrated into seen via git@7358320. |
|
This patch series was integrated into next via git@7358320. |
|
This patch series was integrated into master via git@7358320. |
|
Closed via 7358320. |
|
On the Git mailing list, Nipunn Koorapati wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
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.
Hi,
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
|
|
On the Git mailing list, Nipunn Koorapati wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Jacob Keller wrote (reply to this): |
|
User |
| @@ -751,9 +751,13 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Jacob Keller wrote (reply to this):
On Mon, Dec 21, 2020 at 8:01 PM Nipunn Koorapati via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Nipunn Koorapati <nipunn@dropbox.com>
>
> The logic added to check for negative pathspec match by c0192df630
> (refspec: add support for negative refspecs, 2020-09-30) looks at
> refspec->src assuming it is never NULL, however when
> remote.origin.push is set to ":", then refspec->src is NULL,
> causing a segfault within strcmp.
>
> Tell git to handle matching refspec by adding the needle to the
> set of positively matched refspecs, since matching ":" refspecs
> match anything as src.
>
This seems like the right approach to me. Thanks for the fix, and the
tests so we don't break it on accident again in the future.
belated, but....
Reviewed-by: Jacob Keller <jacob.keller@gmail.com>
> Add test for matching refspec pushes fetch-negative-refspec
> both individually and in combination with a negative refspec.
>
> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
> ---
> remote.c | 10 ++++--
> t/t5582-fetch-negative-refspec.sh | 51 +++++++++++++++++++++++++++++++
> 2 files changed, 58 insertions(+), 3 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 8be67f0892b..4f1a4099f1a 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -751,9 +751,13 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite
>
> if (match_name_with_pattern(key, needle, value, &expn_name))
> string_list_append_nodup(&reversed, expn_name);
> - } else {
> - if (!strcmp(needle, refspec->src))
> - string_list_append(&reversed, refspec->src);
> + } else if (refspec->matching) {
> + /* For the special matching refspec, any query should match */
> + string_list_append(&reversed, needle);
Right, so we explicitly handle matching first...
> + } else if (!refspec->src) {
> + BUG("refspec->src should not be null here");
and then carefully check to make sure we don't end up with a NULL src
for some other reason, and at least BUG() instead of just crashing.
This shouldn't be possible because when we build the refspec, src is
always not NULL unless in the case of matching. Ok.
> + } else if (!strcmp(needle, refspec->src)) {
> + string_list_append(&reversed, refspec->src);
> }
> }
Yep, this looks like the best approach to solving this.
>
> diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
> index 8c61e28fec8..2f3b064d0e7 100755
> --- a/t/t5582-fetch-negative-refspec.sh
> +++ b/t/t5582-fetch-negative-refspec.sh
> @@ -186,4 +186,55 @@ test_expect_success "fetch --prune with negative refspec" '
> )
> '
>
> +test_expect_success "push with matching : and negative refspec" '
> + # 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.
> + 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_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 "^$current" &&
> +
> + # With "master" excluded, this push is a no-op. Nothing gets
> + # pushed and it succeeds.
> + git -C two push -v one
> +'
> +
> test_done
> --
> gitgitgadget
>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
c0192df (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:
Update since Patch series V2
Update since Patch series V3
Update since Patch series V4
Appreciate the reviews from Junio and Eric! Happy Holidays!
cc: Eric Sunshine sunshine@sunshineco.com
cc: Jacob Keller jacob.keller@gmail.com