[PATCH 1/2] Makefile: re-include cgit.conf in cgit.mk

[PATCH 1/2] Makefile: re-include cgit.conf in cgit.mk

[john]

This avoids needed to export every variable that might be used in
cgit.mk from the top-level Makefile.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cgit.mk | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/cgit.mk b/cgit.mk
index e1aed63..bf3bbb1 100644
--- a/cgit.mk
+++ b/cgit.mk
@@ -5,6 +5,8 @@ include Makefile
 
 CGIT_PREFIX = ../
 
+-include $(CGIT_PREFIX)cgit.conf
+
 # The CGIT_* variables are inherited when this file is called from the
 # main Makefile - they are defined there.
 
-- 
1.8.2.540.gf023cfe

[PATCH 2/2] compat/timegm: new compat function for systems lacking timegm()

[john]

The implementation used here is taken from Samba, since discussions on
their mailing list indicate that the "portable version" in timegm(3)
does not actually work on Solaris.

This commit also adds the infrastructure required for compat functions
in CGit itself.  Mostly we get everything we need from Git, but timegm
is not used in Git so there is no reason for them to carry a
compatibility version.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
I wanted to split this into two patches, one to introduce "compat"
infrastructure and one to add timegm.c but I couldn't really see a clean
way of doing that and I don't think it's too bad as a single patch.

 cgit-compat-util.h | 11 ++++++++
 cgit.h             |  2 +-
 cgit.mk            | 14 ++++++++--
 compat/timegm.c    | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 102 insertions(+), 3 deletions(-)
 create mode 100644 cgit-compat-util.h
 create mode 100644 compat/timegm.c

diff --git a/cgit-compat-util.h b/cgit-compat-util.h
new file mode 100644
index 0000000..2573f32
--- /dev/null
+++ b/cgit-compat-util.h
@@ -0,0 +1,11 @@
+#ifndef CGIT_COMPAT_UTIL_H
+#define CGIT_COMPAT_UTIL_H
+
+#include "git/git-compat-util.h"
+
+#ifdef NO_TIMEGM
+#define timegm cgittimegm
+extern time_t cgittimegm(struct tm *tm);
+#endif
+
+#endif
diff --git a/cgit.h b/cgit.h
index ed5cf14..0a9f7f5 100644
--- a/cgit.h
+++ b/cgit.h
@@ -2,7 +2,7 @@
 #define CGIT_H
 
 
-#include <git-compat-util.h>
+#include "cgit-compat-util.h"
 #include <cache.h>
 #include <grep.h>
 #include <object.h>
diff --git a/cgit.mk b/cgit.mk
index bf3bbb1..b8c7b68 100644
--- a/cgit.mk
+++ b/cgit.mk
@@ -15,12 +15,19 @@ $(CGIT_PREFIX)VERSION: force-version
 -include $(CGIT_PREFIX)VERSION
 .PHONY: force-version
 
+ifdef NO_TIMEGM
+	CGIT_COMPAT_CFLAGS += -DNO_TIMEGM
+	CGIT_COMPAT_OBJ_NAMES += compat/timegm.o
+endif
+
 # CGIT_CFLAGS is a separate variable so that we can track it separately
 # and avoid rebuilding all of Git when these variables change.
 CGIT_CFLAGS += -DCGIT_CONFIG='"$(CGIT_CONFIG)"'
 CGIT_CFLAGS += -DCGIT_SCRIPT_NAME='"$(CGIT_SCRIPT_NAME)"'
 CGIT_CFLAGS += -DCGIT_CACHE_ROOT='"$(CACHE_ROOT)"'
 
+CGIT_CFLAGS += $(CGIT_COMPAT_CFLAGS)
+
 ifdef NO_C99_FORMAT
 	CFLAGS += -DNO_C99_FORMAT
 endif
