USE_NSEC bug?

USE_NSEC bug?

[Andrew Rodland]

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

--

diff --git a/git-compat-util.h b/git-compat-util.h
index 90e0372..222caaa 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -638,10 +638,13 @@ void git_qsort(void *base, size_t nmemb, size_t size,
 # define FORCE_DIR_SET_GID 0
 #endif

 #ifdef NO_NSEC
 #undef USE_NSEC
+#endif
+
+#ifndef USE_NSEC
 #define ST_CTIME_NSEC(st) 0
 #define ST_MTIME_NSEC(st) 0
 #else
 #ifdef USE_ST_TIMESPEC
 #define ST_CTIME_NSEC(st) ((unsigned int)((st).st_ctimespec.tv_nsec))

--

Way two (bigger diff for a prettier result):

diff --git a/git-compat-util.h b/git-compat-util.h
index 90e0372..0a15b1a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -638,20 +638,23 @@ void git_qsort(void *base, size_t nmemb, size_t size,
 # define FORCE_DIR_SET_GID 0
 #endif

 #ifdef NO_NSEC
 #undef USE_NSEC
-#define ST_CTIME_NSEC(st) 0
-#define ST_MTIME_NSEC(st) 0
-#else
-#ifdef USE_ST_TIMESPEC
-#define ST_CTIME_NSEC(st) ((unsigned int)((st).st_ctimespec.tv_nsec))
-#define ST_MTIME_NSEC(st) ((unsigned int)((st).st_mtimespec.tv_nsec))
-#else
-#define ST_CTIME_NSEC(st) ((unsigned int)((st).st_ctim.tv_nsec))
-#define ST_MTIME_NSEC(st) ((unsigned int)((st).st_mtim.tv_nsec))
 #endif
+
+#ifdef USE_NSEC
+# ifdef USE_ST_TIMESPEC
+#  define ST_CTIME_NSEC(st) ((unsigned int)((st).st_ctimespec.tv_nsec))
+#  define ST_MTIME_NSEC(st) ((unsigned int)((st).st_mtimespec.tv_nsec))
+# else
+#  define ST_CTIME_NSEC(st) ((unsigned int)((st).st_ctim.tv_nsec))
+#  define ST_MTIME_NSEC(st) ((unsigned int)((st).st_mtim.tv_nsec))
+# endif
+#else /* !USE_NSEC */
+# define ST_CTIME_NSEC(st) 0
+# define ST_MTIME_NSEC(st) 0
 #endif

 #ifdef UNRELIABLE_FSTAT
 #define fstat_is_reliable() 0
 #else

Re: USE_NSEC bug?

[Jeff King]

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

> 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.

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

Re: USE_NSEC bug?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But maybe there is some subtle reason I'm missing for having the two
> options separate.

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.

Re: USE_NSEC bug?

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > But maybe there is some subtle reason I'm missing for having the two
> > options separate.
> 
> 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.

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

Re: USE_NSEC bug?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

I don't, either ;-)

Re: USE_NSEC bug?

[Andrew Rodland]

On Wed, Mar 20, 2013 at 1:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> 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.
>
> I don't, either ;-)
>

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

Re: USE_NSEC bug?

[Jeff King]

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

> 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.

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

Re: USE_NSEC bug?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Yes.

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

Re: USE_NSEC bug?

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > So I think your real complaint is "configure does not set NO_NSEC
> > properly", which is something worth fixing.
> 
> Yes.
> 
> I think our "Meh" was about "do we want to unify USE_NSEC and
> NO_NSEC?" and nothing else.

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