{"thread":{"id":"13967","subject":"[Crash] git-push $remote $non_ref:$anything","startedAt":"2008-06-15T19:38:23Z","lastAt":"2008-06-16T16:17:55Z","messageCount":5,"participants":["Mike Hommey","Jeff King","Daniel Barkalow"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"79950","messageId":"20080615193823.GA11218@glandium.org","threadId":"13967","inReplyTo":null,"subject":"[Crash] git-push $remote $non_ref:$anything","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2008-06-15T19:38:23Z","receivedAt":"2008-06-15T19:38:23Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Hi,\n\nI somehow managed to get a segfault by running this:\n\ngit push origin non-existant-branch-name:non-existant-branch-name\n\nFWIW, the output looks like this:\n\nerror: src refspec patches/uri-handlers does not match any.\nSegmentation fault\nfatal: The remote end hung up unexpectedly\n\nI don't have time to dig into this, unfortunately.\n\nMike\n"},{"id":"79951","messageId":"20080615195541.GA7683@sigill.intra.peff.net","threadId":"13967","inReplyTo":"20080615193823.GA11218@glandium.org","subject":"Re: [Crash] git-push $remote $non_ref:$anything","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-15T19:55:41Z","receivedAt":"2008-06-15T19:55:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[cc'ing Daniel for remote.c advice]\n\nOn Sun, Jun 15, 2008 at 09:38:23PM +0200, Mike Hommey wrote:\n\n> I somehow managed to get a segfault by running this:\n> \n> git push origin non-existant-branch-name:non-existant-branch-name\n\nHmm. The problem is the ref-guessing code. Given \"git push foo:bar\",\nwhen we try to figure out what \"bar\" means we first try to find\nrefs/heads/bar or refs/heads/bar on the remote. But if that fails, we\nare pushing a new item, so we try to use the same prefix as what \"foo\"\nresolved to (e.g., if \"foo\" is a branch, we make it \"refs/heads/bar\").\n\nSo if \"foo\" doesn't resolve, we end up dereferencing NULL as part of our\nguess. And the fix is obvious and the patch is below.\n\nBut it kind of makes me wonder why we bother looking at the dst side of\nthe refspec at all, since the src has already failed. Is there a good\nreason not to just bail from match_explicit when we can't resolve the\nsrc?\n\n---\ndiff --git a/remote.c b/remote.c\nindex 91e3b11..fd8c71a 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -920,7 +920,8 @@ static int match_explicit(struct ref *src, struct ref *dst,\n \tcase 0:\n \t\tif (!memcmp(dst_value, \"refs/\", 5))\n \t\t\tmatched_dst = make_linked_ref(dst_value, dst_tail);\n-\t\telse if((dst_guess = guess_ref(dst_value, matched_src)))\n+\t\telse if(matched_src &&\n+\t\t\t(dst_guess = guess_ref(dst_value, matched_src)))\n \t\t\tmatched_dst = make_linked_ref(dst_guess, dst_tail);\n \t\telse\n \t\t\terror(\"unable to push to unqualified destination: %s\\n\"\n"},{"id":"79958","messageId":"alpine.LNX.1.00.0806151626260.19665@iabervon.org","threadId":"13967","inReplyTo":"20080615195541.GA7683@sigill.intra.peff.net","subject":"Re: [Crash] git-push $remote $non_ref:$anything","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-06-15T21:06:52Z","receivedAt":"2008-06-15T21:06:52Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 15 Jun 2008, Jeff King wrote:\n\n> [cc'ing Daniel for remote.c advice]\n> \n> On Sun, Jun 15, 2008 at 09:38:23PM +0200, Mike Hommey wrote:\n> \n> > I somehow managed to get a segfault by running this:\n> > \n> > git push origin non-existant-branch-name:non-existant-branch-name\n> \n> Hmm. The problem is the ref-guessing code. Given \"git push foo:bar\",\n> when we try to figure out what \"bar\" means we first try to find\n> refs/heads/bar or refs/heads/bar on the remote. But if that fails, we\n> are pushing a new item, so we try to use the same prefix as what \"foo\"\n> resolved to (e.g., if \"foo\" is a branch, we make it \"refs/heads/bar\").\n> \n> So if \"foo\" doesn't resolve, we end up dereferencing NULL as part of our\n> guess. And the fix is obvious and the patch is below.\n> \n> But it kind of makes me wonder why we bother looking at the dst side of\n> the refspec at all, since the src has already failed. Is there a good\n> reason not to just bail from match_explicit when we can't resolve the\n> src?\n\nThe only thing I can think of is that a user might have made some mistake \nthat would be more obvious with the error messages about both sides. Aside \nfrom that, it doesn't seem to have any possible effects anyway.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"80042","messageId":"20080616161502.GA7219@sigill.intra.peff.net","threadId":"13967","inReplyTo":"alpine.LNX.1.00.0806151626260.19665@iabervon.org","subject":"Re: [Crash] git-push $remote $non_ref:$anything","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-16T16:15:02Z","receivedAt":"2008-06-16T16:15:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 15, 2008 at 05:06:52PM -0400, Daniel Barkalow wrote:\n\n> The only thing I can think of is that a user might have made some mistake \n> that would be more obvious with the error messages about both sides. Aside \n> from that, it doesn't seem to have any possible effects anyway.\n\nHmm. Yes, looking at it more closely, I think the intent was to show\nas many errors as possible. We report all errors for the source and\ndest sides of every refspec before aborting.\n\nI think the patch below is an improvement. Apologies for the long-winded\ncommit message, but it took me a while to figure out what was going on,\nso I thought it worth explaining.\n\n-- >8 --\nclean up error conventions of remote.c:match_explicit\n\nmatch_explicit is called for each push refspec to try to\nfully resolve the source and destination sides of the\nrefspec.  Currently, we look at each refspec and report\nerrors on both the source and the dest side before aborting.\n\nIt makes sense to report errors for each refspec, since an\nerror in one is independent of an error in the other.\nHowever, reporting errors on the 'dst' side of a refspec if\nthere has been an error on the 'src' side does not\nnecessarily make sense, since the interpretation of the\n'dst' side depends on the 'src' side (for example, when\ncreating a new unqualified remote ref, we use the same type\nas the src ref).\n\nThis patch lets match_explicit return early when the src\nside of the refspec is bogus. We still look at all of the\nrefspecs before aborting the push, though.\n\nAt the same time, we clean up the call signature, which\npreviously took an extra \"errs\" flag. This was pointless, as\nwe didn't act on that flag, but rather just passed it back\nto the caller. Instead, we now use the more traditional\n\"return -1\" to signal an error, and the caller aggregates\nthe error count.\n\nThis change fixes two bugs, as well:\n\n  - the early return avoids a segfault when passing a NULL\n    matched_src to guess_ref()\n\n  - the check for multiple sources pointing to a single dest\n    aborted if the \"err\" flag was set. Presumably the intent\n    was not to bother with the check if we had no\n    matched_src. However, since the err flag was passed in\n    from the caller, we might abort the check just because a\n    previous refspec had a problem, which doesn't make\n    sense.\n\n    In practice, this didn't matter, since due to the error\n    flag we end up aborting the push anyway.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote.c |   25 ++++++++++---------------\n 1 files changed, 10 insertions(+), 15 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 91e3b11..55a8f9c 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -867,8 +867,7 @@ static char *guess_ref(const char *name, struct ref *peer)\n \n static int match_explicit(struct ref *src, struct ref *dst,\n \t\t\t  struct ref ***dst_tail,\n-\t\t\t  struct refspec *rs,\n-\t\t\t  int errs)\n+\t\t\t  struct refspec *rs)\n {\n \tstruct ref *matched_src, *matched_dst;\n \n@@ -876,7 +875,7 @@ static int match_explicit(struct ref *src, struct ref *dst,\n \tchar *dst_guess;\n \n \tif (rs->pattern || rs->matching)\n-\t\treturn errs;\n+\t\treturn 0;\n \n \tmatched_src = matched_dst = NULL;\n \tswitch (count_refspec_match(rs->src, src, &matched_src)) {\n@@ -898,14 +897,12 @@ static int match_explicit(struct ref *src, struct ref *dst,\n \t}\n \n \tif (!matched_src)\n-\t\terrs = 1;\n+\t\treturn -1;\n \n \tif (!dst_value) {\n \t\tunsigned char sha1[20];\n \t\tint flag;\n \n-\t\tif (!matched_src)\n-\t\t\treturn errs;\n \t\tdst_value = resolve_ref(matched_src->name, sha1, 1, &flag);\n \t\tif (!dst_value ||\n \t\t    ((flag & REF_ISSYMREF) &&\n@@ -936,18 +933,16 @@ static int match_explicit(struct ref *src, struct ref *dst,\n \t\t      dst_value);\n \t\tbreak;\n \t}\n-\tif (errs || !matched_dst)\n-\t\treturn 1;\n-\tif (matched_dst->peer_ref) {\n-\t\terrs = 1;\n-\t\terror(\"dst ref %s receives from more than one src.\",\n+\tif (!matched_dst)\n+\t\treturn -1;\n+\tif (matched_dst->peer_ref)\n+\t\treturn error(\"dst ref %s receives from more than one src.\",\n \t\t      matched_dst->name);\n-\t}\n \telse {\n \t\tmatched_dst->peer_ref = matched_src;\n \t\tmatched_dst->force = rs->force;\n \t}\n-\treturn errs;\n+\treturn 0;\n }\n \n static int match_explicit_refs(struct ref *src, struct ref *dst,\n@@ -956,8 +951,8 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,\n {\n \tint i, errs;\n \tfor (i = errs = 0; i < rs_nr; i++)\n-\t\terrs |= match_explicit(src, dst, dst_tail, &rs[i], errs);\n-\treturn -errs;\n+\t\terrs += match_explicit(src, dst, dst_tail, &rs[i]);\n+\treturn errs;\n }\n \n static const struct refspec *check_pattern_match(const struct refspec *rs,\n-- \n1.5.6.rc3.8.gabbfb\n"},{"id":"80043","messageId":"20080616161754.GB7219@sigill.intra.peff.net","threadId":"13967","inReplyTo":"20080616161502.GA7219@sigill.intra.peff.net","subject":"Re: [Crash] git-push $remote $non_ref:$anything","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-16T16:17:55Z","receivedAt":"2008-06-16T16:17:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 16, 2008 at 12:15:02PM -0400, Jeff King wrote:\n\n>  \tif (!matched_src)\n> -\t\terrs = 1;\n> +\t\treturn -1;\n\nThis could maybe be cleaned up even more by just returning -1 at each\npoint where matched_src becomes NULL (in the switch statement above).\n\n-Peff\n"}]}