@@ -52,7 +59,7 @@ CGIT_OBJ_NAMES += ui-tag.o
 CGIT_OBJ_NAMES += ui-tree.o
 CGIT_OBJ_NAMES += vector.o
 
-CGIT_OBJS := $(addprefix $(CGIT_PREFIX),$(CGIT_OBJ_NAMES))
+CGIT_OBJS := $(addprefix $(CGIT_PREFIX),$(CGIT_OBJ_NAMES) $(CGIT_COMPAT_OBJ_NAMES))
 
 # Only cgit.c reference CGIT_VERSION so we only rebuild its objects when the
 # version changes.
@@ -65,8 +72,11 @@ $(CGIT_VERSION_OBJS): EXTRA_CPPFLAGS = \
 ifeq ($(wildcard $(CGIT_PREFIX).depend),)
 missing_dep_dirs += $(CGIT_PREFIX).depend
 endif
+ifeq ($(wildcard $(CGIT_PREFIX)compat/.depend),)
+missing_dep_dirs += $(CGIT_PREFIX)compat/.depend
+endif
 
-$(CGIT_PREFIX).depend:
+$(addprefix $(CGIT_PREFIX),.depend compat/.depend):
 	@mkdir -p $@
 
 $(CGIT_PREFIX)CGIT-CFLAGS: FORCE
diff --git a/compat/timegm.c b/compat/timegm.c
new file mode 100644
index 0000000..1491538
--- /dev/null
+++ b/compat/timegm.c
@@ -0,0 +1,78 @@
+/*
+ * Copyright (c) 1997 Kungliga Tekniska H?gskolan
+ * (Royal Institute of Technology, Stockholm, Sweden).
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * 3. Neither the name of the Institute nor the names of its contributors
+ *    may be used to endorse or promote products derived from this software
+ *    without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE INSTITUTE AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL THE INSTITUTE OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+/*
+ * Adapted for Samba4 by Andrew Tridgell.
+ * Adapted for CGit by John Keeping.
+ */
+
+#include <time.h>
+
+static int is_leap(unsigned y)
+{
+	y += 1900;
+	return (y % 4) == 0 && ((y % 100) != 0 || (y % 400) == 0);
+}
+
+time_t cgittimegm(struct tm *tm)
+{
+	static const unsigned ndays[2][12] ={
+		{31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31},
+		{31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31}};
+	time_t res = 0;
+	unsigned i;
+
+	if (tm->tm_mon > 12 ||
+	    tm->tm_mon < 0 ||
+	    tm->tm_mday > 31 ||
+	    tm->tm_min > 60 ||
+	    tm->tm_sec > 60 ||
+	    tm->tm_hour > 24) {
+		/* invalid tm structure */
+		return 0;
+	}
+
+	for (i = 70; i < tm->tm_year; ++i)
+		res += is_leap(i) ? 366 : 365;
+
+	for (i = 0; i < tm->tm_mon; ++i)
+		res += ndays[is_leap(tm->tm_year)][i];
+	res += tm->tm_mday - 1;
+	res *= 24;
+	res += tm->tm_hour;
+	res *= 60;
+	res += tm->tm_min;
+	res *= 60;
+	res += tm->tm_sec;
+	return res;
+}
-- 
1.8.2.540.gf023cfe

[PATCH 2/2 v2] compat/timegm: new compat function for systems lacking timegm()

[john]

The implementation used here is taken from Samba, since discussions on
their mailing list indicate that the "portable version" in timegm(3)
does not actually work on Solaris.

This commit also adds the infrastructure required for compat functions
in CGit itself.  Mostly we get everything we need from Git, but timegm
is not used in Git so there is no reason for them to carry a
compatibility version.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
Sorry, the previous version of this used "ifdef" in a Makefile, which is
clearly rubbish.  This version uses "ifneq" like it should have done to
begin with.

 cgit-compat-util.h | 11 ++++++++
 cgit.h             |  2 +-
 cgit.mk            | 14 ++++++++--
 compat/timegm.c    | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 102 insertions(+), 3 deletions(-)
 create mode 100644 cgit-compat-util.h
 create mode 100644 compat/timegm.c

