From: Stefan Sperling <stsp@stsp.name>
Subject: Re: gotd protected references
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Wed, 5 Apr 2023 08:27:35 +0200

On Tue, Apr 04, 2023 at 09:12:39PM +0200, Omar Polo wrote:
> On 2023/04/04 20:17:24 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> > On Tue, Apr 04, 2023 at 07:56:29PM +0200, Omar Polo wrote:
> > > On 2023/04/04 13:31:07 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> > > > +	error = got_pathlist_insert(NULL, &repo->protected_branches,
> > > > +	    refname, NULL);
> > > > +	if (error) {
> > > > +		yyerror("got_pathlist_insert: %s", error->msg);
> > > 
> > > nit: we could free(refname) here.
> > 
> > No. got_pathlist_insert() does not copy the string. It adds the
> > naked pointer to the list.  This is by design because some callers
> > add const char * variables which cannot be freed because they point
> > to strings that are were not allocated with malloc.
> 
> Yeah, but my point was that in the error path the insertion was not
> done and the memory leaked.  We're also not handling the case of a
> duplicate entry.  Since it's easier I'm considering it a syntax error
> too.

Ah, indeed. Sorry, I misunderstood your initial remark.
Ok stsp for your diff.

> If we agree to disallow duplicates it could be interesting to catch
> redundant namespaces too, like
> 
> 	protect branch namespace foo/bar/baz/
> 	protect branch namespace foo/

Not sure that is worth it, though it would be easy to do.
I would just silently emit redundant entries, so in the above
case only foo/ would remain.

We should detect cases where branches and tags overlap and error out:

 	protect branch namespace foo/
 	protect tag namespace foo/

> diff /home/op/w/gotacl
> commit - 9afa3de221045d529287cc3fa75fdc2915aed5c1
> path + /home/op/w/gotacl
> blob - 3b6daa1a2a93121b0ee884ddd8cd1ceb5e85fa9b
> file + gotd/parse.y
> --- gotd/parse.y
> +++ gotd/parse.y
> @@ -918,6 +918,7 @@ conf_protect_ref_namespace(struct got_pathlist_head *r
>  conf_protect_ref_namespace(struct got_pathlist_head *refs, char *namespace)
>  {
>  	const struct got_error *error;
> +	struct got_pathlist_entry *new;
>  	char *s;
>  
>  	got_path_strip_trailing_slashes(namespace);
> @@ -928,9 +929,13 @@ conf_protect_ref_namespace(struct got_pathlist_head *r
>  		return -1;
>  	}
>  
> -	error = got_pathlist_insert(NULL, refs, s, NULL);
> -	if (error) {
> -		yyerror("got_pathlist_insert: %s", error->msg);
> +	error = got_pathlist_insert(&new, refs, s, NULL);
> +	if (error || new == NULL) {
> +		free(s);
> +		if (error)
> +			yyerror("got_pathlist_insert: %s", error->msg);
> +		else
> +			yyerror("duplicate protect namespace %s", namespace);
>  		return -1;
>  	}
>  
> @@ -955,6 +960,7 @@ conf_protect_branch(struct gotd_repo *repo, char *bran
>  conf_protect_branch(struct gotd_repo *repo, char *branchname)
>  {
>  	const struct got_error *error;
> +	struct got_pathlist_entry *new;
>  	char *refname;
>  
>  	if (strncmp(branchname, "refs/heads/", 11) != 0) {
> @@ -975,10 +981,14 @@ conf_protect_branch(struct gotd_repo *repo, char *bran
>  		return -1;
>  	}
>  
> -	error = got_pathlist_insert(NULL, &repo->protected_branches,
> +	error = got_pathlist_insert(&new, &repo->protected_branches,
>  	    refname, NULL);
> -	if (error) {
> -		yyerror("got_pathlist_insert: %s", error->msg);
> +	if (error || new == NULL) {
> +		free(refname);
> +		if (error)
> +			yyerror("got_pathlist_insert: %s", error->msg);
> +		else
> +			yyerror("duplicate protect branch %s", branchname);
>  		return -1;
>  	}
>  
> 
>