On Mon, Jan 29, 2024 at 02:52:04PM +0100, Omar Polo wrote:
> On 2024/01/29 14:20:43 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> > On Mon, Jan 29, 2024 at 01:52:30PM +0100, Omar Polo wrote:
> > > @@ -218,6 +223,10 @@ gotweb_process_request(struct request *c)
> > >  
> > >  	switch (qs->action) {
> > >  	case BLAME:
> > > +		if (qs->folder == NULL || qs->file == NULL) {
> > 
> > This change introduces a small inconsistency:
> > qs->folder is allowed to be NULL in got_output_file_blame() which now
> > becomes impossible to reach.
> 
> I realized it, but now we seem to always emit a folder parameter[1], maybe
> empty, so I was considering being more strict here.

Good!
 
> I think got_oput_file_blame() did it either to be pedantic against bad
> input or because before folder was not always set.

I am not sure. The most likely case is just careful coding. I suspect
this code was added before templating was introduced. I might even have
been the first line of input validation defence back then.

> I can remove the qs->folder check if you prefer.

Please remove it from the other place instead.

Overall, I would prefer to keep input validation checks in one place.
That makes it easier to spot problems where such checks are missing.

So when adding the qs->folder check at this higher level please remove it
from the lower level so that we converge towards eventual consistency.