Merge pull request #2762 from libgit2/cmn/hide-push

remote: remove git_push from the public API

CHANGELOG.md

@@ -93,8 +93,11 @@ 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.
+* The git_push struct to perform a push has been replaced with
+  git_remote_upload(). The refspecs and options are passed as a
+  function argument. git_push_update_tips() is now also
+  git_remote_update_tips() and the callbacks are in the same struct as
+  the rest.
 
 * Introduce git_merge_bases() and the git_oidarray type to expose all
   merge bases between two commits.

include/git2/push.h

@@ -59,116 +59,6 @@ typedef int (*git_push_transfer_progress)(
 	size_t bytes,
 	void* payload);
 
-/**
- * Create a new push object
- *
- * @param out New push object
- * @param remote Remote instance
- *
- * @return 0 or an error code
- */
-GIT_EXTERN(int) git_push_new(git_push **out, git_remote *remote);
-
-/**
- * Set options on a push object
- *
- * @param push The push object
- * @param opts The options to set on the push object
- *
- * @return 0 or an error code
- */
-GIT_EXTERN(int) git_push_set_options(
-	git_push *push,
-	const git_push_options *opts);
-
-/**
- * Set the callbacks for a push
- *
- * @param push The push object
- * @param pack_progress_cb Function to call with progress information during
- * pack building. Be aware that this is called inline with pack building
- * operations, so performance may be affected.
- * @param pack_progress_cb_payload Payload for the pack progress callback.
- * @param transfer_progress_cb Function to call with progress information during
- * the upload portion of a push. Be aware that this is called inline with
- * pack building operations, so performance may be affected.
- * @param transfer_progress_cb_payload Payload for the network progress callback.
- * @return 0 or an error code
- */
-GIT_EXTERN(int) git_push_set_callbacks(
-	git_push *push,
-	git_packbuilder_progress pack_progress_cb,
-	void *pack_progress_cb_payload,
-	git_push_transfer_progress transfer_progress_cb,
-	void *transfer_progress_cb_payload);
-
-/**
- * Add a refspec to be pushed
- *
- * @param push The push object
- * @param refspec Refspec string
- *
- * @return 0 or an error code
- */
-GIT_EXTERN(int) git_push_add_refspec(git_push *push, const char *refspec);
-
-/**
- * Update remote tips after a push
- *
- * @param push The push object
- * @param signature The identity to use when updating reflogs
- * @param reflog_message The message to insert into the reflogs. If NULL, the
- *                       default is "update by push".
- *
- * @return 0 or an error code
- */
-GIT_EXTERN(int) git_push_update_tips(
-		git_push *push,
-		const git_signature *signature,
-		const char *reflog_message);
-
-/**
- * Perform the push
- *
- * 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);
-
-/**
- * Invoke callback `cb' on each status entry
- *
- * For each of the updated references, we receive a status report in the
- * form of `ok refs/heads/master` or `ng refs/heads/master <msg>`.
- * `msg != NULL` means the reference has not been updated for the given
- * reason.
- *
- * Return a non-zero value from the callback to stop the loop.
- *
- * @param push The push object
- * @param cb The callback to call on each object
- *
- * @return 0 on success, non-zero callback return value, or error code
- */
-GIT_EXTERN(int) git_push_status_foreach(git_push *push,
-			int (*cb)(const char *ref, const char *msg, void *data),
-			void *data);
-
-/**
- * Free the given push object
- *
- * @param push The push object
- */
-GIT_EXTERN(void) git_push_free(git_push *push);
-
 /** @} */
 GIT_END_DECL
 #endif

include/git2/remote.h

