From: Stefan Sperling <stsp@stsp.name>
Subject: Re: gotd protected references
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Tue, 4 Apr 2023 20:17:24 +0200

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:
> some comments inline.  Haven't tested yet, but it reads fine.  we can
> address these points in-tree, it's ok op@ as-is.

Thanks! I will commit this and we can add more teaks in-tree.

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

> > +static const struct got_error *
> > +protect_ref_namespace(const char *refname, const char *namespace)
> > +{
> > +	const struct got_error *err;
> > +
> > +	err = validate_namespace(namespace);
> > +	if (err)
> > +		return err;
> 
> If I'm reading this right, in this function and in the others variant
> below, `namespace' is the namespace provided in the configuration
> file.  if so, I guess we could drop these calls since the one done at
> configuration parsing time should be enough.  Or do you want to keep
> these extra checks around for safety for a while?

I added this early on, before I had the config file bits written,
to avoid problems due to a missing trailing slash.

The trailing slash must be present otherwise it is unclear where
the namespace boundary is  Consider refs/heads/foo/ vs refs/heads/foobar/
showing up here without a trailing slash.

These checks are cheap and important for correctness. It does not hurt
to do this sanity check at run-time as well as when reading the config.

> > +	if (strncmp(namespace, refname, strlen(namespace)) == 0)
> >  		return got_error_fmt(GOT_ERR_REFS_PROTECTED, "%s", namespace);
> >  
> >  	return NULL;
> >  }
> > [...]
> > +static const struct got_error *
> > +protect_require_yca(struct got_object_id *tip_id,
> > +    size_t max_commits_to_traverse, struct got_pack *pack,
> > +    struct got_packidx *packidx, struct got_reference *ref)
> > +{
> > [...]
> > +		parent_ids = got_object_commit_get_parent_ids(commit);
> > +		STAILQ_FOREACH(pid, parent_ids, entry) {
> > +			err = check_cancelled(NULL);
> > +			if (err)
> > +				goto done;
> 
> just for curiosity, this loop doesn't seem heavy, so why
> check_cancelled() here?  (asking to figure out if I got
> check_cancelled() usage correct in my head.)

Just in case someone sends a commit with an insane list of parents.