diff --git a/cgit-compat-util.h b/cgit-compat-util.h
new file mode 100644
index 0000000..2573f32
--- /dev/null
+++ b/cgit-compat-util.h
@@ -0,0 +1,11 @@
+#ifndef CGIT_COMPAT_UTIL_H
+#define CGIT_COMPAT_UTIL_H
+
+#include "git/git-compat-util.h"
+
+#ifdef NO_TIMEGM
+#define timegm cgittimegm
+extern time_t cgittimegm(struct tm *tm);
+#endif
+
+#endif
diff --git a/cgit.h b/cgit.h
index ed5cf14..0a9f7f5 100644
--- a/cgit.h
+++ b/cgit.h
@@ -2,7 +2,7 @@
 #define CGIT_H
 
 
-#include <git-compat-util.h>
+#include "cgit-compat-util.h"
 #include <cache.h>
 #include <grep.h>
 #include <object.h>
diff --git a/cgit.mk b/cgit.mk
index bf3bbb1..aa2f344 100644
--- a/cgit.mk
+++ b/cgit.mk
@@ -15,12 +15,19 @@ $(CGIT_PREFIX)VERSION: force-version
 -include $(CGIT_PREFIX)VERSION
 .PHONY: force-version
 
+ifneq ($(NO_TIMEGM),)
+	CGIT_COMPAT_CFLAGS += -DNO_TIMEGM
+	CGIT_COMPAT_OBJ_NAMES += compat/timegm.o
+endif
+
 # CGIT_CFLAGS is a separate variable so that we can track it separately
 # and avoid rebuilding all of Git when these variables change.
 CGIT_CFLAGS += -DCGIT_CONFIG='"$(CGIT_CONFIG)"'
 CGIT_CFLAGS += -DCGIT_SCRIPT_NAME='"$(CGIT_SCRIPT_NAME)"'
 CGIT_CFLAGS += -DCGIT_CACHE_ROOT='"$(CACHE_ROOT)"'
 
+CGIT_CFLAGS += $(CGIT_COMPAT_CFLAGS)
+
 ifdef NO_C99_FORMAT
 	CFLAGS += -DNO_C99_FORMAT
 endif
@@ -52,7 +59,7 @@ CGIT_OBJ_NAMES += ui-tag.o
 CGIT_OBJ_NAMES += ui-tree.o
 CGIT_OBJ_NAMES += vector.o
 
-CGIT_OBJS := $(addprefix $(CGIT_PREFIX),$(CGIT_OBJ_NAMES))
+CGIT_OBJS := $(addprefix $(CGIT_PREFIX),$(CGIT_OBJ_NAMES) $(CGIT_COMPAT_OBJ_NAMES))
 
 # Only cgit.c reference CGIT_VERSION so we only rebuild its objects when the
 # version changes.
@@ -65,8 +72,11 @@ $(CGIT_VERSION_OBJS): EXTRA_CPPFLAGS = \
 ifeq ($(wildcard $(CGIT_PREFIX).depend),)
 missing_dep_dirs += $(CGIT_PREFIX).depend
 endif
+ifeq ($(wildcard $(CGIT_PREFIX)compat/.depend),)
+missing_dep_dirs += $(CGIT_PREFIX)compat/.depend
+endif
 
-$(CGIT_PREFIX).depend:
+$(addprefix $(CGIT_PREFIX),.depend compat/.depend):
 	@mkdir -p $@
 
 $(CGIT_PREFIX)CGIT-CFLAGS: FORCE
