Download raw body.
gotwebd: avoid full history traversal in briefs/commits
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
gotwebd: avoid full history traversal in briefs/commits