[PATCH 0/4] replace unsinged long with time_t

From: David Bryson <redacted>

Here is a patch set from the Janitor page to replace unsigned long with time_t.
Since it overlaps slightly with a patch I made previously, I wrote this to
apply to next.  Comments welcome.

David Bryson (4):
  Changed timestamps to time_t instead of unsigned
  Changed timestamps to time_t in header files
  Changed timestamps to time_t instead of unsigned long for
    approxidate()
  Changed timestamps to time_t

 builtin-gc.c          |    2 +-
 builtin-prune.c       |    2 +-
 builtin-reflog.c      |   14 +++++++-------
 builtin-show-branch.c |    4 ++--
 cache.h               |    2 +-
 parse-options.c       |    2 +-
 refs.h                |    2 +-
 revision.h            |    4 ++--
 8 files changed, 16 insertions(+), 16 deletions(-)

From: David Bryson <redacted>

---
 cache.h    |    2 +-
 refs.h     |    2 +-
 revision.h |    4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index a3c77f0..2c114f8 100644
--- a/cache.h
+++ b/cache.h

@@ -625,7 +625,7 @@ enum date_mode {
 const char *show_date(unsigned long time, int timezone, enum date_mode mode);
 int parse_date(const char *date, char *buf, int bufsize);
 void datestamp(char *buf, int bufsize);
-unsigned long approxidate(const char *);
+time_t approxidate(const char *);
 enum date_mode parse_date_format(const char *format);
 
 #define IDENT_WARN_ON_NO_NAME  1

diff --git a/refs.h b/refs.h
index 06ad260..97c4bfe 100644
--- a/refs.h
+++ b/refs.h

@@ -55,7 +55,7 @@ extern void unlock_ref(struct ref_lock *lock);
 extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);
 
 /** Reads log for the value of ref during at_time. **/
-extern int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char *sha1, char **msg, unsigned long *cutoff_time, int *cutoff_tz, int *cutoff_cnt);
+extern int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char *sha1, char **msg, time_t *cutoff_time, int *cutoff_tz, int *cutoff_cnt);
 
 /* iterate over reflog entries */
 typedef int each_reflog_ent_fn(unsigned char *osha1, unsigned char *nsha1, const char *, unsigned long, int, const char *, void *);

diff --git a/revision.h b/revision.h
index 2fdb2dd..7cc9dbd 100644
--- a/revision.h
+++ b/revision.h

