On Wed, Jan 15, 2020 at 07:56:18AM -0700, Tracey Emery wrote:
> After several months, here is the first diff to introduce gotweb to the
> got tree. To see a working example of the code, go to got.traceyemery.net.
Great!

> Gotweb currently has the same functionality as tog: log, diff, tree, and
> blame, and is still currently under development. As it stands, I think
> it's ready to start the code audit and review, so I can integrate it to
> the tree.
I agree.

Comments inline,  I'm happy whether you address them before or after
importing gotweb.

> diff refs/heads/master refs/heads/gotweb
> blob - 7aaea69b30edf716c5f17759fd96059ac9dd2286
> blob + ed241a51a70f1885bc59f89cb7f41f4ce53b66bb
> --- Makefile
> +++ Makefile
> @@ -3,7 +3,7 @@ SUBDIR = libexec got tog
Why do you not add gotweg to this line?

>  .PHONY: release dist
>  
>  .if make(regress) || make(obj) || make(clean) || make(release)
> -SUBDIR += regress
> +SUBDIR += regress gotweb
>  .endif
>  
>  .include "got-version.mk"
> @@ -24,5 +24,15 @@ dist: clean
>  		| sort > got-dist.txt.new
>  	diff -u got-dist.txt got-dist.txt.new
>  	rm got-dist.txt.new
> +
> +web:
> +	sed -i -e "s/MAKEWEB=No/MAKEWEB=Yes/" got-version.mk
> +	${MAKE} -C gotweb
> +	sed -i -e "s/MAKEWEB=Yes/MAKEWEB=No/" got-version.mk
> +
> +web-install:
> +	sed -i -e "s/MAKEWEB=No/MAKEWEB=Yes/" got-version.mk
> +	${MAKE} -C gotweb install
> +	sed -i -e "s/MAKEWEB=Yes/MAKEWEB=No/" got-version.mk
Parameters always take precedence over variables in files, so this can
simply be

	${MAKE} -C gotweb MAKEWEB=Yes  


