USE_NSEC bug?

While investigating this StackOverflow question: http://stackoverflow.com/questions/15516168/how-to-cross-compile-git-for-arm I found that fetch-pack.c uses ST_MTIME_NSEC outside of the protection of #ifdef USE_NSEC. This results in a broken build if !defined(USE_NSEC) && !defined(NO_NSEC) and the target system doesn't happen to be recent glibc.

I'm not sure where to pin the bug exactly — on fetch-pack.c, on git-compat-util.h, on configure for not seeing if one of those fields actually exists and setting NO_NSEC otherwise, or elsewhere, but I would be tempted to fix it in one of the two below ways, which should always be foolproof when !defined(USE_NSEC). That seems right to me, since USE_NSEC is an experimental-ish feature.

Please Cc any replies as I'm not subscribed.

Way one (smaller diff for an uglier result):

--

On Wed, Mar 20, 2013 at 02:36:32AM -0400, Andrew Rodland wrote:

Right; the point of NO_NSEC is to tell git that your libc does not have those fields. If it's not set, then it is a bug in your config.mak (or in the autoconf script, if you are using it).

That being said, I really don't see the point of having both USE_NSEC and NO_NSEC. If you do not set USE_NSEC (which most people do not; it is off by default), what is the point of not setting NO_NSEC? As far as I can tell, it means we will copy stat information out "struct st" into the index, but we will not actually _use_ it for anything.

Should there just be a single option USE_NSEC that defaults to off? And if it is not set, we do not even bother caring whether libc provides nsec fields, since we are not going to use them for anything (i.e., we just do the equivalent of NO_NSEC now).

Which I think is the direction you are proposing with your patches below, but the rationale should be "why bother requiring users to use the NO_NSEC knob explicitly when we know we do not care about its value anyway".

But maybe there is some subtle reason I'm missing for having the two options separate. Or maybe it's just a historical accident that we ended up here.

-Peff

Jeff King <peff@peff.net> writes:

The closest I found was c06ff4908bf9 (Record ns-timestamps if possible, but do not use it without USE_NSEC, 2009-03-04).

commit c06ff4908bf9ad8bf2448439a3574321c9399b17
Author: Kjetil Barvik <barvik@broadpark.no>
Date:   Wed Mar 4 18:47:40 2009 +0100

    Record ns-timestamps if possible, but do not use it without USE_NSEC
    
    Traditionally, the lack of USE_NSEC meant "do not record nor use the
    nanosecond resolution part of the file timestamps".  To avoid problems on
    filesystems that lose the ns part when the metadata is flushed to the disk
    and then later read back in, disabling USE_NSEC has been a good idea in
    general.
    
    If you are on a filesystem without such an issue, it does not hurt to read
    and store them in the cached stat data in the index entries even if your
    git is compiled without USE_NSEC.  The index left with such a version of
    git can be read by git compiled with USE_NSEC and it can make use of the
    nanosecond part to optimize the check to see if the path on the filesystem
    hsa been modified since we last looked at.

On Wed, Mar 20, 2013 at 10:04:14AM -0700, Junio C Hamano wrote:

Thanks, I suspected that might be the reason. IMHO it is not a big win, as you would just refresh the index when using the version compiled with USE_NSEC (so it really only helps you if you are frequently switching between the two versions, which seems silly).

And the cost is that we have another Makefile knob people need to tweak that would not otherwise need to be there. Which can be annoying, but is also not that huge a cost to deal with (we might want to improve the configure script or something, though).

I admit I don't care too much either way.

-Peff

Jeff King <peff@peff.net> writes:

I don't, either ;-)

On Wed, Mar 20, 2013 at 1:31 PM, Junio C Hamano <gitster@pobox.com> wrote:

For what it's worth, I'm not really invested either; I just figured that raising the issue was better than not, since I bumped into it.

I think it would be good if NO_NSEC and USE_ST_TIMESPEC were controlled by configure instead of config.mak, and it doesn't seem like too tall of an order.

Andrew

On Wed, Mar 20, 2013 at 02:22:27PM -0400, Andrew Rodland wrote:

There is no "instead of" here. The Makefile provides knobs which you can tweak on the command-line or via config.mak, and the configure script is expected to set any of those knobs it wants. But as most of the regular developers do not use the configure script themselves, it is often missing support for many of the knobs.

So I think your real complaint is "configure does not set NO_NSEC properly", which is something worth fixing.

-Peff

Jeff King <peff@peff.net> writes:

Yes.

I think our "Meh" was about "do we want to unify USE_NSEC and NO_NSEC?" and nothing else.

On Wed, Mar 20, 2013 at 11:52:28AM -0700, Junio C Hamano wrote:

Exactly. Now I am hoping Andrew will volunteer to fix configure.ac, as I will be happy to go to my grave never having had to figure out autoconf m4 syntax ever again. :)

-Peff