Merge pull request #2756 from libgit2/cmn/push-error-concerns

Fold `git_push_unpack_ok()` into `git_push_finish()`

5 files changed, 15 insertions, 29 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index a2c219124..cfb4f0430 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -88,6 +88,9 @@ v0.21 + 1
 * Rename git_remote_load() to git_remote_lookup() to bring it in line
   with the rest of the lookup functions.
 
+* git_push_unpack_ok() has been removed and git_push_finish() now
+  returns an error if the unpacking failed.
+
 * Introduce git_merge_bases() and the git_oidarray type to expose all
   merge bases between two commits.
 
diff --git a/include/git2/push.h b/include/git2/push.h
index cbf115661..7bd1377e9 100644
--- a/include/git2/push.h
+++ b/include/git2/push.h
@@ -128,12 +128,14 @@ GIT_EXTERN(int) git_push_update_tips(
 		const char *reflog_message);
 
 /**
- * Actually push all given refspecs
+ * Perform the push
  *
- * Note: To check if the push was successful (i.e. all remote references
- * have been updated as requested), you need to call both
- * `git_push_unpack_ok` and `git_push_status_foreach`. The remote
- * repository might have refused to update some or all of the references.
+ * This function will return an error in case of a protocol error or
+ * the server being unable to unpack the data we sent.
+ *
+ * The return value does not reflect whether the server accepted or
+ * refused any reference updates. Use `git_push_status_foreach()` in
+ * order to find out which updates were accepted or rejected.
  *
  * @param push The push object
  *
@@ -142,15 +144,6 @@ GIT_EXTERN(int) git_push_update_tips(
 GIT_EXTERN(int) git_push_finish(git_push *push);
 
 /**
- * Check if remote side successfully unpacked
- *
- * @param push The push object
- *
- * @return true if remote side successfully unpacked, false otherwise
- */
-GIT_EXTERN(int) git_push_unpack_ok(const git_push *push);
-
-/**
  * Invoke callback `cb' on each status entry
  *
  * For each of the updated references, we receive a status report in the
diff --git a/src/push.c b/src/push.c
index 6671da465..fb51383cb 100644
--- a/src/push.c
+++ b/src/push.c
@@ -632,12 +632,12 @@ int git_push_finish(git_push *push)
 		(error = do_push(push)) < 0)
 		return error;
 
-	return 0;
-}
+	if (!push->unpack_ok) {
+		error = -1;
+		giterr_set(GITERR_NET, "unpacking the sent packfile failed on the remote");
+	}
 
-int git_push_unpack_ok(const git_push *push)
-{
-	return push->unpack_ok;
+	return error;
 }
 
 int git_push_status_foreach(git_push *push,
diff --git a/src/remote.c b/src/remote.c
index 531912c64..dd9b17854 100644
--- a/src/remote.c
+++ b/src/remote.c
@@ -2160,12 +2160,6 @@ int git_remote_push(git_remote *remote, git_strarray *refspecs, const git_push_o
 	if ((error = git_push_finish(push)) < 0)
 		goto cleanup;
 
-	if (!git_push_unpack_ok(push)) {
-		error = -1;
-		giterr_set(GITERR_NET, "error in the remote while trying to unpack");
-		goto cleanup;
-	}
-
 	if (cbs->push_update_reference &&
 	    (error = git_push_status_foreach(push, cbs->push_update_reference, cbs->payload)) < 0)
 		goto cleanup;
diff --git a/tests/network/remote/local.c b/tests/network/remote/local.c
index 170d67ef9..baeb25df4 100644
--- a/tests/network/remote/local.c
+++ b/tests/network/remote/local.c
@@ -216,7 +216,6 @@ void test_network_remote_local__push_to_bare_remote(void)
 	cl_git_pass(git_push_new(&push, localremote));
 	cl_git_pass(git_push_add_refspec(push, "refs/heads/master"));
 	cl_git_pass(git_push_finish(push));
-	cl_assert(git_push_unpack_ok(push));
 
 	/* Clean up */
 	git_push_free(push);
@@ -262,7 +261,6 @@ void test_network_remote_local__push_to_bare_remote_with_file_url(void)
 	cl_git_pass(git_push_new(&push, localremote));
 	cl_git_pass(git_push_add_refspec(push, "refs/heads/master"));
 	cl_git_pass(git_push_finish(push));
-	cl_assert(git_push_unpack_ok(push));
 
 	/* Clean up */
 	git_push_free(push);
@@ -305,7 +303,6 @@ void test_network_remote_local__push_to_non_bare_remote(void)
 	cl_git_pass(git_push_new(&push, localremote));
 	cl_git_pass(git_push_add_refspec(push, "refs/heads/master"));
 	cl_git_fail_with(git_push_finish(push), GIT_EBAREREPO);
-	cl_assert_equal_i(0, git_push_unpack_ok(push));
 
 	/* Clean up */
 	git_push_free(push);
@@ -452,7 +449,6 @@ void test_network_remote_local__update_tips_for_new_remote(void) {
 	cl_git_pass(git_push_new(&push, new_remote));
 	cl_git_pass(git_push_add_refspec(push, "refs/heads/master"));
 	cl_git_pass(git_push_finish(push));
-	cl_assert(git_push_unpack_ok(push));
 
 	/* Update tips and make sure remote branch has been created */
 	cl_git_pass(git_push_update_tips(push, NULL, NULL));