clean up error conventions of remote.c:match_explicit

match_explicit is called for each push refspec to try to
fully resolve the source and destination sides of the
refspec.  Currently, we look at each refspec and report
errors on both the source and the dest side before aborting.

It makes sense to report errors for each refspec, since an
error in one is independent of an error in the other.
However, reporting errors on the 'dst' side of a refspec if
there has been an error on the 'src' side does not
necessarily make sense, since the interpretation of the
'dst' side depends on the 'src' side (for example, when
creating a new unqualified remote ref, we use the same type
as the src ref).

This patch lets match_explicit return early when the src
side of the refspec is bogus. We still look at all of the
refspecs before aborting the push, though.

At the same time, we clean up the call signature, which
previously took an extra "errs" flag. This was pointless, as
we didn't act on that flag, but rather just passed it back
to the caller. Instead, we now use the more traditional
"return -1" to signal an error, and the caller aggregates
the error count.

This change fixes two bugs, as well:

  - the early return avoids a segfault when passing a NULL
    matched_src to guess_ref()

  - the check for multiple sources pointing to a single dest
    aborted if the "err" flag was set. Presumably the intent
    was not to bother with the check if we had no
    matched_src. However, since the err flag was passed in
    from the caller, we might abort the check just because a
    previous refspec had a problem, which doesn't make
    sense.

    In practice, this didn't matter, since due to the error
    flag we end up aborting the push anyway.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/remote.c b/remote.c
index 91e3b112bb5ffa151a8ede9391d3bd88ea4d3500..ff2c8021678fee5ef12060d35c6eb1960e8792c7 100644 (file)
--- a/remote.c
+++ b/remote.c
@@ -867,8 +867,7 @@ static char *guess_ref(const char *name, struct ref *peer)
 
 static int match_explicit(struct ref *src, struct ref *dst,
                          struct ref ***dst_tail,
-                         struct refspec *rs,
-                         int errs)
+                         struct refspec *rs)
 {
        struct ref *matched_src, *matched_dst;
 
@@ -876,7 +875,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
        char *dst_guess;
 
        if (rs->pattern || rs->matching)
-               return errs;
+               return 0;
 
        matched_src = matched_dst = NULL;
        switch (count_refspec_match(rs->src, src, &matched_src)) {
@@ -889,23 +888,16 @@ static int match_explicit(struct ref *src, struct ref *dst,
                 */
                matched_src = try_explicit_object_name(rs->src);
                if (!matched_src)
-                       error("src refspec %s does not match any.", rs->src);
+                       return error("src refspec %s does not match any.", rs->src);
                break;
        default:
-               matched_src = NULL;
-               error("src refspec %s matches more than one.", rs->src);
-               break;
+               return error("src refspec %s matches more than one.", rs->src);
        }
 
-       if (!matched_src)
-               errs = 1;
-
        if (!dst_value) {
                unsigned char sha1[20];
                int flag;
 
-               if (!matched_src)
-                       return errs;
                dst_value = resolve_ref(matched_src->name, sha1, 1, &flag);
                if (!dst_value ||
                    ((flag & REF_ISSYMREF) &&
@@ -936,18 +928,16 @@ static int match_explicit(struct ref *src, struct ref *dst,
                      dst_value);
                break;
        }
-       if (errs || !matched_dst)
-               return 1;
-       if (matched_dst->peer_ref) {
-               errs = 1;
-               error("dst ref %s receives from more than one src.",
+       if (!matched_dst)
+               return -1;
+       if (matched_dst->peer_ref)
+               return error("dst ref %s receives from more than one src.",
                      matched_dst->name);
-       }
        else {
                matched_dst->peer_ref = matched_src;
                matched_dst->force = rs->force;
        }
-       return errs;
+       return 0;
 }
 
 static int match_explicit_refs(struct ref *src, struct ref *dst,
@@ -956,8 +946,8 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
 {
        int i, errs;
        for (i = errs = 0; i < rs_nr; i++)
-               errs |= match_explicit(src, dst, dst_tail, &rs[i], errs);
-       return -errs;
+               errs += match_explicit(src, dst, dst_tail, &rs[i]);
+       return errs;
 }
 
 static const struct refspec *check_pattern_match(const struct refspec *rs,

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 6946557c677ccc190309cd5961b046c420174f7d..df7750f7d1ede6e888ec3071397115022e067eb3 100755 (executable)
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -37,7 +37,8 @@ test_expect_success "clone and setup child repos" '
                echo "Pull: refs/heads/one:refs/heads/one"
        } >.git/remotes/two &&
        cd .. &&
-       git clone . bundle
+       git clone . bundle &&
+       git clone . seven
 '
 
 test_expect_success "fetch test" '
@@ -295,4 +296,11 @@ test_expect_success 'configured fetch updates tracking' '
        )
 '
 
+test_expect_success 'pushing nonexistent branch by mistake should not segv' '
+
+       cd "$D" &&
+       test_must_fail git push seven no:no
+
+'
+
 test_done