diff --git a/compat/timegm.c b/compat/timegm.c
new file mode 100644
index 0000000..1491538
--- /dev/null
+++ b/compat/timegm.c
@@ -0,0 +1,78 @@
+/*
+ * Copyright (c) 1997 Kungliga Tekniska H?gskolan
+ * (Royal Institute of Technology, Stockholm, Sweden).
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * 3. Neither the name of the Institute nor the names of its contributors
+ *    may be used to endorse or promote products derived from this software
+ *    without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE INSTITUTE AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL THE INSTITUTE OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+/*
+ * Adapted for Samba4 by Andrew Tridgell.
+ * Adapted for CGit by John Keeping.
+ */
+
+#include <time.h>
+
+static int is_leap(unsigned y)
+{
+	y += 1900;
+	return (y % 4) == 0 && ((y % 100) != 0 || (y % 400) == 0);
+}
+
+time_t cgittimegm(struct tm *tm)
+{
+	static const unsigned ndays[2][12] ={
+		{31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31},
+		{31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31}};
+	time_t res = 0;
+	unsigned i;
+
+	if (tm->tm_mon > 12 ||
+	    tm->tm_mon < 0 ||
+	    tm->tm_mday > 31 ||
+	    tm->tm_min > 60 ||
+	    tm->tm_sec > 60 ||
+	    tm->tm_hour > 24) {
+		/* invalid tm structure */
+		return 0;
+	}
+
+	for (i = 70; i < tm->tm_year; ++i)
+		res += is_leap(i) ? 366 : 365;
+
+	for (i = 0; i < tm->tm_mon; ++i)
+		res += ndays[is_leap(tm->tm_year)][i];
+	res += tm->tm_mday - 1;
+	res *= 24;
+	res += tm->tm_hour;
+	res *= 60;
+	res += tm->tm_min;
+	res *= 60;
+	res += tm->tm_sec;
+	return res;
+}
-- 
1.8.2.540.gf023cfe

[PATCH 2/2 v2] compat/timegm: new compat function for systems lacking timegm()

[Jason]

Hi John,

Is this strictly necessary? Would we be perhaps better off working
around gmtime by using an alternative function all together?  Or, if
we do require a compat version, the man page suggests this much
simpler alternative:

>        For a portable version of timegm(), set the TZ environment variable to UTC, call mktime(3) and restore the value of TZ.  Something like

>            #include <time.h>
>            #include <stdlib.h>
>            time_t
>            my_timegm(struct tm *tm)
>            {
>                time_t ret;
>                char *tz;
>                tz = getenv("TZ");
>                setenv("TZ", "", 1);
>                tzset();
>                ret = mktime(tm);
>                if (tz)
>                    setenv("TZ", tz, 1);
>                else
>                    unsetenv("TZ");
>                tzset();
>                return ret;
>            }


This might not be entirely safe, were cgit to ever become
multi-threaded, but it's certainly a lot simpler than rolling our own.

Jason

[PATCH 2/2 v2] compat/timegm: new compat function for systems lacking timegm()

[john]

On Thu, Apr 04, 2013 at 04:13:04PM +0200, Jason A. Donenfeld wrote:
> Is this strictly necessary? Would we be perhaps better off working
> around gmtime by using an alternative function all together?  Or, if
> we do require a compat version, the man page suggests this much
> simpler alternative:
> 
> >        For a portable version of timegm(), set the TZ environment variable to UTC, call mktime(3) and restore the value of TZ.  Something like
> 
> >            #include <time.h>
> >            #include <stdlib.h>
> >            time_t
> >            my_timegm(struct tm *tm)
> >            {
> >                time_t ret;
> >                char *tz;
> >                tz = getenv("TZ");
> >                setenv("TZ", "", 1);
> >                tzset();
> >                ret = mktime(tm);
> >                if (tz)
> >                    setenv("TZ", tz, 1);
> >                else
> >                    unsetenv("TZ");
> >                tzset();
> >                return ret;
> >            }
> 
> 
> This might not be entirely safe, were cgit to ever become
> multi-threaded, but it's certainly a lot simpler than rolling our own.

