From 4edf6c114118232da86d4de1543222b12ca960ae Mon Sep 17 00:00:00 2001 From: Miguel Arroz <750683+arroz@users.noreply.github.com> Date: Mon, 27 Mar 2023 11:07:57 -0700 Subject: [PATCH] Partial fix for #6532: insert-by-date order. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #6532 issue description now fetches exactly the same objects than core git, and other examples I tested as well. --- src/libgit2/commit_list.c | 11 ++++++++--- src/libgit2/commit_list.h | 1 + src/libgit2/revwalk.c | 30 ++++++++++++++++++++++++++---- 3 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/libgit2/commit_list.c b/src/libgit2/commit_list.c index 12b329b251e..2e28d5caf8a 100644 --- a/src/libgit2/commit_list.c +++ b/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; } diff --git a/src/libgit2/commit_list.h b/src/libgit2/commit_list.h index aad39f3513b..e2dbd2aaed3 100644 --- a/src/libgit2/commit_list.h +++ b/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); diff --git a/src/libgit2/revwalk.c b/src/libgit2/revwalk.c index 3269d9279b6..4ea6fae8ffb 100644 --- a/src/libgit2/revwalk.c +++ b/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; } }