Stefan Sperling <stsp@stsp.name> wrote:
> On Thu, Nov 21, 2024 at 08:56:50PM +1100, Mark Jamsek wrote:
> > Stefan Sperling <stsp@stsp.name> 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.

Great! Still reads much nicer and this indeed works for me, thanks :)

ok with or without the stray tabs (annotated inline) removed.

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

trailing tab here^

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

\t	\t
two tabs here

> +		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)


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