several date related issues

*** Dates do not respect LC_TIME

*** git log --date-order and --author-date-order do not order by date

Quoted 1 line · H.Merijn Brand

*** Dates do not respect LC_TIME

Right, we use our own routines for formatting the dates, and not strftime. And it probably should stay that way in general, as git's output is often meant to be parsed.

That being said, I do not think it would be wrong to have a date-mode that just showed strftime("%c"), or some other arbitrary strftime string. You could then set log.date as appropriate for human consumption.

Quoted 1 line

*** git log --date-order and --author-date-order do not order by date

Correct. The documentation says:

--date-order
Show no parents before all of its children are shown, but otherwise show commits in the commit timestamp order.

In your example, one commit is the parent of the other, so it hits the first part of the sentence, and the dates are never even compared.

There is not a simple way to show commits in arbitrary order without respect to parentage. I think you'd have to do something like:

  git log --format='%at %H' |
  sort -rn |
  cut -d' ' -f2 |
  git log --stdin --no-walk

Quoted 4 lines

That being said, I do not think it would be wrong to have a date-mode that just showed strftime("%c"), or some other arbitrary strftime string. You could then set log.date as appropriate for human consumption.

Yes please :)

--date=lc
--date=lc_time
--date=locale

all spring to mind as valid options

In preparation for adding date modes that may carry extra information beyond the mode itself, this patch converts the date_mode enum into a struct.

Most of the conversion is fairly straightforward; we pass the struct as a pointer and dereference the type field where necessary. Locations that declare a date_mode can use a "{}" constructor. However, the tricky case is where we use the enum labels as constants, like:

  show_date(t, tz, DATE_NORMAL);

Ideally we could say:

  show_date(t, tz, &{ DATE_NORMAL });

but of course C does not allow that.

Likewise, we cannot
cast the constant to a struct, because we need to pass an actual address. Our options are basically:

1. Manually add a "struct date_mode d = { DATE_NORMAL }" definition to each caller, and pass "&d". This makes the callers uglier, because they sometimes do not even have their own scope (e.g., they are inside a switch statement).

2. Provide a pre-made global "date_normal" struct that can be passed by address. We'd also need "date_rfc2822", "date_iso8601", and so forth. But at least the ugliness is defined in one place.

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.

Quoted 5 lines

Ideally we could say:

  show_date(t, tz, &{ DATE_NORMAL });

but of course C does not allow that.

Yes it does, e.g. in 6.5.2.5 of C11, example 3 shows:

Of course, whether all of the compilers we target support it is a different question. If they do, perhaps something like:

#define SIMPLE_DATE(f) &(struct date_mode) { DATE_NORMAL }

would allow the callers to remain reasonably sane.

... However, the tricky case is where we use the
enum labels as constants, like:

  show_date(t, tz, DATE_NORMAL);

Ideally we could say:

  show_date(t, tz, &{ DATE_NORMAL });

but of course C does not allow that.
...
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.

Quoted 8 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).

And that is because DATE_NORMAL is defined to be 0; we can claim that the compiler is being stupid to take one of the enum date_mode_type values that happens to be 0 and misinterpret it as the program wanted to pass a NULL pointer to a structure, but that is not what happened.

date.c const char *show_date(unsigned long time, int tz, const struct date_mode *mode) · line 176

Quoted 1 line

date.c +1 -0

+	static const struct fallback_mode = { DATE_NORMAL };

Yes, that is nasty. Renumbering the enum to begin with 1 may be a much saner solution, unless somebody does

In any case, I did another evil merge to fix it.

OK. Do you want to leave it be, then, or would you prefer me to do the NULL fallback? Or we could bump the enum to start with 1, and then explicitly treat "0" as a synonym for DATE_NORMAL (in case it comes in through a memset or similar).

This feeds the format directly to strftime. Besides being a little more flexible, the main advantage is that your system strftime may know more about your locale's preferred format (e.g., how to spell the days of the week).

Signed-off-by Jeff King <[email protected]>

---

strbuf.c +18 -0

