Partial fix for libgit2#6532: insert-by-date order.

This commit fixes the following issues:

1. In `git_revwalk__push_commit`, if `opts->insert_by_date` is true, `git_commit_list_insert_by_date` is called. However, by this point the commit wasn’t parsed yet, so the `time` field still has 0 as value. Solved by parsing the commit immediately if `opts->insert_by_date` is true.

2. In the same function, there was an error in the boolean logic. When `opts->insert_by_date` was true, the commit would be inserted twice, first “by date” (not really, due to the issue described above) and then in the first position again. Logic was corrected.

3. In `prepare_walk`, when processing `user_input` and building the `commits` list, the order was being inverted. Assuming both fixes above, this would mean we would start negotiation by the oldest reference, not the newest one. This was fixed by producing a `commits` list in the same order as `user_input`.

The output list for the list of “have” statements during the negotiation is still not the same as core git’s because there is an optimization missing (excluding ancestors of commits known to be common between the client and the server) but this commit brings it much closer to core git’s. Specifically, the example on the libgit2#6532 issue description now fetches exactly the same objects than core git, and other examples I tested as well.

Author: arroz

src/libgit2/commit_list.c

@@ -43,13 +43,18 @@ int git_commit_list_time_cmp(const void *a, const void *b)
 	return 0;
 }
 
-git_commit_list *git_commit_list_insert(git_commit_list_node *item, git_commit_list **list_p)
-{
+git_commit_list *git_commit_list_create(git_commit_list_node *item, git_commit_list *next) {
 	git_commit_list *new_list = git__malloc(sizeof(git_commit_list));
 	if (new_list != NULL) {
 		new_list->item = item;
-		new_list->next = *list_p;
+		new_list->next = next;
 	}
+	return new_list;
+}
+
+git_commit_list *git_commit_list_insert(git_commit_list_node *item, git_commit_list **list_p)
+{
+	git_commit_list *new_list = git_commit_list_create(item, *list_p);
 	*list_p = new_list;
 	return new_list;
 }

src/libgit2/commit_list.h

@@ -49,6 +49,7 @@ git_commit_list_node *git_commit_list_alloc_node(git_revwalk *walk);
 int git_commit_list_generation_cmp(const void *a, const void *b);
 int git_commit_list_time_cmp(const void *a, const void *b);
 void git_commit_list_free(git_commit_list **list_p);
+git_commit_list *git_commit_list_create(git_commit_list_node *item, git_commit_list *next);
 git_commit_list *git_commit_list_insert(git_commit_list_node *item, git_commit_list **list_p);
 git_commit_list *git_commit_list_insert_by_date(git_commit_list_node *item, git_commit_list **list_p);
 int git_commit_list_parse(git_revwalk *walk, git_commit_list_node *commit);

src/libgit2/revwalk.c

@@ -83,8 +83,13 @@ int git_revwalk__push_commit(git_revwalk *walk, const git_oid *oid, const git_re
 
 	commit->uninteresting = opts->uninteresting;
 	list = walk->user_input;
-	if ((opts->insert_by_date &&
-	    git_commit_list_insert_by_date(commit, &list) == NULL) ||
+
+	/* To insert by date, we need to parse so we know the date. */
+	if (opts->insert_by_date && ((error = git_commit_list_parse(walk, commit)) < 0))
+		return error;
+
+	if ((opts->insert_by_date == 0 ||
+	    git_commit_list_insert_by_date(commit, &list) == NULL) &&
 	    git_commit_list_insert(commit, &list) == NULL) {
 		git_error_set_oom();
 		return -1;
@@ -609,7 +614,7 @@ static int sort_in_topological_order(git_commit_list **out, git_revwalk *walk, g
 static int prepare_walk(git_revwalk *walk)
 {
 	int error = 0;
-	git_commit_list *list, *commits = NULL;
+	git_commit_list *list, *commits = NULL, *commits_last = NULL;
 	git_commit_list_node *next;
 
 	/* If there were no pushes, we know that the walk is already over */
@@ -618,6 +623,12 @@ static int prepare_walk(git_revwalk *walk)
 		return GIT_ITEROVER;
 	}
 
+	/* 
+	 * This is a bit convoluted, but necessary to maintain the order of
+	 * the commits. This is especially important in situations where
+	 * git_revwalk__push_glob is called with a git_revwalk__push_options
+	 * setting insert_by_date = 1, which is critical for fetch negotiation.
+	 */
 	for (list = walk->user_input; list; list = list->next) {
 		git_commit_list_node *commit = list->item;
 		if ((error = git_commit_list_parse(walk, commit)) < 0)
@@ -627,8 +638,19 @@ static int prepare_walk(git_revwalk *walk)
 			mark_parents_uninteresting(commit);
 
 		if (!commit->seen) {
+			git_commit_list *new_list = NULL;
+			if ((new_list = git_commit_list_create(commit, NULL)) == NULL) {
+				git_error_set_oom();
+				return -1;
+			}
+			
 			commit->seen = 1;
-			git_commit_list_insert(commit, &commits);
+			if (commits_last == NULL)
+				commits = new_list;
+			else
+				commits_last->next = new_list;
+			
+			commits_last = new_list;
 		}
 	}