From: Stefan Sperling Subject: Re: gotwebd got_get_repo_age simplification To: Mark Jamsek Cc: gameoftrees@openbsd.org Date: Thu, 21 Nov 2024 11:14:04 +0100 On Thu, Nov 21, 2024 at 08:56:50PM +1100, Mark Jamsek wrote: > Stefan Sperling wrote: > > Instead of sorting refs by name and then hunting for the youngest > > commits in that list, obtain an appropriately sorted list via > > got_ref_cmp_by_commit_timestamp_descending and then pick the first > > ref from this list. > > > > And in the case were we already know which ref to check, just open > > this ref directly instead of listing all the refs. > > > > Easier to read and should improve performance in both cases. > > > > This diff is hard to read. I'd recommend applying it and reviewing > > the result. > > > > ok? > > Reads much nicer! But this is giving me different results for a fossil > repo converted to git that I moved into my /got/public repos a couple > days ago for testing the test harness. I'm not sure if replacing > got_object_commit_get_committer_time() with got_ref_get_mtime() is right > as they can produce different results. > > Before this change, various branches are correctly listed as X years, > Y months, or Z days ago, but with the diff they're all showing as 2 days > ago, which would've been when I copied the repo into /got/public. Right, the reference's mtime is an on-disk timestamp of the ref file, which does not correspond to the commit object's timestamp. Fixed version below. diff /home/stsp/src/got commit - ece731b025b35fd112ea1faebfabb163b61aacbe path + /home/stsp/src/got blob - f4a93f9f22c2a2ae93a2bf822794bade8af45b6c file + gotwebd/got_operations.c --- gotwebd/got_operations.c +++ gotwebd/got_operations.c @@ -116,59 +116,82 @@ got_get_repo_owner(char **owner, struct request *c) return NULL; } +static const struct got_error * +get_ref_commit_timestamp(time_t *timestamp, + struct got_reference *ref, struct got_repository *repo) +{ + const struct got_error *error; + struct got_object_id *id = NULL; + int obj_type; + struct got_commit_object *commit = NULL; + + /* We might have a cached timestamp available. */ + *timestamp = got_ref_get_cached_committer_time(ref); + if (*timestamp != 0) + return NULL; + + error = got_ref_resolve(&id, repo, ref); + if (error) + return error; + + error = got_object_get_type(&obj_type, repo, id); + if (error || obj_type != GOT_OBJ_TYPE_COMMIT) + goto done; + + error = got_object_open_as_commit(&commit, repo, id); + if (error) + goto done; + + *timestamp = got_object_commit_get_committer_time(commit); +done: + free(id); + if (commit) + got_object_commit_close(commit); + + return error; +} + +/* + * Find the youngest branch tip in the repository, or the age of + * a specific branch tip if a name was provided by the caller. + */ const struct got_error * got_get_repo_age(time_t *repo_age, struct request *c, const char *refname) { const struct got_error *error = NULL; struct transport *t = c->t; struct got_repository *repo = t->repo; - struct got_commit_object *commit = NULL; - struct got_reflist_head refs; - struct got_reflist_entry *re; - time_t committer_time = 0, cmp_time = 0; - TAILQ_INIT(&refs); - *repo_age = 0; - error = got_ref_list(&refs, repo, "refs/heads", - got_ref_cmp_by_name, NULL); - if (error) - goto done; + if (refname) { + struct got_reference *ref; + + error = got_ref_open(&ref, repo, refname, 0); + if (error) + return error; - /* - * Find the youngest branch tip in the repository, or the age of - * the a specific branch tip if a name was provided by the caller. - */ - TAILQ_FOREACH(re, &refs, entry) { - struct got_object_id *id = NULL; + error = get_ref_commit_timestamp(repo_age, ref, repo); + got_ref_close(ref); + } else { + struct got_reflist_head refs; + struct got_reflist_entry *re; - if (refname && strcmp(got_ref_get_name(re->ref), refname) != 0) - continue; + TAILQ_INIT(&refs); - error = got_ref_resolve(&id, repo, re->ref); + error = got_ref_list(&refs, repo, "refs/heads", + got_ref_cmp_by_commit_timestamp_descending, repo); if (error) - goto done; + return error; - error = got_object_open_as_commit(&commit, repo, id); - free(id); - if (error) - goto done; - - committer_time = - got_object_commit_get_committer_time(commit); - got_object_commit_close(commit); - if (cmp_time < committer_time) - cmp_time = committer_time; - - if (refname) - break; + re = TAILQ_FIRST(&refs); + if (re) { + error = get_ref_commit_timestamp(repo_age, + re->ref, repo); + } + got_ref_list_free(&refs); } - if (cmp_time != 0) - *repo_age = cmp_time; - done: - got_ref_list_free(&refs); return error; } blob - 0e987a302c6c21819424c0bbf8c957b06744abd8 file + include/got_reference.h --- include/got_reference.h +++ include/got_reference.h @@ -66,6 +66,17 @@ const char *got_ref_get_symref_target(struct got_refer time_t got_ref_get_mtime(struct got_reference *); /* + * Get the modification timestamp of the commit pointed at by the + * given reference. Will return zero if the commit timestamp is + * unknown or if the reference does not point at a commit. + * + * The cached timestamp will be known after got_ref_list() was called + * with either got_ref_cmp_by_commit_timestamp_descending() or + * with got_ref_cmp_tags(). + */ +time_t got_ref_get_cached_committer_time(struct got_reference *); + +/* * Create a duplicate copy of a reference. * The caller must dispose of this copy with got_ref_close(). */ blob - 3b728f8e2874054c0698accbbe24e51d69d93929 file + lib/reference.c --- lib/reference.c +++ lib/reference.c @@ -644,6 +644,12 @@ got_ref_get_mtime(struct got_reference *ref) return ref->mtime; } +time_t +got_ref_get_cached_committer_time(struct got_reference *ref) +{ + return ref->committer_time; +} + const struct got_error * got_ref_cmp_by_name(void *arg, int *cmp, struct got_reference *re1, struct got_reference* re2)