index 0d4f4e5..a7ba028 100644
@@ -709,3 +709,32 @@ char *xstrfmt(const char *fmt, ...)
+void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
+{
+	size_t len;
+
+	/*
+	 * strftime reports "0" if it could not fit the result in the buffer.
+	 * Unfortunately, it also reports "0" if the requested time string
+	 * takes 0 bytes. So if we were to probe and grow, we have to choose
+	 * some arbitrary cap beyond which we guess that the format probably
+	 * just results in a 0-length output. Since we have to choose some
+	 * reasonable cap anyway, and since it is not that big, we may
+	 * as well just grow to their in the first place.
+	 */
+	strbuf_grow(sb, 128);
+	len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);
+
+	if (!len) {
+		/*

Clients of strbuf rightly expect the buffer to grow as needed in order to complete the requested operation. It is, therefore, both weird and expectation-breaking for strbuf_addftime() to lack this behavior. Worse, it doesn't even signal when the format has failed due to insufficient buffer space.

--- 8< ---
void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm) {
size_t len;
struct strbuf f = STRBUF_INIT;

/*
* This is a bit tricky since strftime returns 0 if the result did not
* fit in the supplied buffer, as well as when the formatted time has
* zero length. In the former case, we need to grow the buffer and try
* again. To distinguish between the two cases, we supply strftime with
* a format string one character longer than what the client supplied,
* which ensures that a successful format will have non-zero length,
* and then drop the extra character from the formatted time before
* returning.
*/
strbuf_addf(&f, "%s ", fmt);

	do {
		strbuf_grow(sb, 128);
		len = strftime(sb->buf + sb->len, sb->alloc - sb->len,
			       f.buf, tm);
	} while (!len);

[...]

If this is performance critical code, then the augmented format string can be constructed with less expensive functions than strbuf_addf().

Quoted 5 lines · Eric Sunshine

void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm) {
size_t len;
struct strbuf f = STRBUF_INIT;

Quoted 11 lines

/*
* This is a bit tricky since strftime returns 0 if the result did not
* fit in the supplied buffer, as well as when the formatted time has
* zero length. In the former case, we need to grow the buffer and try
* again. To distinguish between the two cases, we supply strftime with
* a format string one character longer than what the client supplied,
* which ensures that a successful format will have non-zero length,
* and then drop the extra character from the formatted time before
* returning.
*/
strbuf_addf(&f, "%s ", fmt);

Basically I was trying to avoid making any assumptions about exactly how strftime works. But presumably "stick a space in the format" is a universally reasonable thing to do. It's a hack, but it's contained to the function.

Quoted 5 lines

      do {
              strbuf_grow(sb, 128);
              len = strftime(sb->buf + sb->len, sb->alloc - sb->len,
                             f.buf, tm);
      } while (!len);

I think we need to keep growing this 128 ourselves, or else each loop iteration will just say "yup, we have 128 bytes available; no need to grow".

Quoted 3 lines

If this is performance critical code, then the augmented format string can be constructed with less expensive functions than strbuf_addf().

This does get called a lot (e.g., once per commit). One extra allocation would probably not kill us there [...]

[...] but I think we could fairly trivially put this on the unlikely path:

  size_t hint = 128;
  size_t len;

/* optimize out obvious 0-length case */
if (!*fmt)

        return;

  strbuf_grow(sb, hint);
  len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);

  /* maybe not enough room, or maybe 0-length output */
  if (!len) {
        struct strbuf f = STRBUF_INIT;
        strbuf_addf(&f, "%s ", fmt);
        while (!len) {
                hint *= 2;
                strbuf_grow(sb, hint);
                len = strftime(sb->buf + sb->len, sb->alloc - sb->len, f.buf, tm);
        }
  }

I'd guess most cases will fit in 128 bytes and never even hit this code path. You could also get fancier and start the buffer smaller, but only do the fmt hack when we cross a threshold.

Quoted 4 lines

Basically I was trying to avoid making any assumptions about exactly how strftime works. But presumably "stick a space in the format" is a universally reasonable thing to do. It's a hack, but it's contained to the function.

I don't think we're making any assumptions about strftime(). POSIX states:

The format string consists of zero or more conversion specifications and ordinary characters. [...] All ordinary characters (including the terminating NUL character) are copied unchanged into the array.

