From: Omar Polo <op@omarpolo.com>
Subject: gotwebd: avoid full history traversal in briefs/commits
To: gameoftrees@openbsd.org
Date: Sun, 22 Jan 2023 10:23:52 +0100

Hello,

as discussed some time ago on IRC, diff belows drop the history
traversal got_get_repo_commits does when jumping to a commit.  In
simpler terms, this purposefully breaks the 'previous' button in the
commits and briefs page.

It's a bit hard on git to find the parent of a commit since they can
only be iterated forward.  The way the previous button was generated
was by walking the history from the HEAD down to the specified commit.
This is both costly but more importantly leads to issue when dealing
with paths that were deleted from the repository.


diffstat /home/op/w/got
 M  gotwebd/got_operations.c  |  13+  81-

1 file changed, 13 insertions(+), 81 deletions(-)

diff /home/op/w/got
commit - fd61193437fe9adcf481b1c0bef72878cb6058e6
path + /home/op/w/got
blob - cb7924b39f5629118d7515bb7ae7bce48d66c550
file + gotwebd/got_operations.c
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -327,8 +327,7 @@ got_get_repo_commits(struct request *c, int limit)
 	struct querystring *qs = t->qs;
 	struct repo_dir *repo_dir = t->repo_dir;
 	char *in_repo_path = NULL, *repo_path = NULL, *file_path = NULL;
-	int chk_next = 0, chk_multi = 0, commit_found = 0;
-	int obj_type, limit_chk = 0;
+	int chk_next = 0, chk_multi = 0;
 
 	TAILQ_INIT(&refs);
 
@@ -343,60 +342,19 @@ got_get_repo_commits(struct request *c, int limit)
 		goto done;
 	}
 
-	/*
-	 * XXX: jumping directly to a commit id via
-	 * got_repo_match_object_id_prefix significantly improves performance,
-	 * but does not allow us to create a PREVIOUS button, since commits can
-	 * only be itereated forward. So, we have to match as we iterate from
-	 * the headref.
-	 */
-	if (qs->action == BRIEFS || qs->action == COMMITS ||
-	    (qs->action == TREE && qs->commit == NULL)) {
-		error = got_ref_open(&ref, repo, qs->headref, 0);
-		if (error)
-			goto done;
-
-		error = got_ref_resolve(&id, repo, ref);
-		if (error)
-			goto done;
-	} else if (qs->commit != NULL) {
-		error = got_ref_open(&ref, repo, qs->commit, 0);
-		if (error == NULL) {
-			error = got_ref_resolve(&id, repo, ref);
-			if (error)
-				goto done;
-			error = got_object_get_type(&obj_type, repo, id);
-			if (error)
-				goto done;
-			if (obj_type == GOT_OBJ_TYPE_TAG) {
-				struct got_tag_object *tag;
-				error = got_object_open_as_tag(&tag, repo, id);
-				if (error)
-					goto done;
-				if (got_object_tag_get_object_type(tag) !=
-				    GOT_OBJ_TYPE_COMMIT) {
-					got_object_tag_close(tag);
-					error = got_error(GOT_ERR_OBJ_TYPE);
-					goto done;
-				}
-				free(id);
-				id = got_object_id_dup(
-				    got_object_tag_get_object_id(tag));
-				if (id == NULL)
-					error = got_error_from_errno(
-					    "got_object_id_dup");
-				got_object_tag_close(tag);
-				if (error)
-					goto done;
-			} else if (obj_type != GOT_OBJ_TYPE_COMMIT) {
-				error = got_error(GOT_ERR_OBJ_TYPE);
-				goto done;
-			}
-		}
+	if (qs->commit) {
 		error = got_repo_match_object_id_prefix(&id, qs->commit,
 		    GOT_OBJ_TYPE_COMMIT, repo);
 		if (error)
 			goto done;
+	} else {
+		error = got_ref_open(&ref, repo, qs->headref, 0);
+		if (error)
+			goto done;
+
+		error = got_ref_resolve(&id, repo, ref);
+		if (error)
+			goto done;
 	}
 
 	error = got_repo_map_path(&in_repo_path, repo, repo_path);
@@ -447,35 +405,10 @@ got_get_repo_commits(struct request *c, int limit)
 			goto done;
 		}
 
-		if (limit_chk == ((limit * qs->page) - limit) &&
-		    commit_found == 0 && repo_commit->commit_id != NULL) {
-			t->prev_id = strdup(repo_commit->commit_id);
-			if (t->prev_id == NULL) {
-				error = got_error_from_errno("strdup");
-				gotweb_free_repo_commit(repo_commit);
-				goto done;
-			}
-		}
-
-		if (qs->commit != NULL && commit_found == 0 && limit != 1) {
-			if (strcmp(qs->commit, repo_commit->commit_id) == 0)
-				commit_found = 1;
-			else if (qs->file != NULL && strlen(qs->file) > 0 &&
-			    qs->page == 0)
-				commit_found = 1;
-			else {
-				gotweb_free_repo_commit(repo_commit);
-				limit_chk++;
-				continue;
-			}
-		}
-
 		TAILQ_INSERT_TAIL(&t->repo_commits, repo_commit, entry);
 
-		if (limit == 1 && chk_multi == 0 &&
-		    srv->max_commits_display != 1)
-			commit_found = 1;
-		else {
+		if (!chk_multi || limit != 1 ||
+		    srv->max_commits_display == 1) {
 			chk_multi = 1;
 
 			/*
@@ -501,8 +434,7 @@ got_get_repo_commits(struct request *c, int limit)
 			}
 		}
 		if (error || (limit && --limit == 0)) {
-			if (commit_found || (qs->file != NULL &&
-			    strlen(qs->file) > 0))
+			if (qs->file != NULL && *qs->file != '\0')
 				if (chk_multi == 0)
 					break;
 			chk_next = 1;