Merge pull request #1183 from congyiwu/push_delete_fix

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

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;
 				}
 			}
 		}

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;
 }

tests-clar/network/push.c

@@ -453,6 +453,7 @@ void test_network_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 } };
@@ -464,15 +465,18 @@ void test_network_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),