Fix bug in gen_pktline() for deletes of missing remote refs

* gen_pktline() in smart_protocol.c was skipping refspecs that deleted
  refs that were not advertised by the server.  The new behavior is to
  send a delete command with an old-id of zero, which matches the behavior
  of the official git client.
* Update test_network_push__delete() in reaction to above fix.
* Obviate messy logic that handles missing push_spec rrefs by canonicalizing
  push_spec.  After calculate_work(), loid, roid, and rref, are filled in with
  exactly what is sent to the server

diff --git a/src/push.c b/src/push.c
index 1d63d574e833d56cccf017ad48311f417a52549f..efca743b3b95ce90804ebb21a7f990772b0dc808 100644 (file)
--- a/src/push.c
+++ b/src/push.c
@@ -101,24 +101,27 @@ static int parse_refspec(push_spec **spec, const char *str)
        if (delim == NULL) {
                s->lref = git__strdup(str);
                check(s->lref);
-               s->rref = NULL;
        } else {
                if (delim - str) {
                        s->lref = git__strndup(str, delim - str);
                        check(s->lref);
-               } else
-                       s->lref = NULL;
+               }
 
                if (strlen(delim + 1)) {
                        s->rref = git__strdup(delim + 1);
                        check(s->rref);
-               } else
-                       s->rref = NULL;
+               }
        }
 
        if (!s->lref && !s->rref)
                goto on_error;
 
+       /* If rref is ommitted, use the same ref name as lref */
+       if (!s->rref) {
+               s->rref = git__strdup(s->lref);
+               check(s->rref);
+       }
+
 #undef check
 
        *spec = s;
@@ -282,44 +285,24 @@ static int calculate_work(git_push *push)
        push_spec *spec;
        unsigned int i, j;
 