@@ -320,6 +320,19 @@ GIT_EXTERN(int) git_remote_ls(const git_remote_head ***out,  size_t *size, git_r
 GIT_EXTERN(int) git_remote_download(git_remote *remote, const git_strarray *refspecs);
 
 /**
+ * Create a packfile and send it to the server
+ *
+ * Connect to the remote if it hasn't been done yet, negotiate with
+ * the remote git which objects are missing, create a packfile with the missing objects and send it.
+ *
+ * @param remote the remote
+ * @param refspecs the refspecs to use for this negotiation and
+ * upload. Use NULL or an empty array to use the base refspecs
+ * @return 0 or an error code
+ */
+GIT_EXTERN(int) git_remote_upload(git_remote *remote, const git_strarray *refspecs, const git_push_options *opts);
+
+/**
  * Check whether the remote is connected
  *
  * Check whether the remote's underlying transport is connected to the
@@ -407,7 +420,7 @@ GIT_EXTERN(int) git_remote_fetch(
  * @param reflog_message message to use for the reflog of upated references
  */
 GIT_EXTERN(int) git_remote_push(git_remote *remote,
-				git_strarray *refspecs,
+				const git_strarray *refspecs,
 				const git_push_options *opts,
 				const git_signature *signature, const char *reflog_message);
 

src/push.h

@@ -51,4 +51,114 @@ struct git_push {
  */
 void git_push_status_free(push_status *status);
 
+/**
+ * Create a new push object
+ *
+ * @param out New push object
+ * @param remote Remote instance
+ *
+ * @return 0 or an error code
+ */
+int git_push_new(git_push **out, git_remote *remote);
+
+/**
+ * Set options on a push object
+ *
+ * @param push The push object
+ * @param opts The options to set on the push object
+ *
+ * @return 0 or an error code
+ */
+int git_push_set_options(
+	git_push *push,
+	const git_push_options *opts);
+
+/**
+ * Set the callbacks for a push
+ *
+ * @param push The push object
+ * @param pack_progress_cb Function to call with progress information during
+ * pack building. Be aware that this is called inline with pack building
+ * operations, so performance may be affected.
+ * @param pack_progress_cb_payload Payload for the pack progress callback.
+ * @param transfer_progress_cb Function to call with progress information during
+ * the upload portion of a push. Be aware that this is called inline with
+ * pack building operations, so performance may be affected.
+ * @param transfer_progress_cb_payload Payload for the network progress callback.
+ * @return 0 or an error code
+ */
+int git_push_set_callbacks(
+	git_push *push,
+	git_packbuilder_progress pack_progress_cb,
+	void *pack_progress_cb_payload,
+	git_push_transfer_progress transfer_progress_cb,
+	void *transfer_progress_cb_payload);
+
+/**
+ * Add a refspec to be pushed
+ *
+ * @param push The push object
+ * @param refspec Refspec string
+ *
+ * @return 0 or an error code
+ */
+int git_push_add_refspec(git_push *push, const char *refspec);
+
+/**
+ * Update remote tips after a push
+ *
+ * @param push The push object
+ * @param signature The identity to use when updating reflogs
+ * @param reflog_message The message to insert into the reflogs. If NULL, the
+ *                       default is "update by push".
+ *
+ * @return 0 or an error code
+ */
+int git_push_update_tips(
+		git_push *push,
+		const git_signature *signature,
+		const char *reflog_message);
+
+/**
+ * Perform the push
+ *
+ * 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
+ */
+int git_push_finish(git_push *push);
+
+/**
+ * Invoke callback `cb' on each status entry
+ *
+ * For each of the updated references, we receive a status report in the
+ * form of `ok refs/heads/master` or `ng refs/heads/master <msg>`.
+ * `msg != NULL` means the reference has not been updated for the given
+ * reason.
+ *
+ * Return a non-zero value from the callback to stop the loop.
+ *
+ * @param push The push object
+ * @param cb The callback to call on each object
+ *
+ * @return 0 on success, non-zero callback return value, or error code
+ */
+int git_push_status_foreach(git_push *push,
+			int (*cb)(const char *ref, const char *msg, void *data),
+			void *data);
+
+/**
+ * Free the given push object
+ *
+ * @param push The push object
+ */
+void git_push_free(git_push *push);
+
 #endif

src/remote.c

@@ -18,6 +18,7 @@
 #include "refs.h"
 #include "refspec.h"
 #include "fetchhead.h"
+#include "push.h"
 
 static int dwim_refspecs(git_vector *out, git_vector *refspecs, git_vector *refs);
 
@@ -1275,6 +1276,11 @@ int git_remote_update_tips(
 	int error;
 	size_t i;
 
+	/* push has its own logic hidden away in the push object */
+	if (remote->push) {
+		return git_push_update_tips(remote->push, signature, reflog_message);
+	}
+
 	if (git_refspec__parse(&tagspec, GIT_REFSPEC_TAGS, true) < 0)
 		return -1;
 
@@ -1355,6 +1361,7 @@ void git_remote_free(git_remote *remote)
 	free_refspecs(&remote->passive_refspecs);
 	git_vector_free(&remote->passive_refspecs);
 
+	git_push_free(remote->push);
 	git__free(remote->url);
 	git__free(remote->pushurl);
 	git__free(remote->name);
@@ -2117,22 +2124,29 @@ int git_remote_default_branch(git_buf *out, git_remote *remote)
 	return git_buf_puts(out, guess->name);
 }
 
-int git_remote_push(git_remote *remote, git_strarray *refspecs, const git_push_options *opts,
-		    const git_signature *signature, const char *reflog_message)
+int git_remote_upload(git_remote *remote, const git_strarray *refspecs, const git_push_options *opts)
 {
-	int error;
 	size_t i;
-	git_push *push = NULL;
-	git_remote_callbacks *cbs;
+	int error;
+	git_push *push;
 	git_refspec *spec;
+	git_remote_callbacks *cbs;
 
-	assert(remote && refspecs);
+	assert(remote);
 
-	if ((error = git_remote_connect(remote, GIT_DIRECTION_PUSH)) < 0)
+	if (!git_remote_connected(remote) &&
+	    (error = git_remote_connect(remote, GIT_DIRECTION_PUSH)) < 0)
+		goto cleanup;
+
+	if (remote->push) {
+		git_push_free(remote->push);
+		remote->push = NULL;
+	}
+
+	if ((error = git_push_new(&remote->push, remote)) < 0)
 		return error;
 
-	if ((error = git_push_new(&push, remote)) < 0)
-		goto cleanup;
+	push = remote->push;
 
 	if (opts && (error = git_push_set_options(push, opts)) < 0)
 		goto cleanup;
@@ -2164,10 +2178,25 @@ int git_remote_push(git_remote *remote, git_strarray *refspecs, const git_push_o
 	    (error = git_push_status_foreach(push, cbs->push_update_reference, cbs->payload)) < 0)
 		goto cleanup;
 
-	error = git_push_update_tips(push, signature, reflog_message);
-
 cleanup:
+	return error;
+}
+
+int git_remote_push(git_remote *remote, const git_strarray *refspecs, const git_push_options *opts,
+		    const git_signature *signature, const char *reflog_message)
+{
+	int error;
+
+	assert(remote && refspecs);
+
+	if ((error = git_remote_connect(remote, GIT_DIRECTION_PUSH)) < 0)
+		return error;
+
+	if ((error = git_remote_upload(remote, refspecs, opts)) < 0)
+		return error;
+
+	error = git_remote_update_tips(remote, signature, reflog_message);
+
 	git_remote_disconnect(remote);
-	git_push_free(push);
 	return error;
 }

src/remote.h

@@ -28,6 +28,7 @@ struct git_remote {
 	void *transport_cb_payload;
 	git_transport *transport;
 	git_repository *repo;
+	git_push *push;
 	git_remote_callbacks callbacks;
 	git_transfer_progress stats;
 	unsigned int need_pack;

tests/network/remote/local.c

@@ -7,6 +7,14 @@ static git_repository *repo;
 static git_buf file_path_buf = GIT_BUF_INIT;
 static git_remote *remote;
 
+static char *push_refspec_strings[] = {
+	"refs/heads/master",
+};
+static git_strarray push_array = {
+	push_refspec_strings,
+	1,
+};
+
 void test_network_remote_local__initialize(void)
 {
 	cl_git_pass(git_repository_init(&repo, "remotelocal/", 0));
@@ -191,9 +199,9 @@ void test_network_remote_local__push_to_bare_remote(void)
 		refspec_strings,
 		1,
 	};
+
 	/* Should be able to push to a bare remote */
 	git_remote *localremote;
-	git_push *push;
 
 	/* Get some commits */
 	connect_to_local_repository(cl_fixture("testrepo.git"));
@@ -213,12 +221,9 @@ void test_network_remote_local__push_to_bare_remote(void)
 	cl_git_pass(git_remote_connect(localremote, GIT_DIRECTION_PUSH));
 
 	/* Try to push */
-	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_git_pass(git_remote_upload(remote, &push_array, NULL));
 
 	/* Clean up */
-	git_push_free(push);
 	git_remote_free(localremote);
 	cl_fixture_cleanup("localbare.git");
 }
@@ -234,7 +239,6 @@ void test_network_remote_local__push_to_bare_remote_with_file_url(void)
 	};
 	/* Should be able to push to a bare remote */
 	git_remote *localremote;
-	git_push *push;
 	const char *url;
 
 	/* Get some commits */
@@ -258,12 +262,9 @@ void test_network_remote_local__push_to_bare_remote_with_file_url(void)
 	cl_git_pass(git_remote_connect(localremote, GIT_DIRECTION_PUSH));
 
 	/* Try to push */
-	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_git_pass(git_remote_upload(remote, &push_array, NULL));
 
 	/* Clean up */
-	git_push_free(push);
 	git_remote_free(localremote);
 	cl_fixture_cleanup("localbare.git");
 }
