On Sun, Aug 20, 2023 at 09:32:45PM +0200, Omar Polo wrote:
> > +#define _GNU_SOURCE
> > +
> >  #include <sys/types.h>
> >  
> >  #include <errno.h>
> > +#include <argp.h>
> >  
> > +#include "got_compat.h"
> > +
> 
> I'd avoid explicitly defining _GNU_SOURCE and instead move the include
> of got_compat.h so it's the first header.  we're already using
> AC_USE_SYSTEM_EXTENSIONS which defines _GNU_SOURCE at the top of
> got_compat.h on GNU systems, so we're already covered.

Sure, I will try including got_config.h instead.

> (We have many files that don't include got_compat.h and not all have
> got_compat.h at the very start, but this is a separate issue.)
> 
> also, is argp.h standard?  Don't see it on OpenBSD.  We should check
> its presence in configure.ac (add an entry to the AC_CHECK_HEADERS
> macro) and guard the include with #ifdef HAVE_ARGP_H or something
> along these lines.
> 
> The manpage of program_invocation_short_name(3) on GNU/Linux only
> lists errno.h as required header moreover.

No idea...
I am open to suggestions and further improvements. This was the only
way I found to get this working on Ubuntu.

> >  #ifndef GITWRAPPER_GIT_LIBEXEC_DIR
> > +#ifdef __linux__
> > +#define GITWRAPPER_GIT_LIBEXEC_DIR "/usr/lib/git-core/"
> > +#else
> >  #define GITWRAPPER_GIT_LIBEXEC_DIR "/usr/local/libexec/git"
> >  #endif
> > +#endif
> 
> not to block progress but these should also not be hardcoded because
> 
>  1. it's ugly to #ifdef __linux__; this seems to affect other systems
>     as well
> 
>  2. different systems/distro install different things in different
>     places.  (I won't be surprised if DragonflyBSD has git in /usr/bin
>     for example.)
> 
> so this could be fine for a first stab, but would then need some work
> to allow changing these paths at configure-time (at least.)

This is not meant as the final solution indeed. For now I was mostly
interested in getting a gotd setup up and running one way or another.

> One way would be to detect at configure time where the git libexecs
> are installed and provide a --with flag to override the automatic
> guessing game.

Yes, that would be better.
 
> > +#ifdef __linux__
> > +#define GOTD_EMPTY_PATH	"/var/run/gotd"
> > +#else
> >  #define GOTD_EMPTY_PATH	"/var/empty"
> > +#endif
> 
> same thing here, these should be changeable from the configure script
> IMHO.  It's fine for me if you want to land these as-is, I can add the
> autoconf bits later (assuming thomas agrees.)

I agree. But I would still commit this as-is and leave improvements for later.