I decided not to do that based on this message:

    http://lists.samba.org/archive/samba-technical/2002-November/025578.html

but of course it doesn't provide any actual reason for why that version
doesn't work.

We only use timegm in week calculations, where we do:

    time_t t = timegm(tm);
    t = <new value>;
    gmtime_r(&t, tm);

so I wonder whether it would be equivalent to use mktime/localtime_r
instead of timegm/gmtime_r.

[PATCH 2/2 v2] compat/timegm: new compat function for systems lacking timegm()

[Jason]

On Thu, Apr 4, 2013 at 4:46 PM, John Keeping <john at keeping.me.uk> wrote:
>
> I decided not to do that based on this message:
>
>
> http://lists.samba.org/archive/samba-technical/2002-November/025578.html
>
> but of course it doesn't provide any actual reason for why that version
> doesn't work.
>

Aye yie yie. Time handling functions are a mess. I just went looking at how
glibc [1] and uclibc [2] actually go about doing things, and it's a real
mess in there. Looks like the replacement function in the patch might not
be completely correct.

[1] http://git.uclibc.org/uClibc/tree/libc/misc/time/time.c#n2330
[2]
http://sourceware.org/git/?p=glibc.git;a=blob;f=time/mktime.c;h=e75132c2e457af4b5629eef302c0d6dac15fef5c;hb=HEAD#l348
(uclibc gets bonus points for using cgit)




>
> We only use timegm in week calculations, where we do:
>
>     time_t t = timegm(tm);
>     t = <new value>;
>     gmtime_r(&t, tm);
>
> so I wonder whether it would be equivalent to use mktime/localtime_r
> instead of timegm/gmtime_r.
>


gmtime(timegm(tm)) == tm

and

localtime(mktime(tm)) == tm


So I can't imagine there'd be a problem with just replacing those functions.

[PATCH 2/2 v2] compat/timegm: new compat function for systems lacking timegm()

[john]

On Thu, Apr 04, 2013 at 04:59:50PM +0200, Jason A. Donenfeld wrote:
> On Thu, Apr 4, 2013 at 4:46 PM, John Keeping <john at keeping.me.uk> wrote:
> >
> > I decided not to do that based on this message:
> >
> >
> > http://lists.samba.org/archive/samba-technical/2002-November/025578.html
> >
> > but of course it doesn't provide any actual reason for why that version
> > doesn't work.
> >
> 
> Aye yie yie. Time handling functions are a mess. I just went looking at how
> glibc [1] and uclibc [2] actually go about doing things, and it's a real
> mess in there. Looks like the replacement function in the patch might not
> be completely correct.
> 
> [1] http://git.uclibc.org/uClibc/tree/libc/misc/time/time.c#n2330
> [2]
> http://sourceware.org/git/?p=glibc.git;a=blob;f=time/mktime.c;h=e75132c2e457af4b5629eef302c0d6dac15fef5c;hb=HEAD#l348
> (uclibc gets bonus points for using cgit)
> 
> >
> > We only use timegm in week calculations, where we do:
> >
> >     time_t t = timegm(tm);
> >     t = <new value>;
> >     gmtime_r(&t, tm);
> >
> > so I wonder whether it would be equivalent to use mktime/localtime_r
> > instead of timegm/gmtime_r.
> >
> 
> 
> gmtime(timegm(tm)) == tm
> 
> and
> 
> localtime(mktime(tm)) == tm
> 
> 
> So I can't imagine there'd be a problem with just replacing those functions.

Actually, there is a potential for a difference here, since
gmtime/timegm will not have daylight savings adjustments but
localtime/mktim might.  I suspect this means that the calculation we do
of adding/subtracting a week from the time could end up being an hour
out after converting back to struct tm.

I'm not sure what the best way to tackle this is, especially if there is
not a simple, correct replacement function that we can use.