On Sat, 2023-11-04 at 11:33 +0100, Stefan Sperling wrote:
> On Fri, Nov 03, 2023 at 09:19:23PM +0100, Martijn van Duren wrote:
> > Hello,
> > 
> > I just started playing with got{,d} for the first time and after
> > wrapping my head around the fact that the repository needs to live
> > outside the working tree I'm starting to like it.
> > 
> > In setting up a gotd server, I quickly became bored with the copy-paste
> > work of the different per repository settings.
> > 
> > Diff below allows to declare repository directives in the global scope
> > and are stored in a repo_template. If a directive is not specified in
> > the repository it will be copied over from the template. The path
> > directive will be used as a root-directory and the name of the
> > repository will be appended to complete the per repository path.
> > 
> > thoughts?
> 
> Hey Martijn,
> 
> I understand the need to avoid repetition. This requirement will probably
> come up over and over again so we should definitely do something about this.
> 
> I have doubts about appending the repository name to an on-disk path because
> the repository name is in a "virtual" path space. It is not an on-disk path.
> Consider this example from gotd.conf:
> 
>            # This repository can be accessed via
>            # ssh://user@example.com/openbsd/ports
>            repository "openbsd/ports" {
>                    path "/var/git/ports.git"

I thought about this, but it's new functionality, so we won't break
anything here. If people want to use this new feature on their existing
gotd.conf they can either keep the path directive and remove the rest,
or move the above example to /var/git/openbsd/ports.git.
> 
> The repository name is used to construct the repository URL which clients can
> fetch from. This URL has no relation to where the repository appears on disk.
> I would prefer to keep the URL and on-disk path spaces completely separate.
> 
> While at it, we should support multiple repository directories because
> people might want to add more partitions when existing ones start running
> out of space.
> 
> So how about implementing something like this instead:
> 
>  directory "/git"  # some repositories are here
>  directory "/var/git"  # some more repositories are here

Well, my original thinking was to make it possible to completely
skip the repository keyword and let gotd scan the entire template
path. I'm not sure how far your suggestion is feasible with the
current pledge/unveil structure, but my original idea was more
trouble then it's worth for me.

Then comes the second problem: what if by some oversight a
repository-specific path is specified twice, under different repo
names?

Another option I was thinking about, which builds upon my diff is
that we could add a template keyword, so we would have something
like this:
repository template "martijn" {
	path "/var/got/martijn"
	permit rw martijn
	...
}
repository template "stsp" {
	path "/var/got/stsp"
	permit rw stsp
	...
}
repository got {
	template stsp
}
repository snmpd {
	template martijn
}

I didn't implement this in the diff, because it's big enough as it
is and I don't see an immediate need for it on top of this
functionality, but the code was written with this option in mind.
> 
>  repository "openbsd/ports" {
>    # a relative path is looked up in all directories, first match wins:
>    path "ports.git"
> 
>    ...
>  }
> 
> This scheme is backwards compatible with existing configs because so far
> gotd.conf has required absolute paths to repositories.
> 
> Regarding the new code, could the new protect_set flag be replaced with
> TAILQ_EMPTY checks, for example via a helper function like this:
> 
> static int
> repo_has_protected_refs(struct gotd_repo *repo)
> {
>   return (TAILQ_EMPTY(&repo->protected_tag_namespaces) &&
>       TAILQ_EMPTY(&repo->protected_branch_namespaces) &&
>       TAILQ_EMPTY(&repo->protected_branches));
> }
> 
> I suspect this might reduce the size of the diff and would make the code
> easier to follow because there would be less state flags to keep in mind.

This doesn't work. Consider the following example:
protect {
	branch release
}
repository garbage {
	protect {}
}
garbage would pass repo_has_protected_refs(), but the intent here is to
remove all protections from the global scope.
> 
> Regarding the docs, could you extend the EXAMPLES section in the man page
> to cover the new possibilities? I suspect many people jump there and begin
> copy-pasting, and only read the rest of the page if they are unsure about
> something.

Sure, but first let's settle on the semantics.
> 
> Another problem I noticed while looking at this is that variables don't work
> in gotd.conf because symset() is unreachable. I have a diff to make this work
> by copying relevant code from gotwebd/parse.y. I had hoped this could help
> with avoiding repetition but it won't because variables will not be expanded
> inside strings. A directive such as path "$gitrootdir/repo.git" will not
> expand $gitrootdir.

Luckily marcos also aren't a documented feature in gotd.conf(5). :-)

> Still, variables could be another solution to avoiding repetition.
> We could try modifying parse.y to expand variables in strings somehow, but I
> don't know how difficult that would be. And maybe there is a good reason why
> variables don't get expanded in strings?

I never really looked too far into parse.y macros. But macro
expanding is part of yylex() so at the very least the effort would
need to be duplicated/moved. After that there's always the risk that
some sole finds a reason to add a literal '$' in their string.
After wrapping my head around your statement twice, I can see how
this reduces the repetition, but putting an entire config section in
a macro just feels wrong to me. But if it works... *shrugs*
> 
> Thanks,
> Stefan