@@ -280,7 +281,6 @@ void test_network_remote_local__push_to_non_bare_remote(void)
 	};
 	/* Shouldn't be able to push to a non-bare remote */
 	git_remote *localremote;
-	git_push *push;
 
 	/* Get some commits */
 	connect_to_local_repository(cl_fixture("testrepo.git"));
@@ -300,12 +300,9 @@ void test_network_remote_local__push_to_non_bare_remote(void)
 	cl_git_pass(git_remote_connect(localremote, GIT_DIRECTION_PUSH));
 
 	/* Try to push */
-	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_git_fail_with(GIT_EBAREREPO, git_remote_upload(localremote, &push_array, NULL));
 
 	/* Clean up */
-	git_push_free(push);
 	git_remote_free(localremote);
 	cl_fixture_cleanup("localbare.git");
 }
@@ -433,7 +430,6 @@ void test_network_remote_local__update_tips_for_new_remote(void) {
 	git_repository *src_repo;
 	git_repository *dst_repo;
 	git_remote *new_remote;
-	git_push *push;
 	git_reference* branch;
 
 	/* Copy test repo */
@@ -446,16 +442,13 @@ void test_network_remote_local__update_tips_for_new_remote(void) {
 	/* Push to bare repo */
 	cl_git_pass(git_remote_create(&new_remote, src_repo, "bare", "./localbare.git"));
 	cl_git_pass(git_remote_connect(new_remote, GIT_DIRECTION_PUSH));
-	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_git_pass(git_remote_upload(new_remote, &push_array, NULL));
 
 	/* Update tips and make sure remote branch has been created */
-	cl_git_pass(git_push_update_tips(push, NULL, NULL));
+	cl_git_pass(git_remote_update_tips(new_remote, NULL, NULL));
 	cl_git_pass(git_branch_lookup(&branch, src_repo, "bare/master", GIT_BRANCH_REMOTE));
 
 	git_reference_free(branch);
-	git_push_free(push);
 	git_remote_free(new_remote);
 	git_repository_free(dst_repo);
 	cl_fixture_cleanup("localbare.git");

tests/network/remote/remotes.c

@@ -72,7 +72,14 @@ void test_network_remote_remotes__error_when_not_found(void)
 void test_network_remote_remotes__error_when_no_push_available(void)
 {
 	git_remote *r;
-	git_push *p;
+	char *specs = {
+		"refs/heads/master",
+	};
+	git_strarray arr = {
+		&specs,
+		1,
+	};
+
 
 	cl_git_pass(git_remote_create_anonymous(&r, _repo, cl_fixture("testrepo.git"), NULL));
 
@@ -83,11 +90,8 @@ void test_network_remote_remotes__error_when_no_push_available(void)
 	/* Make sure that push is really not available */
 	r->transport->push = NULL;
 
-	cl_git_pass(git_push_new(&p, r));
-	cl_git_pass(git_push_add_refspec(p, "refs/heads/master"));
-	cl_git_fail_with(git_push_finish(p), GIT_ERROR);
+	cl_git_fail_with(-1, git_remote_upload(r, &arr, NULL));
 
-	git_push_free(p);
 	git_remote_free(r);
 }
 

tests/online/push.c

@@ -198,7 +198,7 @@ static void verify_tracking_branches(git_remote *remote, expected_ref expected_r
 	git_branch_t branch_type;
 	git_reference *ref;
 
-	/* Get current remote branches */
+	/* Get current remote-tracking branches */
 	cl_git_pass(git_branch_iterator_new(&iter, remote->repo, GIT_BRANCH_REMOTE));
 
 	while ((error = git_branch_next(&ref, &branch_type, iter)) == 0) {
@@ -215,7 +215,7 @@ static void verify_tracking_branches(git_remote *remote, expected_ref expected_r
 	/* Loop through expected refs, make sure they exist */
 	for (i = 0; i < expected_refs_len; i++) {
 
-		/* Convert remote reference name into tracking branch name.
+		/* Convert remote reference name into remote-tracking branch name.
 		 * If the spec is not under refs/heads/, then skip.
 		 */
 		fetch_spec = git_remote__matching_refspec(remote, expected_refs[i].name);
@@ -318,8 +318,7 @@ void test_online_push__initialize(void)
 {
 	git_vector delete_specs = GIT_VECTOR_INIT;
 	const git_remote_head **heads;
-	size_t i, heads_len;
-	char *curr_del_spec;
+	size_t heads_len;
 
 	_repo = cl_git_sandbox_init("push_src");
 
@@ -382,17 +381,12 @@ void test_online_push__initialize(void)
 	cl_git_pass(git_remote_ls(&heads, &heads_len, _remote));
 	cl_git_pass(create_deletion_refspecs(&delete_specs, heads, heads_len));
 	if (delete_specs.length) {
-		git_push *push;
-
-		cl_git_pass(git_push_new(&push, _remote));
-
-		git_vector_foreach(&delete_specs, i, curr_del_spec) {
-			git_push_add_refspec(push, curr_del_spec);
-			git__free(curr_del_spec);
-		}
+		git_strarray arr = {
+			(char **) delete_specs.contents,
+			delete_specs.length,
+		};
 
-		cl_git_pass(git_push_finish(push));
-		git_push_free(push);
+		cl_git_pass(git_remote_upload(_remote, &arr, NULL));
 	}
 
 	git_remote_disconnect(_remote);
@@ -818,30 +812,24 @@ void test_online_push__bad_refspecs(void)
 	/* All classes of refspecs that should be rejected by
 	 * git_push_add_refspec() should go in this test.
 	 */
-	git_push *push;
+	char *specs = {
+		"b6:b6",
+	};
+	git_strarray arr = {
+		&specs,
+		1,
+	};
 
 	if (_remote) {
-/*		cl_git_pass(git_remote_connect(_remote, GIT_DIRECTION_PUSH)); */
-		cl_git_pass(git_push_new(&push, _remote));
-
-		/* Unexpanded branch names not supported */
-		cl_git_fail(git_push_add_refspec(push, "b6:b6"));
-
-		git_push_free(push);
+		cl_git_fail(git_remote_upload(_remote, &arr, NULL));
 	}
 }
 
 void test_online_push__expressions(void)
 {
-	git_push *push;
-
 	/* TODO: Expressions in refspecs doesn't actually work yet */
 	const char *specs_left_expr[] = { "refs/heads/b2~1:refs/heads/b2" };
 
-	cl_git_pass(git_push_new(&push, _remote));
-	cl_git_fail(git_push_add_refspec(push, "refs/heads/b2:refs/heads/b2~1"));
-	git_push_free(push);
-
 	/* TODO: Find a more precise way of checking errors than a exit code of -1. */
 	do_push(specs_left_expr, ARRAY_SIZE(specs_left_expr),
 		NULL, 0,