Re: [PATCH 2/3] convert "enum date_mode" into a struct

From: Jeff King <peff@peff.net>
Date: Jul 7, 2015, 20:48 UTC
Message-ID: <20150707204837.GA15483@peff.net>
In-Reply-To: <xmqqbnfn3dsb.fsf@gitster.dls.corp.google.com>

On Tue, Jul 07, 2015 at 01:37:08PM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> >   3. Provide a wrapper that generates the correct struct on
> >      the fly. The big downside is that we end up pointing to
> >      a single global, which makes our wrapper non-reentrant.
> >      But show_date is already not reentrant, so it does not
> >      matter.
> >
> > This patch implements 3, along with a minor macro to keep
> > the size of the callers sane.
> 
> Another big downside is that DATE_NORMAL is defined to be "0".
> 
> This makes it very cumbersome to merge a side branch that uses an
> outdated definition of show_date() and its friends and tell them
> to show date normally.  The compiler does not help detecting
> places that need to be adjusted during merge and instead just pass
> a NULL pointer as a pointer to the new struct.

My assumption was that using the raw "0" is something we would frowned upon in new code. There was a single historical instance that I fixed in the series, but I wouldn't expect new ones (and actually, that instance was "1", which would be caught by the compiler).

However, if you're concerned, I think we could have show_date massage a NULL date, like:

diff --git a/date.c b/date.c
index 8f91569..a04d089 100644
--- a/date.c
+++ b/date.c
@@ -173,6 +173,10 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)
 {
 	struct tm *tm;
 	static struct strbuf timebuf = STRBUF_INIT;
+	static const struct fallback_mode = { DATE_NORMAL };
+
+	if (!mode)
+		mode = &fallback_mode;
 
 	if (mode->type == DATE_RAW) {
 		strbuf_reset(&timebuf);


That would also allow people to explicitly call:

  show_date(t, tz, NULL);

to get the default format, though I personally prefer spelling it out.

I guess we _could_ introduce:

  #define DATE_MODE(x) ((struct date_mode *)(x))

and then take any numeric value, under the assumption that the first
page of memory will never be a valid pointer:

diff --git a/date.c b/date.c
index 8f91569..f388fee 100644
--- a/date.c
+++ b/date.c
@@ -173,6 +173,18 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)
 {
 	struct tm *tm;
 	static struct strbuf timebuf = STRBUF_INIT;
+	struct date_mode fallback;
+
+	/* hysterical compatibility */
+	if (mode < 1024) {
+		if (mode == DATE_STRFTIME)
+			die("BUG: nice try");
+		fallback.type = mode;
+		mode = &fallback;
+	}
+
+	if (!mode)
+		mode = &fallback_mode;
 
 	if (mode->type == DATE_RAW) {
 		strbuf_reset(&timebuf);

That's kind of nasty, but at least it's hidden from the callers.

-Peff