So, we seem to be on solid footing with this approach (even though it's a localized hack).

Yeah, I toyed with the idea of increasing the requested amount each iteration but wanted to keep the example simple, thus left it out. However, for some reason, I was thinking that strbuf_grow() was unconditionally expanding the buffer by the requested amount rather than merely ensuring that that amount was availabile, so the amount clearly needs to be increased on each iteration. Thanks for pointing that out.

Beyond the extra allocation, I was also concerned about the sledgehammer approach of "%s " to append a single character when there are much less expensive ways to do so.

Quoted 3 lines · Eric Sunshine

Beyond the extra allocation, I was also concerned about the sledgehammer approach of "%s " to append a single character when there are much less expensive ways to do so.

I don't think there's any other way. We have to feed a contiguous buffer to strftime, and we don't own the buffer, so we have to make a new copy.

Sorry, I meant that the interpolation expense of "%s ". A cheaper (but more verbose) alternative might be:

    size_t n = strlen(fmt);
    const char *f = xmalloc(n + 2);
    strcpy(f, fmt);
    f[n] = ' ';
    f[n + 1] = '\0';
    ...
    free(f);

or something similar.

Quoted 1 line

	strbuf_addf(&f, "%s ", fmt);

Basically I was trying to avoid making any assumptions about exactly how strftime works. But presumably "stick a space in the format" is a universally reasonable thing to do. It's a hack, but it's contained to the function.

Quoted 6 lines · Jeff King

Quoted 1 line

	strbuf_addf(&f, "%s ", fmt);

Basically I was trying to avoid making any assumptions about exactly how strftime works. But presumably "stick a space in the format" is a universally reasonable thing to do. It's a hack, but it's contained to the function.

Why can't I shake this feeling that (" %s", fmt), i.e. prepend not append, is the safer thing to do than to append?

Quoted 9 lines · Junio C Hamano

Quoted 6 lines · Jeff King

Quoted 1 line

	strbuf_addf(&f, "%s ", fmt);

Basically I was trying to avoid making any assumptions about exactly how strftime works. But presumably "stick a space in the format" is a universally reasonable thing to do. It's a hack, but it's contained to the function.

Quoted 2 lines

Why can't I shake this feeling that (" %s", fmt), i.e. prepend not append, is the safer thing to do than to append?

Because then removing the extra space involves memmove of the buffer, rather than just shortening the length by one.

Quoted 5 lines

Quoted 2 lines

Why can't I shake this feeling that (" %s", fmt), i.e. prepend not append, is the safer thing to do than to append?

Because then removing the extra space involves memmove of the buffer, rather than just shortening the length by one.

That does not explain why I feel the other way is safer, though ;-)

In any case, responding myself to my other question, strftime(3) does not define any useful error information even for a case where you feed nonsense format to it, so it is even not possible to protect ourselves by checking errno or doing something similar X-<.

This does get called a lot (e.g., once per commit). One extra allocation would probably not kill us there, but I think we could fairly trivially put this on the unlikely path:

  size_t hint = 128;
  size_t len;

/* optimize out obvious 0-length case */
if (!*fmt)

	return;

  strbuf_grow(sb, hint);
  len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);

/* maybe not enough room, or maybe 0-length output */

  if (!len) {
	struct strbuf f = STRBUF_INIT;
	strbuf_addf(&f, "%s ", fmt);
	while (!len) {
		hint *= 2;
		strbuf_grow(sb, hint);
		len = strftime(sb->buf + sb->len, sb->alloc - sb->len, f.buf, tm);
	}

}

I'd guess most cases will fit in 128 bytes and never even hit this code path. You could also get fancier and start the buffer smaller, but only do the fmt hack when we cross a threshold.

Quoted 3 lines

I'd guess most cases will fit in 128 bytes and never even hit this code path. You could also get fancier and start the buffer smaller, but only do the fmt hack when we cross a threshold.

I'd assume that the "hint" thing will persist across calls somehow? In an invocation of "git log --date=format:<some format>" for millions of commits, it is likely that the length of the formatted date string will stay the same or close to the same (yeah, I know "Wednesday" would be longer than "Monday").

Answering myself to my earlier question, the reason is because I was worried what happens when given fmt is a malformed strftime format specifier. Perhaps it ends with a lone % and "% " may format to something unexpected, or something.

Are we checking an error from strftime(3)?

Answering myself to my earlier question, the reason is because I was worried what happens when given fmt is a malformed strftime format specifier. Perhaps it ends with a lone % and "% " may format to something unexpected, or something.

Are we checking an error from strftime(3)?

Clients of strbuf rightly expect the buffer to grow as needed in order to complete the requested operation. It is, therefore, both weird and expectation-breaking for strbuf_addftime() to lack this behavior. Worse, it doesn't even signal when the format has failed due to insufficient buffer space.

How about taking this approach (or something similar), instead, which grows the strbuf as needed?

Quoted 6 lines · Eric Sunshine

