From: Klemens Nanni Subject: Re: Gotweb To: Tracey Emery Cc: gameoftrees@openbsd.org Date: Wed, 15 Jan 2020 17:59:04 +0100 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 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 >