@@ -103,8 +103,8 @@ struct rev_info {
 	/* special limits */
 	int skip_count;
 	int max_count;
-	unsigned long max_age;
-	unsigned long min_age;
+	time_t max_age;
+	time_t min_age;
 
 	/* diff info for patches and for paths limiting */
 	struct diff_options diffopt;

-- 
1.6.0.1

From: David Bryson <redacted>

---
 builtin-gc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-gc.c b/builtin-gc.c
index 7af65bb..168f484 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c

@@ -58,7 +58,7 @@ static int gc_config(const char *var, const char *value, void *cb)
 	}
 	if (!strcmp(var, "gc.pruneexpire")) {
 		if (value && strcmp(value, "now")) {
-			unsigned long now = approxidate("now");
+			time_t now = approxidate("now");
 			if (approxidate(value) >= now)
 				return error("Invalid %s: '%s'", var, value);
 		}

-- 
1.6.0.1

From: David Bryson <redacted>

Some static values and return codes from approxidate()
have now been changed from unsigned long to time_t
---
 builtin-reflog.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-reflog.c b/builtin-reflog.c
index fdf78a9..f8ee25a 100644
--- a/builtin-reflog.c
+++ b/builtin-reflog.c

@@ -17,8 +17,8 @@ static const char reflog_expire_usage[] =
 static const char reflog_delete_usage[] =
 "git reflog delete [--verbose] [--dry-run] [--rewrite] [--updateref] <refs>...";
 
-static unsigned long default_reflog_expire;
-static unsigned long default_reflog_expire_unreachable;
+static time_t default_reflog_expire;
+static time_t default_reflog_expire_unreachable;
 
 struct cmd_reflog_expire_cb {
 	struct rev_info revs;

@@ -462,7 +462,7 @@ static void set_reflog_expiry_param(struct cmd_reflog_expire_cb *cb, int slot, c
 static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 {
 	struct cmd_reflog_expire_cb cb;
-	unsigned long now = time(NULL);
+	time_t now = time(NULL);
 	int i, status, do_all;
 	int explicit_expiry = 0;
 

-- 
1.6.0.1

From: David Bryson <redacted>

---
 builtin-prune.c       |    2 +-
 builtin-reflog.c      |    8 ++++----
 builtin-show-branch.c |    4 ++--
 parse-options.c       |    2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin-prune.c b/builtin-prune.c
index 7b4ec80..e1d46f0 100644
--- a/builtin-prune.c
+++ b/builtin-prune.c

@@ -12,7 +12,7 @@ static const char * const prune_usage[] = {
 };
 static int show_only;
 static int verbose;
-static unsigned long expire;
+static time_t expire;
 
 static int prune_tmp_object(const char *path, const char *filename)
 {

diff --git a/builtin-reflog.c b/builtin-reflog.c
index 6b3667e..fdf78a9 100644
--- a/builtin-reflog.c
+++ b/builtin-reflog.c

@@ -27,8 +27,8 @@ struct cmd_reflog_expire_cb {
 	int rewrite;
 	int updateref;
 	int verbose;
-	unsigned long expire_total;
-	unsigned long expire_unreachable;
+	time_t expire_total;
+	time_t expire_unreachable;
 	int recno;
 };
 

@@ -361,7 +361,7 @@ static struct reflog_expire_cfg *find_cfg_ent(const char *pattern, size_t len)
 	return ent;
 }
 
-static int parse_expire_cfg_value(const char *var, const char *value, unsigned long *expire)
+static int parse_expire_cfg_value(const char *var, const char *value, time_t *expire)
 {
 	if (!value)
 		return config_error_nonbool(var);

@@ -380,7 +380,7 @@ static int parse_expire_cfg_value(const char *var, const char *value, unsigned l
 static int reflog_expire_config(const char *var, const char *value, void *cb)
 {
 	const char *lastdot = strrchr(var, '.');
-	unsigned long expire;
+	time_t expire;
 	int slot;
 	struct reflog_expire_cfg *ent;
 

diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 306b850..73b3dc0 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c

@@ -716,7 +716,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			base = strtoul(reflog_base, &ep, 10);
 			if (*ep) {
 				/* Ah, that is a date spec... */
-				unsigned long at;
+				time_t at;
 				at = approxidate(reflog_base);
 				read_ref_at(ref, at, -1, sha1, NULL,
 					    NULL, NULL, &base);

@@ -726,7 +726,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		for (i = 0; i < reflog; i++) {
 			char *logmsg, *m;
 			const char *msg;
-			unsigned long timestamp;
+			time_t timestamp;
 			int tz;
 
 			if (read_ref_at(ref, 0, base+i, sha1, &logmsg,

diff --git a/parse-options.c b/parse-options.c
index fd08bb4..4581b50 100644
--- a/parse-options.c
+++ b/parse-options.c

@@ -480,7 +480,7 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
 int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
 			     int unset)
 {
-	*(unsigned long *)(opt->value) = approxidate(arg);
+	*(time_t *)(opt->value) = approxidate(arg);
 	return 0;
 }
 

-- 
1.6.0.1

On Thu, 6 Nov 2008, david@statichacks.org wrote:

Here is a patch set from the Janitor page to replace unsigned long with time_t.

I actually don't much like this.

"time_t" is one of those totally broken unix types. The standards say that 
it's an "arithmetic" type, but leaves it open to be just about anything. 
Traditionally, it's a signed integer (bad), and in theory it could even be 
a floating point value, I think.

And in _all_ such cases, it's actually better to cast it to "unsigned 
long" than keep time in a system-dependent format that is most likely 
either _already_ "unsigned long", or alternatively broken.

IOW, "unsigned long" is practically always either the same, or better 
than, time_t. Do you actually have a platform where that isn't the case?

And we do end up casting it to "unsigned long" in the end anyway - the 
date format in the commit is fundamentally not a signed one, and we use 
"%lu" to print those things. Again, if we were to use "time_t", we'd now 
have a huge and fundamental confusion about how to print them out, and 
what to do if they were negative.

So "time_t" really is a pretty damn worthless type. It's not _quite_ as 
broken as "socklen_t" (which is just a broken name for "int", and anybody 
who declares it to be anythign else is a total moron), but it's close.

In theory, some platform might have a 64-but "unsigned long long" time_t 
even if the architecture is 32-bit (apparently windows used to do that if 
you included <time64.h>, for example), but since we wouldn't take 
advantage of that anyway, even then there is no real advantage.

				Linus

Hi,

On Thu, Nov 06, 2008 at 10:13:46AM -0800 or thereabouts, Linus Torvalds wrote:

So "time_t" really is a pretty damn worthless type. It's not _quite_ as 
broken as "socklen_t" (which is just a broken name for "int", and anybody 
who declares it to be anythign else is a total moron), but it's close.

I have always thought that time_t and similar were braindead, but hey
the Janitor page listed it as desireable so what do I know ?

In theory, some platform might have a 64-but "unsigned long long" time_t 
even if the architecture is 32-bit (apparently windows used to do that if 
you included <time64.h>, for example), but since we wouldn't take 
advantage of that anyway, even then there is no real advantage.

Having a problem between 32 and 64 bit implementations does seem
undesireable.

http://git.or.cz/gitwiki/Janitor?action=info

Janitor wiki log says Pasky added the time_t conversion section.  Care
to explain the reason for the request Pasky ?

Dave

On Thu, 6 Nov 2008, David Bryson wrote:

I have always thought that time_t and similar were braindead, but hey
the Janitor page listed it as desireable so what do I know ?

It might be worth it to make the internal git time always be 64-bit.

It was kind of a long-term plan anyway: git doesn't really ever have to 
work with dates in the future (and things like "approxidate()" actually 
know that and use it to guess what date you must be talking about), so 
even a 32-bit "unsigned long" is expected to work well until 2038, but at 
_some_ point we'd need to guarantee 64-bit epoch times.

It just wasn't something I was in a huge hurry over. Others have to worry 
about dates from the future long before wrap-around, git really doesn't. 
But from a janitorial standpoint, I certainly wouldn't totally hate using 
a known 64-bit type for dates.

			Linus

On Thu, 6 Nov 2008, Linus Torvalds wrote:

In theory, some platform might have a 64-but "unsigned long long" time_t 
even if the architecture is 32-bit (apparently windows used to do that if 
you included <time64.h>, for example), but since we wouldn't take advantage 
of that anyway, even then there is no real advantage.

It could also be worth to notice that there are even 64-bit architectures that 
feature 32-bit 'time_t'...

-- 

  / daniel.haxx.se