Clients of strbuf rightly expect the buffer to grow as needed in order to complete the requested operation. It is, therefore, both weird and expectation-breaking for strbuf_addftime() to lack this behavior. Worse, it doesn't even signal when the format has failed due to insufficient buffer space.

Quoted 2 lines

How about taking this approach (or something similar), instead, which grows the strbuf as needed?

Here's a patch, on top of jk/date-mode-format (I think it would also be fine to just squash into the tip commit; the explanation in the commit message is sufficiently mirrored in the code comment).

-- >8 --
Subject: [PATCH] strbuf: make strbuf_addftime more robust

The return value of strftime is poorly designed; when it returns 0, the caller cannot tell if the buffer was not large enough, or if the output was actually 0 bytes. In the original implementation of strbuf_addftime, we simply punted and guessed that our 128-byte hint would be large enough.

We can do better, though, if we're willing to treat strftime like less of a black box. We can munge the incoming format to make sure that it never produces 0-length output, and then "fix" the resulting output. That lets us reliably grow the buffer based on strftime's return value.

Clever-idea-by Eric Sunshine <[email protected]>

Signed-off-by Jeff King <[email protected]>

---
 strbuf.c                | 38 +++++++++++++++++++++-----------------
 t/t6300-for-each-ref.sh | 10 ++++++++++
 2 files changed, 31 insertions(+), 17 deletions(-)

strbuf.c +21 -17

index a7ba028..e5e7370 100644
@@ -712,29 +712,33 @@ char *xstrfmt(const char *fmt, ...)
 
 void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
 {
+       size_t hint = 128;
        size_t len;
 
-       /*
-        * strftime reports "0" if it could not fit the result in the buffer.
-        * Unfortunately, it also reports "0" if the requested time string
-        * takes 0 bytes. So if we were to probe and grow, we have to choose
-        * some arbitrary cap beyond which we guess that the format probably
-        * just results in a 0-length output. Since we have to choose some
-        * reasonable cap anyway, and since it is not that big, we may
-        * as well just grow to their in the first place.
-        */
-       strbuf_grow(sb, 128);
+       if (!*fmt)
+               return;
+
+       strbuf_grow(sb, hint);
        len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);
 
        if (!len) {
                /*
-                * Either we failed, or the format actually produces a 0-length
-                * output. There's not much we can do, so we leave it blank.
-                * However, the output array is left in an undefined state, so
-                * we must re-assert our NUL terminator.
+                * strftime reports "0" if it could not fit the result in the buffer.
+                * Unfortunately, it also reports "0" if the requested time string
+                * takes 0 bytes. So our strategy is to munge the format so that the
+                * output contains at least one character, and then drop the extra
+                * character before returning.
                 */
-               sb->buf[sb->len] = '\0';
-       } else {
-               sb->len += len;
+               struct strbuf munged_fmt = STRBUF_INIT;
+               strbuf_addf(&munged_fmt, "%s ", fmt);
+               while (!len) {
+                       hint *= 2;
+                       strbuf_grow(sb, hint);
+                       len = strftime(sb->buf + sb->len, sb->alloc - sb->len,
+                                      munged_fmt.buf, tm);
+               }
+               strbuf_release(&munged_fmt);
+               len--; /* drop munged space */
        }
+       strbuf_setlen(sb, sb->len + len);
 }

t/t6300-for-each-ref.sh +10 -1

index c7f368c..7c9bec7 100755
@@ -235,6 +235,16 @@ test_expect_success 'Check format of strftime date fields' '
        test_cmp expected actual
 '
 
+test_expect_success 'exercise strftime with odd fields' '
+       echo >expected &&
+       git for-each-ref --format="%(authordate:format:)" refs/heads >actual &&
+       test_cmp expected actual &&
+       long="long format -- $_z40$_z40$_z40$_z40$_z40$_z40$_z40" &&
+       echo $long >expected &&
+       git for-each-ref --format="%(authordate:format:$long)" refs/heads >actual &&
+       test_cmp expected actual
+'
+
 cat >expected <<\EOF
 refs/heads/master
 refs/remotes/origin/master
--

2.5.0.rc0.336.g8460790

Quoted 6 lines · Eric Sunshine

Clients of strbuf rightly expect the buffer to grow as needed in order to complete the requested operation. It is, therefore, both weird and expectation-breaking for strbuf_addftime() to lack this behavior. Worse, it doesn't even signal when the format has failed due to insufficient buffer space.

Quoted 2 lines

