From: Mark Jamsek <mark@jamsek.com>
Subject: Re: gotwebd: avoid full history traversal in briefs/commits
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Sun, 22 Jan 2023 21:21:32 +1100

On 23-01-22 10:23AM, Omar Polo wrote:
> 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(-)

I remember this discussion and I agree it's a good move. The diff looks
good and apart from the absent 'previous' link everything seems to be
working as usual. Thanks!

ok

> 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;
> 

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68