Re: [PATCH 3/3] introduce "format" date-mode

From: Jeff King <peff@peff.net>
Date: Jun 30, 2015, 19:17 UTC
Message-ID: <20150630191758.GA6845@peff.net>
In-Reply-To: <xmqqr3ot6s9u.fsf@gitster.dls.corp.google.com>

On Tue, Jun 30, 2015 at 10:05:33AM -0700, Junio C Hamano wrote:

Show 9 quoted 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").

I hadn't thought about that. It could persist, but I don't think this is necessarily the right place to do it. For two reasons:

  1. You have no idea in strbuf_addftime if it's the same fmt being
     added over and over. This is the wrong place to make that
     optimization.

  2. If you are interested in efficiency in a loop, then you should be
     reusing the same strbuf over and over, and avoiding the extra
     allocation in the first place. And that is indeed what we do for
     "git log --date", as we will always use the same static-local
     buffer in show_date().

Show 6 quoted lines

> 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)?

POSIX doesn't define any errno values for strftime (and in fact says "No errors are defined"). The return value description for POSIX (and the glibc manpage) talk about only whether or not the output fits. However, POSIX does say "If a conversion specifier is not one of the above, the behavior is undefined".

So certainly I could imagine an implementation that returns "0" when you feed it a bogus value. If you (as a user) feed us crap to give to strftime, I am not particularly concerned with whether you get crap out. My main concern is that it would return "0" and we would loop forever. OTOH, I think any sane implementation would simply copy unknown placeholders out (certainly glibc does that). So I think we could simply consider it a quality of implementation issue, and deal with any particular crappy implementations if and when they get reported. We could add something tricky (like "--date=format:%") to the test suite to make it likelier to catch such a thing.

-Peff