How about taking this approach (or something similar), instead, which grows the strbuf as needed?

Here's a patch, on top of jk/date-mode-format (I think it would also be fine to just squash into the tip commit; the explanation in the commit message is sufficiently mirrored in the code comment).

-- >8 --
Subject: [PATCH] strbuf: make strbuf_addftime more robust

The return value of strftime is poorly designed; when it returns 0, the caller cannot tell if the buffer was not large enough, or if the output was actually 0 bytes. In the original implementation of strbuf_addftime, we simply punted and guessed that our 128-byte hint would be large enough.

We can do better, though, if we're willing to treat strftime like less of a black box. We can munge the incoming format to make sure that it never produces 0-length output, and then "fix" the resulting output. That lets us reliably grow the buffer based on strftime's return value.

Clever-idea-by Eric Sunshine <[email protected]>

Signed-off-by Jeff King <[email protected]>

---
 strbuf.c                | 38 +++++++++++++++++++++-----------------
 t/t6300-for-each-ref.sh | 10 ++++++++++
 2 files changed, 31 insertions(+), 17 deletions(-)

strbuf.c +21 -17

index a7ba028..e5e7370 100644
@@ -712,29 +712,33 @@ char *xstrfmt(const char *fmt, ...)
 
 void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
 {
+       size_t hint = 128;
        size_t len;
 
-       /*
-        * strftime reports "0" if it could not fit the result in the buffer.
-        * Unfortunately, it also reports "0" if the requested time string
-        * takes 0 bytes. So if we were to probe and grow, we have to choose
-        * some arbitrary cap beyond which we guess that the format probably
-        * just results in a 0-length output. Since we have to choose some
-        * reasonable cap anyway, and since it is not that big, we may
-        * as well just grow to their in the first place.
-        */
-       strbuf_grow(sb, 128);
+       if (!*fmt)
+               return;
+
+       strbuf_grow(sb, hint);
        len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);
 
        if (!len) {
                /*
-                * Either we failed, or the format actually produces a 0-length
-                * output. There's not much we can do, so we leave it blank.
-                * However, the output array is left in an undefined state, so
-                * we must re-assert our NUL terminator.
+                * strftime reports "0" if it could not fit the result in the buffer.
+                * Unfortunately, it also reports "0" if the requested time string
+                * takes 0 bytes. So our strategy is to munge the format so that the
+                * output contains at least one character, and then drop the extra
+                * character before returning.
                 */
-               sb->buf[sb->len] = '\0';
-       } else {
-               sb->len += len;
+               struct strbuf munged_fmt = STRBUF_INIT;
+               strbuf_addf(&munged_fmt, "%s ", fmt);
+               while (!len) {
+                       hint *= 2;
+                       strbuf_grow(sb, hint);
+                       len = strftime(sb->buf + sb->len, sb->alloc - sb->len,
+                                      munged_fmt.buf, tm);
+               }
+               strbuf_release(&munged_fmt);
+               len--; /* drop munged space */
        }
+       strbuf_setlen(sb, sb->len + len);
 }

t/t6300-for-each-ref.sh +10 -1

index c7f368c..7c9bec7 100755
@@ -235,6 +235,16 @@ test_expect_success 'Check format of strftime date fields' '
        test_cmp expected actual
 '
 
+test_expect_success 'exercise strftime with odd fields' '
+       echo >expected &&
+       git for-each-ref --format="%(authordate:format:)" refs/heads >actual &&
+       test_cmp expected actual &&
+       long="long format -- $_z40$_z40$_z40$_z40$_z40$_z40$_z40" &&
+       echo $long >expected &&
+       git for-each-ref --format="%(authordate:format:$long)" refs/heads >actual &&
+       test_cmp expected actual
+'
+
 cat >expected <<\EOF
 refs/heads/master
 refs/remotes/origin/master
--

2.5.0.rc0.336.g8460790

Quoted 3 lines

Here's a patch, on top of jk/date-mode-format (I think it would also be fine to just squash into the tip commit; the explanation in the commit message is sufficiently mirrored in the code comment).

While cleaning up old local branches, I noticed that, although the jk/date-mode-format topic[1] made it into 'next' (and will be merged to 'master' according to "What's cooking"[2]), the below follow-on patch[3] which improves strbuf_addftime() never got picked up. Was this omission intentional? Based upon the discussion[4], I was under the impression that the patch was considered reasonably acceptable (and did not worsen problems with bogus format strings -- which are bogus anyway).