Merge pull request #5195 from tiennou/fix/commitish-smart-push

smart: use push_glob instead of manual filtering

src/revwalk.c

@@ -38,7 +38,7 @@ git_commit_list_node *git_revwalk__commit_lookup(
 	return commit;
 }
 
-static int push_commit(git_revwalk *walk, const git_oid *oid, int uninteresting, int from_glob)
+int git_revwalk__push_commit(git_revwalk *walk, const git_oid *oid, const git_revwalk__push_options *opts)
 {
 	git_oid commit_id;
 	int error;
@@ -54,7 +54,7 @@ static int push_commit(git_revwalk *walk, const git_oid *oid, int uninteresting,
 
 	if (error == GIT_ENOTFOUND || error == GIT_EINVALIDSPEC || error == GIT_EPEEL) {
 		/* If this comes from e.g. push_glob("tags"), ignore this */
-		if (from_glob)
+		if (opts->from_glob)
 			return 0;
 
 		git_error_set(GIT_ERROR_INVALID, "object is not a committish");
@@ -74,16 +74,18 @@ static int push_commit(git_revwalk *walk, const git_oid *oid, int uninteresting,
 	if (commit->uninteresting)
 		return 0;
 
-	if (uninteresting) {
+	if (opts->uninteresting) {
 		walk->limited = 1;
 		walk->did_hide = 1;
 	} else {
 		walk->did_push = 1;
 	}
 
-	commit->uninteresting = uninteresting;
+	commit->uninteresting = opts->uninteresting;
 	list = walk->user_input;
-	if (git_commit_list_insert(commit, &list) == NULL) {
+	if ((opts->insert_by_date &&
+	    git_commit_list_insert_by_date(commit, &list) == NULL) ||
+	    git_commit_list_insert(commit, &list) == NULL) {
 		git_error_set_oom();
 		return -1;
 	}
@@ -95,29 +97,36 @@ static int push_commit(git_revwalk *walk, const git_oid *oid, int uninteresting,
 
 int git_revwalk_push(git_revwalk *walk, const git_oid *oid)
 {
+	git_revwalk__push_options opts = GIT_REVWALK__PUSH_OPTIONS_INIT;
+
 	assert(walk && oid);
-	return push_commit(walk, oid, 0, false);
+
+	return git_revwalk__push_commit(walk, oid, &opts);
 }
 
 
 int git_revwalk_hide(git_revwalk *walk, const git_oid *oid)
 {
+	git_revwalk__push_options opts = GIT_REVWALK__PUSH_OPTIONS_INIT;
 	assert(walk && oid);
-	return push_commit(walk, oid, 1, false);
+
+	opts.uninteresting = 1;
+	return git_revwalk__push_commit(walk, oid, &opts);
 }
 
-static int push_ref(git_revwalk *walk, const char *refname, int hide, int from_glob)
+int git_revwalk__push_ref(git_revwalk *walk, const char *refname, const git_revwalk__push_options *opts)
 {
 	git_oid oid;
 
 	if (git_reference_name_to_id(&oid, walk->repo, refname) < 0)
 		return -1;
 
-	return push_commit(walk, &oid, hide, from_glob);
+	return git_revwalk__push_commit(walk, &oid, opts);
 }
 
-static int push_glob(git_revwalk *walk, const char *glob, int hide)
+int git_revwalk__push_glob(git_revwalk *walk, const char *glob, const git_revwalk__push_options *given_opts)
 {
+	git_revwalk__push_options opts = GIT_REVWALK__PUSH_OPTIONS_INIT;
 	int error = 0;
 	git_buf buf = GIT_BUF_INIT;
 	git_reference *ref;
@@ -126,6 +135,9 @@ static int push_glob(git_revwalk *walk, const char *glob, int hide)
 
 	assert(walk && glob);
 
+	if (given_opts)
+		memcpy(&opts, given_opts, sizeof(opts));
+
 	/* refs/ is implied if not given in the glob */
 	if (git__prefixcmp(glob, GIT_REFS_DIR) != 0)
 		git_buf_joinpath(&buf, GIT_REFS_DIR, glob);
@@ -141,8 +153,9 @@ static int push_glob(git_revwalk *walk, const char *glob, int hide)
 	if ((error = git_reference_iterator_glob_new(&iter, walk->repo, buf.ptr)) < 0)
 		goto out;
 
+	opts.from_glob = true;
 	while ((error = git_reference_next(&ref, iter)) == 0) {
-		error = push_ref(walk, git_reference_name(ref), hide, true);
+		error = git_revwalk__push_ref(walk, git_reference_name(ref), &opts);
 		git_reference_free(ref);
 		if (error < 0)
 			break;
@@ -158,36 +171,49 @@ out:
 
 int git_revwalk_push_glob(git_revwalk *walk, const char *glob)
 {
+	git_revwalk__push_options opts = GIT_REVWALK__PUSH_OPTIONS_INIT;
 	assert(walk && glob);
-	return push_glob(walk, glob, 0);
+
+	return git_revwalk__push_glob(walk, glob, &opts);
 }
 
 int git_revwalk_hide_glob(git_revwalk *walk, const char *glob)
 {
+	git_revwalk__push_options opts = GIT_REVWALK__PUSH_OPTIONS_INIT;
 	assert(walk && glob);
-	return push_glob(walk, glob, 1);
+
+	opts.uninteresting = 1;
+	return git_revwalk__push_glob(walk, glob, &opts);
 }
 
 int git_revwalk_push_head(git_revwalk *walk)
 {
+	git_revwalk__push_options opts = GIT_REVWALK__PUSH_OPTIONS_INIT;
 	assert(walk);
-	return push_ref(walk, GIT_HEAD_FILE, 0, false);
+
+	return git_revwalk__push_ref(walk, GIT_HEAD_FILE, &opts);
 }
 
 int git_revwalk_hide_head(git_revwalk *walk)
 {
+	git_revwalk__push_options opts = GIT_REVWALK__PUSH_OPTIONS_INIT;
 	assert(walk);
-	return push_ref(walk, GIT_HEAD_FILE, 1, false);
+
+	opts.uninteresting = 1;
+	return git_revwalk__push_ref(walk, GIT_HEAD_FILE, &opts);
 }
 
 int git_revwalk_push_ref(git_revwalk *walk, const char *refname)
 {
+	git_revwalk__push_options opts = GIT_REVWALK__PUSH_OPTIONS_INIT;
 	assert(walk && refname);
-	return push_ref(walk, refname, 0, false);
+
+	return git_revwalk__push_ref(walk, refname, &opts);
 }
 
 int git_revwalk_push_range(git_revwalk *walk, const char *range)
 {
+	git_revwalk__push_options opts = GIT_REVWALK__PUSH_OPTIONS_INIT;
 	git_revspec revspec;
 	int error = 0;
 
@@ -207,10 +233,12 @@ int git_revwalk_push_range(git_revwalk *walk, const char *range)
 		goto out;
 	}
 
-	if ((error = push_commit(walk, git_object_id(revspec.from), 1, false)))
+	opts.uninteresting = 1;
+	if ((error = git_revwalk__push_commit(walk, git_object_id(revspec.from), &opts)))
 		goto out;
 
-	error = push_commit(walk, git_object_id(revspec.to), 0, false);
+	opts.uninteresting = 0;
+	error = git_revwalk__push_commit(walk, git_object_id(revspec.to), &opts);
 
 out:
 	git_object_free(revspec.from);
@@ -220,8 +248,10 @@ out:
 
 int git_revwalk_hide_ref(git_revwalk *walk, const char *refname)
 {
+	git_revwalk__push_options opts = GIT_REVWALK__PUSH_OPTIONS_INIT;
 	assert(walk && refname);
-	return push_ref(walk, refname, 1, false);
+	opts.uninteresting = 1;
+	return git_revwalk__push_ref(walk, refname, &opts);
 }
 
 static int revwalk_enqueue_timesort(git_revwalk *walk, git_commit_list_node *commit)

src/revwalk.h

@@ -50,4 +50,24 @@ struct git_revwalk {
 
 git_commit_list_node *git_revwalk__commit_lookup(git_revwalk *walk, const git_oid *oid);
 
+typedef struct {
+	int uninteresting;
+	int from_glob;
+	int insert_by_date;
+} git_revwalk__push_options;
+
+#define GIT_REVWALK__PUSH_OPTIONS_INIT { 0 }
+
+int git_revwalk__push_commit(git_revwalk *walk,
+	const git_oid *oid,
+	const git_revwalk__push_options *opts);
+
+int git_revwalk__push_ref(git_revwalk *walk,
+	const char *refname,
+	const git_revwalk__push_options *opts);
+
+int git_revwalk__push_glob(git_revwalk *walk,
+	const char *glob,
+	const git_revwalk__push_options *given_opts);
+
 #endif

src/transports/smart_protocol.c

@@ -17,6 +17,7 @@
 #include "pack-objects.h"
 #include "remote.h"
 #include "util.h"
+#include "revwalk.h"
 
 #define NETWORK_XFER_THRESHOLD (100*1024)
 /* The minimal interval between progress updates (in seconds). */
@@ -270,50 +271,6 @@ static int store_common(transport_smart *t)
 	return 0;
 }
 
-static int fetch_setup_walk(git_revwalk **out, git_repository *repo)
-{
-	git_revwalk *walk = NULL;
-	git_strarray refs;
-	unsigned int i;
-	git_reference *ref = NULL;
-	int error;
-
-	if ((error = git_reference_list(&refs, repo)) < 0)
-		return error;
-
-	if ((error = git_revwalk_new(&walk, repo)) < 0)
-		return error;
-
-	git_revwalk_sorting(walk, GIT_SORT_TIME);
-
-	for (i = 0; i < refs.count; ++i) {
-		git_reference_free(ref);
-		ref = NULL;
-
-		/* No tags */
-		if (!git__prefixcmp(refs.strings[i], GIT_REFS_TAGS_DIR))
-			continue;
-
-		if ((error = git_reference_lookup(&ref, repo, refs.strings[i])) < 0)
-			goto on_error;
-
-		if (git_reference_type(ref) == GIT_REFERENCE_SYMBOLIC)
-			continue;
-
-		if ((error = git_revwalk_push(walk, git_reference_target(ref))) < 0)
-			goto on_error;
-	}
-
-	*out = walk;
-
-on_error:
-	if (error)
-		git_revwalk_free(walk);
-	git_reference_free(ref);
-	git_strarray_free(&refs);
-	return error;
-}
-
 static int wait_while_ack(gitno_buffer *buf)
 {
 	int error;
@@ -347,6 +304,7 @@ static int wait_while_ack(gitno_buffer *buf)
 int git_smart__negotiate_fetch(git_transport *transport, git_repository *repo, const git_remote_head * const *wants, size_t count)
 {
 	transport_smart *t = (transport_smart *)transport;
+	git_revwalk__push_options opts = GIT_REVWALK__PUSH_OPTIONS_INIT;
 	gitno_buffer *buf = &t->buffer;
 	git_buf data = GIT_BUF_INIT;
 	git_revwalk *walk = NULL;
@@ -358,7 +316,11 @@ int git_smart__negotiate_fetch(git_transport *transport, git_repository *repo, c
 	if ((error = git_pkt_buffer_wants(wants, count, &t->caps, &data)) < 0)
 		return error;
 
-	if ((error = fetch_setup_walk(&walk, repo)) < 0)
+	if ((error = git_revwalk_new(&walk, repo)) < 0)
+		goto on_error;
+
+	opts.insert_by_date = 1;
+	if ((error = git_revwalk__push_glob(walk, "refs/*", &opts)) < 0)
 		goto on_error;
 
 	/*

tests/network/fetchlocal.c

@@ -419,7 +419,7 @@ void test_network_fetchlocal__multi_remotes(void)
 	cl_git_pass(git_remote_fetch(test, NULL, &options, NULL));
 
 	cl_git_pass(git_reference_list(&refnames, repo));
-	cl_assert_equal_i(32, (int)refnames.count);
+	cl_assert_equal_i(33, (int)refnames.count);
 	git_strarray_free(&refnames);
 
 	cl_git_pass(git_remote_set_url(repo, "test_with_pushurl", cl_git_fixture_url("testrepo.git")));
@@ -427,7 +427,7 @@ void test_network_fetchlocal__multi_remotes(void)
 	cl_git_pass(git_remote_fetch(test2, NULL, &options, NULL));
 
 	cl_git_pass(git_reference_list(&refnames, repo));
-	cl_assert_equal_i(44, (int)refnames.count);
+	cl_assert_equal_i(45, (int)refnames.count);
 
 	git_strarray_free(&refnames);
 	git_remote_free(test);

tests/network/remote/local.c

@@ -62,7 +62,7 @@ void test_network_remote_local__retrieve_advertised_references(void)
 
 	cl_git_pass(git_remote_ls(&refs, &refs_len, remote));
 
-	cl_assert_equal_i(refs_len, 28);
+	cl_assert_equal_i(refs_len, 29);
 }
 
 void test_network_remote_local__retrieve_advertised_before_connect(void)
@@ -86,7 +86,7 @@ void test_network_remote_local__retrieve_advertised_references_after_disconnect(
 
 	cl_git_pass(git_remote_ls(&refs, &refs_len, remote));
 
-	cl_assert_equal_i(refs_len, 28);
+	cl_assert_equal_i(refs_len, 29);
 }
 
 void test_network_remote_local__retrieve_advertised_references_from_spaced_repository(void)
@@ -101,7 +101,7 @@ void test_network_remote_local__retrieve_advertised_references_from_spaced_repos
 
 	cl_git_pass(git_remote_ls(&refs, &refs_len, remote));
 
-	cl_assert_equal_i(refs_len, 28);
+	cl_assert_equal_i(refs_len, 29);
 
 	git_remote_free(remote);	/* Disconnect from the "spaced repo" before the cleanup */
 	remote = NULL;

tests/refs/foreachglob.c

@@ -48,8 +48,8 @@ static void assert_retrieval(const char *glob, int expected_count)
 
 void test_refs_foreachglob__retrieve_all_refs(void)
 {
-	/* 12 heads (including one packed head) + 1 note + 2 remotes + 7 tags */
-	assert_retrieval("*", 22);
+	/* 12 heads (including one packed head) + 1 note + 2 remotes + 7 tags + 1 blob */
+	assert_retrieval("*", 23);
 }
 
 void test_refs_foreachglob__retrieve_remote_branches(void)

tests/refs/iterator.c

@@ -15,6 +15,7 @@ void test_refs_iterator__cleanup(void)
 }
 
 static const char *refnames[] = {
+	"refs/blobs/annotated_tag_to_blob",
 	"refs/heads/br2",
 	"refs/heads/cannot-fetch",
 	"refs/heads/chomped",
@@ -40,6 +41,7 @@ static const char *refnames[] = {
 };
 
 static const char *refnames_with_symlink[] = {
+	"refs/blobs/annotated_tag_to_blob",
 	"refs/heads/br2",
 	"refs/heads/cannot-fetch",
 	"refs/heads/chomped",
@@ -99,7 +101,7 @@ void test_refs_iterator__list(void)
 	git_vector output;
 	git_reference *ref;
 
-	cl_git_pass(git_vector_init(&output, 32, &refcmp_cb));
+	cl_git_pass(git_vector_init(&output, 33, &refcmp_cb));
 	cl_git_pass(git_reference_iterator_new(&iter, repo));
 
 	while (1) {
@@ -143,7 +145,7 @@ static int refs_foreach_cb(git_reference *reference, void *payload)
 void test_refs_iterator__foreach(void)
 {
 	git_vector output;
-	cl_git_pass(git_vector_init(&output, 32, &refcmp_cb));
+	cl_git_pass(git_vector_init(&output, 33, &refcmp_cb));
 	cl_git_pass(git_reference_foreach(repo, refs_foreach_cb, &output));
 	assert_all_refnames_match(refnames, &output);
 }

tests/resources/testrepo.git/refs/blobs/annotated_tag_to_blob

+521d87c1ec3aef9824daf6d96cc0ae3710766d91