> blob - /dev/null
> blob + c865312166b95b41acd5cf9a60547d5c139e4969 (mode 644)
> --- /dev/null
> +++ gotweb/Makefile
> @@ -0,0 +1,52 @@
> +.PATH:${.CURDIR}/../lib
> +
> +SUBDIR = ../libexec
> +
> +.include "../got-version.mk"
> +
> +PROG =		gotweb
> +SRCS =		gotweb.c parse.y blame.c commit_graph.c delta.c diff.c \
> +		diffreg.c error.c fileindex.c object.c object_cache.c \
> +		object_idset.c object_parse.c opentemp.c path.c pack.c \
> +		privsep.c reference.c repository.c sha1.c worktree.c \
> +		inflate.c buf.c rcsutil.c diff3.c lockfile.c \
> +		deflate.c object_create.c delta_cache.c
> +MAN =		${PROG}.conf.5
> +
> +CPPFLAGS +=	-I${.CURDIR}/../include -I${.CURDIR}/../lib -I${PREFIX}/include
> +
> +LDADD +=	-L${PREFIX}/lib -static -lkcgihtml -lkcgi -lz -lutil
> +
> +.if ${GOT_RELEASE} != "Yes"
> +NOMAN = Yes
> +.endif
> +
> +realinstall:
> +	if [ ! -d ${CGI_DIR}/. ]; then \
> +		${INSTALL} -d -o root -g daemon -m 755 ${CGI_DIR}; \
...
> +	if [ ! -d ${PROG_DIR}/. ]; then \
> +		${INSTALL} -d -o root -g daemon -m 755 ${PROG_DIR}; \
> +	fi
> +	${INSTALL} -c -o www -g www -m 0755 files/htdocs/${PROG}/* ${PROG_DIR}
All those directory tests seem overly cautious... but also don't harm.
I presume this can be improved later in-tree.

> +.include <bsd.prog.mk>
This file (by including bsd.own.mk) provides BINOWN, DIRMODE, et al.
which should probably be used here as mnemonic variables in favour of
literal numerics.

> blob - /dev/null
> blob + c63ca834be73da69a2851808511a8bada49b3d10 (mode 644)
> --- /dev/null
> +++ gotweb/README
> @@ -0,0 +1,61 @@
> +Game of Trees Web (Gotweb) is a read-only web implementation of Got.
> +
> +Gotweb is still under development; it is being developed exclusively
> +on OpenBSD and its target audience are OpenBSD developers. Gotweb is
> +ISC-licensed and was designed with pledge(2) and unveil(2) in mind.
> +
> +Gotweb uses bare Git repositories to read versioned data and is designed to
> +work with httpd(8).
That part as well as the examplt httpd.conf(5) is perfectly fine,
however I doubt the usefulness of duplicating got's README with regard
to patch submission and bug reports.

> +Example configuration for httpd.conf:
> +
> +ext_if = "*"
> +
> +types { include "/usr/share/misc/mime.types" }
> +
> +server "localhost" {
> +	listen on $ext_if port 80
> +
> +	root "/htdocs/gotweb"
> +
> +	location "/cgi-bin/*" {
> +		root "/"
> +		fastcgi
> +	}
> +	location "/*" {
> +		directory index "index.html"
> +	}
> +}
I recognise this configuration ;-)


> blob - /dev/null
> blob + f066d2df1f07bdd931a62cbdb7fb00a3b9bd4989 (mode 644)
> --- /dev/null
> +++ gotweb/parse.y
...
> +main		: GOT_REPOS_PATH STRING {
> +      			if ((gw_conf->got_repos_path = strdup($2)) == NULL)
> +				errx(1, "out of memory");
> +      		}
> +		| GOT_WWW_PATH STRING {
> +      			if ((gw_conf->got_www_path = strdup($2)) == NULL)
> +				errx(1, "out of memory");
> +      		}
Above two closing `}' start with spaces not tabs and need indent fixing.

> +		| GOT_MAX_REPOS NUMBER {
> +			if ($2 > 0)
> +				gw_conf->got_max_repos = $2;
> +		}
> +		| GOT_SITE_NAME STRING {
> +      			if ((gw_conf->got_site_name = strdup($2)) == NULL)
> +				errx(1, "out of memory");
> +		}
> +		| GOT_SITE_OWNER STRING {
> +      			if ((gw_conf->got_site_owner = strdup($2)) == NULL)
> +				errx(1, "out of memory");
> +		}
> +		| GOT_SITE_LINK STRING {
> +      			if ((gw_conf->got_site_link = strdup($2)) == NULL)
> +				errx(1, "out of memory");
> +		}
> +		| GOT_LOGO STRING {
> +      			if ((gw_conf->got_logo = strdup($2)) == NULL)
> +				errx(1, "out of memory");
> +		}
> +		| GOT_LOGO_URL STRING {
> +      			if ((gw_conf->got_logo_url = strdup($2)) == NULL)
All `if ()' lines above start with spaces not tabs.

> +const struct got_error*
> +parse_conf(const char *filename, struct gotweb_conf *gconf)
> +{
> +	static const struct got_error*	 error = NULL;
> +
> +	gw_conf = gconf;
> +      	if ((gw_conf->got_repos_path = strdup(D_GOTPATH)) == NULL)
> +		errx(1, "out of memory");
Is "out of memory" the only way strdup can fail?  Why throw away errno
and use your own message when `err(1, NULL)' or `err(1, "strdup")'
provides all possible cases?  


> blob - da3924e2e27178c8c42cfa8411735cbb75eb814b
> blob + 40fa9cc99ceca8d3d1fe298c1a381c45073a6736
> --- libexec/Makefile.inc
> +++ libexec/Makefile.inc
> @@ -1,7 +1,15 @@
>  .include "../Makefile.inc"
>  
> +.if ${MAKEWEB} == "Yes"
>  realinstall:
> +	if [ ! -d ${LIBEXEC_DIR}/. ]; then \
> +		${INSTALL} -d -o root -g daemon -m 755 ${LIBEXEC_DIR}; \
> +	fi
> +	${INSTALL} ${INSTALL_COPY} -o root -g daemon -m 755 ${PROG} \
Same wrt. BINMODE as above;  below you already use it, so let's be
consistent.

> +	${LIBEXEC_DIR}/${PROG}
> +.else
> +realinstall:
>  	${INSTALL} ${INSTALL_COPY} -o ${BINOWN} -g ${BINGRP} \
>  	-m ${BINMODE} ${PROG} ${LIBEXECDIR}/${PROG}
> -
> +.endif
>  NOMAN = Yes
>