On 2023/06/14 10:01:40 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> 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).

Right.  in diff below, I'm changing the limit to be a size_t too.
it's only called with 1, D_MAXSLCOMMDISP or srv->max_commits_display
so it should be fine.

in parse.y we assign it from a `long long' (signed) which should fit
since we reject negative value.

> And it would be good if invalid max_commits_display caused a parsing error.

done

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

I've committed the diff as-is then :)

> > @@ -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.

sure

> > +	if (limit != 1 || srv->max_commits_display == 1) {
> 
> Use if (limit > 1 ...) for clarity?

also done.  I did these last two in this diff and not in the previous
commit since it wouldn't make sense if `limit' is an int.

if ok i'll try to apply the same treatment to the tags case.

diff /home/op/w/got
commit - 5144d22b0c3bcf6611cc36e93a3a859fcc521277
path + /home/op/w/got
blob - aba5a591662378d9b7363a6435d153408a11fb87
file + gotwebd/got_operations.c
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -311,7 +311,7 @@ got_get_repo_commits(struct request *c, int limit)
 }
 
 const struct got_error *
-got_get_repo_commits(struct request *c, int limit)
+got_get_repo_commits(struct request *c, size_t limit)
 {
 	const struct got_error *error = NULL;
 	struct got_object_id *id = NULL;
@@ -328,7 +328,10 @@ got_get_repo_commits(struct request *c, int limit)
 	char *in_repo_path = NULL, *repo_path = NULL, *file_path = NULL;
 	int chk_next = 0;
 
-	if (limit != 1 || srv->max_commits_display == 1) {
+	if (limit == 0)
+		return got_error(GOT_ERR_RANGE);
+
+	if (limit > 1) {
 		/*
 		 * Traverse one commit more than requested to provide
 		 * the next button.
blob - fae082679ec712db36cb01d56e5edb5857535ab7
file + gotwebd/gotwebd.h
--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ -509,7 +509,7 @@ const struct got_error *got_get_repo_commits(struct re
 const struct got_error *got_get_repo_owner(char **, struct request *);
 const struct got_error *got_get_repo_age(time_t *, struct request *,
     const char *);
-const struct got_error *got_get_repo_commits(struct request *, int);
+const struct got_error *got_get_repo_commits(struct request *, size_t);
 const struct got_error *got_get_repo_tags(struct request *, int);
 const struct got_error *got_get_repo_heads(struct request *);
 const struct got_error *got_open_diff_for_output(FILE **, struct request *);
blob - bd616e146ccf4605ac53053dafa9b17cc39b8b86
file + gotwebd/parse.y
--- gotwebd/parse.y
+++ gotwebd/parse.y
@@ -386,8 +386,12 @@ serveropts1	: REPOS_PATH STRING {
 				new_srv->max_repos_display = $2;
 		}
 		| MAX_COMMITS_DISPLAY NUMBER {
-			if ($2 > 0)
-				new_srv->max_commits_display = $2;
+			if ($2 <= 1) {
+				yyerror("invalid max_commits_display %lld",
+				    $2);
+				YYERROR;
+			}
+			new_srv->max_commits_display = $2;
 		}
 		;