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

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

Author: ethomson

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.
 

include/git2/push.h

@@ -128,28 +128,21 @@ 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
  *
  * @return 0 or an error code
  */
 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
  *

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,

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;

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