On Tue, Jun 13, 2023 at 09:03:53PM +0200, Omar Polo wrote:
> Diff below is an attempt at simplifying this function.  The idea is to
> bump `limit' instead of doing complex logics inside the loop.
> 
> There's a small hack: limit can be 1 also if srv->max_commits_display
> is one, so we have to handle that condition not to break pagination in
> those cases.
> 
> For all I care, srv->max_commits_display=1 should be illegal and I'll
> send a diff to disallow it.  (may need to do a sweep to see if there
> are other places that would be simplified.)

Sure, I don't see why not.

srv->max_commits_display is a size_t (unsigned), and limit is an int (signed).
Should we change both of these to 'unsigned int'?
The type used in parse.y is a long long and we assign this to
srv->max_commits_display only if the value is greater 0 (or greater than 1
with your future tweak).
And it would be good if invalid max_commits_display caused a parsing error.

> full disclaimer: I hadn't had the time to properly test the diff, only
> checked that it loops only once for diffs and pagination of briefs
> works.

It reads good and fixes the issue. Please commit with my OK, we can
add more tweaks on top later.

> @@ -326,8 +326,17 @@ 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;
> +	int chk_next = 0;
>  

It seems a limit of zero should be disallowed on entry to this function.
We could return GOT_ERR_RANGE in that case.

> +	if (limit != 1 || srv->max_commits_display == 1) {

Use if (limit > 1 ...) for clarity?