+       /* Update local and remote oids*/
+
        git_vector_foreach(&push->specs, i, spec) {
                if (spec->lref) {
+                       /* This is a create or update.  Local ref must exist. */
                        if (git_reference_name_to_id(
                                        &spec->loid, push->repo, spec->lref) < 0) {
                                giterr_set(GIT_ENOTFOUND, "No such reference '%s'", spec->lref);
                                return -1;
                        }
+               }
 
-                       if (!spec->rref) {
-                               /*
-                                * No remote reference given; if we find a remote
-                                * reference with the same name we will update it,
-                                * otherwise a new reference will be created.
-                                */
-                               git_vector_foreach(&push->remote->refs, j, head) {
-                                       if (!strcmp(spec->lref, head->name)) {
-                                               /*
-                                                * Update remote reference
-                                                */
-                                               git_oid_cpy(&spec->roid, &head->oid);
-
-                                               break;
-                                       }
-                               }
-                       } else {
-                               /*
-                                * Remote reference given; update the given
-                                * reference or create it.
-                                */
-                               git_vector_foreach(&push->remote->refs, j, head) {
-                                       if (!strcmp(spec->rref, head->name)) {
-                                               /*
-                                                * Update remote reference
-                                                */
-                                               git_oid_cpy(&spec->roid, &head->oid);
-
-                                               break;
-                                       }
+               if (spec->rref) {
+                       /* Remote ref may or may not (e.g. during create) already exist. */
+                       git_vector_foreach(&push->remote->refs, j, head) {
+                               if (!strcmp(spec->rref, head->name)) {
+                                       git_oid_cpy(&spec->roid, &head->oid);
+                                       break;
                                }
                        }
                }

diff --git a/src/transports/smart_protocol.c b/src/transports/smart_protocol.c
index a7331597541352cebb2dcf569331d2626d1622c9..34f0f8449315bc530323bdf3a97ef99bcd8e5e17 100644 (file)
--- a/src/transports/smart_protocol.c
+++ b/src/transports/smart_protocol.c
@@ -499,61 +499,25 @@ on_error:
 
 static int gen_pktline(git_buf *buf, git_push *push)
 {
-       git_remote_head *head;
        push_spec *spec;
-       unsigned int i, j, len;
-       char hex[41]; hex[40] = '\0';
+       size_t i, len;
+       char old_id[41], new_id[41];
+
+       old_id[40] = '\0'; new_id[40] = '\0';
 
        git_vector_foreach(&push->specs, i, spec) {
-               len = 2*GIT_OID_HEXSZ + 7;
+               len = 2*GIT_OID_HEXSZ + 7 + strlen(spec->rref);
 
                if (i == 0) {
-                       len +=1; /* '\0' */
+                       ++len; /* '\0' */
                        if (push->report_status)
                                len += strlen(GIT_CAP_REPORT_STATUS);
                }
 
-               if (spec->lref) {
-                       len += spec->rref ? strlen(spec->rref) : strlen(spec->lref);
+               git_oid_fmt(old_id, &spec->roid);
+               git_oid_fmt(new_id, &spec->loid);
 
-                       if (git_oid_iszero(&spec->roid)) {
-
-                               /*
-                                * Create remote reference
-                                */
-                               git_oid_fmt(hex, &spec->loid);
-                               git_buf_printf(buf, "%04x%s %s %s", len,
-                                       GIT_OID_HEX_ZERO, hex,
-                                       spec->rref ? spec->rref : spec->lref);
-
-                       } else {
-
-                               /*
-                                * Update remote reference
-                                */
-                               git_oid_fmt(hex, &spec->roid);
-                               git_buf_printf(buf, "%04x%s ", len, hex);
-
-                               git_oid_fmt(hex, &spec->loid);
-                               git_buf_printf(buf, "%s %s", hex,
-                                       spec->rref ? spec->rref : spec->lref);
-                       }
-               } else {
-                       /*
-                        * Delete remote reference
-                        */
-                       git_vector_foreach(&push->remote->refs, j, head) {
-                               if (!strcmp(spec->rref, head->name)) {
-                                       len += strlen(spec->rref);
-
-                                       git_oid_fmt(hex, &head->oid);
-                                       git_buf_printf(buf, "%04x%s %s %s", len,
-                                                      hex, GIT_OID_HEX_ZERO, head->name);
-
-                                       break;
-                               }
-                       }
-               }
+               git_buf_printf(buf, "%04x%s %s %s", len, old_id, new_id, spec->rref);
 
                if (i == 0) {
                        git_buf_putc(buf, '\0');
@@ -563,6 +527,7 @@ static int gen_pktline(git_buf *buf, git_push *push)
 
                git_buf_putc(buf, '\n');
        }
+
        git_buf_puts(buf, "0000");
        return git_buf_oom(buf) ? -1 : 0;
 }

diff --git a/tests-clar/online/push.c b/tests-clar/online/push.c
index d8ee4c2ee46d61addc27bac46f09810a6c4266b2..9d949b77b9bb31b498b2732db3455868e37b7919 100644 (file)
--- a/tests-clar/online/push.c
+++ b/tests-clar/online/push.c
@@ -451,6 +451,7 @@ void test_online_push__delete(void)
        const char *specs_del_fake[] = { ":refs/heads/fake" };
        /* Force has no effect for delete. */
        const char *specs_del_fake_force[] = { "+:refs/heads/fake" };
+       push_status exp_stats_fake[] = { { "refs/heads/fake", NULL } };
 
        const char *specs_delete[] = { ":refs/heads/tgt1" };
        push_status exp_stats_delete[] = { { "refs/heads/tgt1", NULL } };
@@ -462,15 +463,18 @@ void test_online_push__delete(void)
                exp_stats1, ARRAY_SIZE(exp_stats1),
                exp_refs1, ARRAY_SIZE(exp_refs1), 0);
 
-       /* Deleting a non-existent branch should fail before the request is sent to
-        * the server because the client cannot find the old oid for the ref.
+       /* When deleting a non-existent branch, the git client sends zero for both
+        * the old and new commit id.  This should succeed on the server with the
+        * same status report as if the branch were actually deleted.  The server
+        * returns a warning on the side-band iff the side-band is supported.
+        *  Since libgit2 doesn't support the side-band yet, there are no warnings.
         */
        do_push(specs_del_fake, ARRAY_SIZE(specs_del_fake),
-               NULL, 0,
-               exp_refs1, ARRAY_SIZE(exp_refs1), -1);
+               exp_stats_fake, 1,
+               exp_refs1, ARRAY_SIZE(exp_refs1), 0);
        do_push(specs_del_fake_force, ARRAY_SIZE(specs_del_fake_force),
-               NULL, 0,
-               exp_refs1, ARRAY_SIZE(exp_refs1), -1);
+               exp_stats_fake, 1,
+               exp_refs1, ARRAY_SIZE(exp_refs1), 0);
 
        /* Delete one of the pushed branches. */
        do_push(specs_delete, ARRAY_SIZE(specs_delete),