[PATCH 1/3] configure.ac: check tv_nsec field in struct stat

[PATCH 1/3] configure.ac: check tv_nsec field in struct stat

[Reuben Hawkins]

This check will automatically set the correct NO_NSEC setting.
---
 configure.ac | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/configure.ac b/configure.ac
index 6af9647..3cfdd51 100644
--- a/configure.ac
+++ b/configure.ac
@@ -754,6 +754,25 @@ AC_CHECK_TYPES([struct itimerval],
 [#include <sys/time.h>])
 GIT_CONF_SUBST([NO_STRUCT_ITIMERVAL])
 #
+# Define HAVE_ST_MTIM=No if you don't have struct stat.st_mtim.tv_nsec.
+AC_CHECK_MEMBER([struct stat.st_mtim.tv_nsec],
+[HAVE_ST_MTIM=Yes],
+[HAVE_ST_MTIM=No],
+[#include <sys/stat.h>])
+#
+# Define HAVE_ST_MTIMESPEC=No if you don't have struct stat.st_mtimespec.tv_nsec.
+AC_CHECK_MEMBER([struct stat.st_mtimespec.tv_nsec],
+[HAVE_ST_MTIMESPEC=Yes],
+[HAVE_ST_MTIMESPEC=No],
+[#include <sys/stat.h>])
+#
+# Define NO_NSEC if both HAVE_ST_MTIMESPEC and HAVE_ST_MTIM are set to No.
+if test '(' "$HAVE_ST_MTIM" = "No" ')' -a '(' "$HAVE_ST_MTIMESPEC" = "No" ')' ; then
+	NO_NSEC=YesPlease
+	GIT_CONF_SUBST([NO_NSEC])
+fi
+
+#
 # Define NO_D_INO_IN_DIRENT if you don't have d_ino in your struct dirent.
 AC_CHECK_MEMBER(struct dirent.d_ino,
 [NO_D_INO_IN_DIRENT=],
-- 
2.2.0.GIT

[PATCH 2/3] configure.ac,trace.c: check for CLOCK_MONOTONIC

[Reuben Hawkins]

CLOCK_MONOTONIC isn't available on RHEL3, but there are still RHEL3
systems being used in production.  This change makes compiling git
less tedious on older platforms.
---
 configure.ac | 26 ++++++++++++++++++++++++++
 trace.c      |  2 +-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 3cfdd51..3900044 100644
--- a/configure.ac
+++ b/configure.ac
@@ -736,8 +736,10 @@ GIT_UNSTASH_FLAGS($ICONVDIR)
 
 GIT_CONF_SUBST([OLD_ICONV])
 
+
 ## Checks for typedefs, structures, and compiler characteristics.
 AC_MSG_NOTICE([CHECKS for typedefs, structures, and compiler characteristics])
+
 #
 TYPE_SOCKLEN_T
 case $ac_cv_type_socklen_t in
@@ -930,6 +932,30 @@ AC_CHECK_LIB([iconv], [locale_charset],
                      [CHARSET_LIB=-lcharset])])
 GIT_CONF_SUBST([CHARSET_LIB])
 #
+# Define NO_CLOCK_GETTIME if you don't have clock_gettime.
+GIT_CHECK_FUNC(clock_gettime,
+[HAVE_CLOCK_GETTIME=Yes],
+[HAVE_CLOCK_GETTIME=])
+GIT_CONF_SUBST([HAVE_CLOCK_GETTIME])
+
+AC_DEFUN([CLOCK_MONOTONIC_SRC], [
+AC_LANG_PROGRAM([[
+#include <time.h>
+clockid_t id = CLOCK_MONOTONIC;
+]], [])])
+
+#
+# Define NO_CLOCK_MONOTONIC on really old systems that are still in production
+# if you need GIT to compile but can't update the machine otherwise.
+AC_MSG_CHECKING([for CLOCK_MONOTONIC])
+AC_COMPILE_IFELSE([CLOCK_MONOTONIC_SRC],
+	[AC_MSG_RESULT([yes])
+	HAVE_CLOCK_MONOTONIC=Yes],
+	[AC_MSG_RESULT([no])
+	HAVE_CLOCK_MONOTONIC=])
+
+GIT_CONF_SUBST([HAVE_CLOCK_MONOTONIC])
+#
 # Define NO_SETITIMER if you don't have setitimer.
 GIT_CHECK_FUNC(setitimer,
 [NO_SETITIMER=],
diff --git a/trace.c b/trace.c
index 4778608..bfbd48f 100644
--- a/trace.c
+++ b/trace.c
@@ -324,7 +324,7 @@ int trace_want(struct trace_key *key)
 	return !!get_trace_fd(key);
 }
 
-#ifdef HAVE_CLOCK_GETTIME
+#if defined(HAVE_CLOCK_GETTIME) && defined(HAVE_CLOCK_MONOTONIC)
 
 static inline uint64_t highres_nanos(void)
 {
-- 
2.2.0.GIT

[PATCH 3/3] configure.ac,imap-send.c: check HMAC_CTX_cleanup

[Reuben Hawkins]

Older versions of OpenSSL have HMAC_cleanup, but not HMAC_CTX_cleanup.
This change checks for both, uses HMAC_CTX_cleanup on platforms which
have it, otherwise falls back to HMAC_cleanup.

This changes makes building GIT on older platforms less tedious.
---
 Makefile     |  6 ++++++
 configure.ac | 13 +++++++++++++
 imap-send.c  |  6 ++++++
 3 files changed, 25 insertions(+)

diff --git a/Makefile b/Makefile
index 7482a4d..a495d94 100644
--- a/Makefile
+++ b/Makefile
@@ -1059,6 +1059,12 @@ ifndef NO_OPENSSL
 	ifdef NEEDS_CRYPTO_WITH_SSL
 		OPENSSL_LIBSSL += -lcrypto
 	endif
+	ifdef HAVE_HMAC_CTX_CLEANUP
+		BASIC_CFLAGS += -DHAVE_HMAC_CTX_CLEANUP
+	endif
+	ifdef HAVE_HMAC_CLEANUP
+		BASIC_CFLAGS += -DHAVE_HMAC_CLEANUP
+	endif
 else
 	BASIC_CFLAGS += -DNO_OPENSSL
 	BLK_SHA1 = 1
diff --git a/configure.ac b/configure.ac
index 3900044..b22788c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -899,6 +899,7 @@ GIT_CONF_SUBST([SNPRINTF_RETURNS_BOGUS])
 ## Checks for library functions.
 ## (in default C library and libraries checked by AC_CHECK_LIB)
 AC_MSG_NOTICE([CHECKS for library functions])
+
 #
 # Define NO_LIBGEN_H if you don't have libgen.h.
 AC_CHECK_HEADER([libgen.h],
@@ -932,6 +933,18 @@ AC_CHECK_LIB([iconv], [locale_charset],
                      [CHARSET_LIB=-lcharset])])
 GIT_CONF_SUBST([CHARSET_LIB])
 #
+# Define HAVE_HMAC_CTX_CLEANUP=Yes if we have the newer HMAC cleanup function
+AC_CHECK_LIB([crypto], [HMAC_CTX_cleanup],
+	[HAVE_HMAC_CTX_CLEANUP=Yes],
+	[], [])
+GIT_CONF_SUBST([HAVE_HMAC_CTX_CLEANUP])
+#
+# Define HAVE_HMAC_CLEANUP=Yes if we have the older HMAC cleanup function
+AC_CHECK_LIB([crypto], [HMAC_cleanup],
+	[HAVE_HMAC_CLEANUP=Yes],
+	[], [])
+GIT_CONF_SUBST([HAVE_HMAC_CLEANUP])
+#
 # Define NO_CLOCK_GETTIME if you don't have clock_gettime.
 GIT_CHECK_FUNC(clock_gettime,
 [HAVE_CLOCK_GETTIME=Yes],
diff --git a/imap-send.c b/imap-send.c
index 70bcc7a..eec2378 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -861,7 +861,13 @@ static char *cram(const char *challenge_64, const char *user, const char *pass)
 	HMAC_Init(&hmac, (unsigned char *)pass, strlen(pass), EVP_md5());
 	HMAC_Update(&hmac, (unsigned char *)challenge, decoded_len);
 	HMAC_Final(&hmac, hash, NULL);
+#if defined(HAVE_HMAC_CTX_CLEANUP)
 	HMAC_CTX_cleanup(&hmac);
+#elif defined(HAVE_HMAC_CLEANUP)
+	HMAC_cleanup(&hmac);
+#else
+# error "no HMAC_cleanup function"
+#endif
 
 	hex[32] = 0;
 	for (i = 0; i < 16; i++) {
-- 
2.2.0.GIT

Re: [PATCH 1/3] configure.ac: check tv_nsec field in struct stat

[Eric Sunshine]

On Sun, Dec 21, 2014 at 1:53 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
> This check will automatically set the correct NO_NSEC setting.

Missing sign-off. See git/Documentation/SubmittingPatches.

> ---
> diff --git a/configure.ac b/configure.ac
> index 6af9647..3cfdd51 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -754,6 +754,25 @@ AC_CHECK_TYPES([struct itimerval],
>  [#include <sys/time.h>])
>  GIT_CONF_SUBST([NO_STRUCT_ITIMERVAL])
>  #
> +# Define HAVE_ST_MTIM=No if you don't have struct stat.st_mtim.tv_nsec.

This comment is misleading. HAVE_ST_MTIM is never actually #defined or
set manually by the user, or used anywhere outside of the conditional
below. Also, the name itself is misleading: HAVE_ST_MTIM_NSEC would be
more appropriate.

> +AC_CHECK_MEMBER([struct stat.st_mtim.tv_nsec],
> +[HAVE_ST_MTIM=Yes],
> +[HAVE_ST_MTIM=No],

In Autoconf, it's customary to use lowercase values (such as "yes"
rather than "Yes") for these temporary variables. A "no" value is
usually represented by an empty assignment (HAVE_ST_MTIM=). However,
AC_CHECK_MEMBER() assigns the test result automatically to a shell
variable (in this case, named
ac_cv_member_struct_stat_st_mtim_tv_nsec), so there is no need to
invent your own (HAVE_ST_MTIM).

So, you can reduce this to:

   AC_CHECK_MEMBER([struct stat.st_mtim.tv_nsec])

and later check the value with

    test x$ac_cv_member_struct_stat_st_mtim_tv_nsec = yes

> +[#include <sys/stat.h>])
> +#
> +# Define HAVE_ST_MTIMESPEC=No if you don't have struct stat.st_mtimespec.tv_nsec.

Ditto regarding misleading comment and variable name.

> +AC_CHECK_MEMBER([struct stat.st_mtimespec.tv_nsec],
> +[HAVE_ST_MTIMESPEC=Yes],
> +[HAVE_ST_MTIMESPEC=No],
> +[#include <sys/stat.h>])
> +#
> +# Define NO_NSEC if both HAVE_ST_MTIMESPEC and HAVE_ST_MTIM are set to No.
> +if test '(' "$HAVE_ST_MTIM" = "No" ')' -a '(' "$HAVE_ST_MTIMESPEC" = "No" ')' ; then

Use of  'test -a' is unportable. The Autoconf manual has this to say:

    The -a, -o, ‘(’, and ‘)’ operands are not present in all
    implementations, and have been marked obsolete by Posix 2008.

Instead, use:

    test ... && test ....

> +       NO_NSEC=YesPlease
> +       GIT_CONF_SUBST([NO_NSEC])

git-compat-util.h also needs to know if it should use st_ctimespec
rather than st_ctim. By this point, although indirect, you've
determined which of the two is available (if any), so as a bonus, you
can tell the build system about that, as well. Something like this,
perhaps:

    if test x$ac_cv_member_struct_stat_st_mtimspec_tv_nsec = xyes; then
        USE_ST_TIMESPEC=YesPlease
        GIT_CONF_SUBST([USE_ST_TIMESPEC])
    elif test x$ac_cv_member_struct_stat_st_mtime_tv_nsec != xyes; then
        NO_NSEC=YesPlease
        GIT_CONF_SUBST([NO_NSEC])
    fi

> +fi
> +
> +#
>  # Define NO_D_INO_IN_DIRENT if you don't have d_ino in your struct dirent.
>  AC_CHECK_MEMBER(struct dirent.d_ino,
>  [NO_D_INO_IN_DIRENT=],
> --
> 2.2.0.GIT

Re: [PATCH 2/3] configure.ac,trace.c: check for CLOCK_MONOTONIC

[Eric Sunshine]

On Sun, Dec 21, 2014 at 1:53 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
> CLOCK_MONOTONIC isn't available on RHEL3, but there are still RHEL3
> systems being used in production.  This change makes compiling git
> less tedious on older platforms.

Missing sign-off.

> ---
> diff --git a/configure.ac b/configure.ac
> index 3cfdd51..3900044 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -736,8 +736,10 @@ GIT_UNSTASH_FLAGS($ICONVDIR)
>
>  GIT_CONF_SUBST([OLD_ICONV])
>
> +
>  ## Checks for typedefs, structures, and compiler characteristics.
>  AC_MSG_NOTICE([CHECKS for typedefs, structures, and compiler characteristics])
> +

Sneaking in a couple whitespace changes unrelated the patch's stated
purpose. Such cleanup should be done typically as a separate
preparatory patch (though they probably aren't warranted in this
case).

>  #
>  TYPE_SOCKLEN_T
>  case $ac_cv_type_socklen_t in
> @@ -930,6 +932,30 @@ AC_CHECK_LIB([iconv], [locale_charset],
>                       [CHARSET_LIB=-lcharset])])
>  GIT_CONF_SUBST([CHARSET_LIB])
>  #
> +# Define NO_CLOCK_GETTIME if you don't have clock_gettime.

The comment talks about NO_CLOCK_GETTIME, however, the code names it
HAVE_CLOCK_GETTIME.

> +GIT_CHECK_FUNC(clock_gettime,
> +[HAVE_CLOCK_GETTIME=Yes],

For consistency with other build values: s/Yes/YesPlease/

> +[HAVE_CLOCK_GETTIME=])
> +GIT_CONF_SUBST([HAVE_CLOCK_GETTIME])
> +
> +AC_DEFUN([CLOCK_MONOTONIC_SRC], [
> +AC_LANG_PROGRAM([[
> +#include <time.h>
> +clockid_t id = CLOCK_MONOTONIC;
> +]], [])])
> +
> +#
> +# Define NO_CLOCK_MONOTONIC on really old systems that are still in production
> +# if you need GIT to compile but can't update the machine otherwise.

The comment talks about NO_CLOCK_MONOTONIC, however, the code names it
HAVE_CLOCK_MONOTONIC.

The "old systems .. need GIT to compile ... can't update" commentary
is unnecessary. That's pretty well implied by the mere use of
Autoconf. It would be sufficient to say "Define ... if you don't have
CLOCK_MONOTONIC."

> +AC_MSG_CHECKING([for CLOCK_MONOTONIC])
> +AC_COMPILE_IFELSE([CLOCK_MONOTONIC_SRC],
> +       [AC_MSG_RESULT([yes])
> +       HAVE_CLOCK_MONOTONIC=Yes],

Consistency: s/Yes/YesPlease/

> +       [AC_MSG_RESULT([no])
> +       HAVE_CLOCK_MONOTONIC=])
> +
> +GIT_CONF_SUBST([HAVE_CLOCK_MONOTONIC])

Be aware that this does not actually #define HAVE_CLOCK_MONOTONIC
anywhere. It only sets a Makefile variable in config.mak.autogen.
(Consequently, this patch it actually breaks support for this feature
in trace.c for people who build without running the configure script.)
You need to do extra work to get the #define.

In particular, add a comment to Makefile describing the
HAVE_CLOCK_MONOTONIC setting. (Placing it just below the existing
comment for HAVE_CLOCK_GETTIME would make loads of sense.)

Next, add code to the Makefile to actually #define
HAVE_CLOCK_MONOTONIC. Something like this (alongside the existing
HAVE_CLOCK_GETTIME code):

    ifdef HAVE_CLOCK_MONOTONIC
        BASIC_CFLAGS += -DHAVE_CLOCK_MONOTONIC
    endif

Finally, update config.make.uname to set
HAVE_CLOCK_MONOTONIC=YesPlease on platforms which are likely to have
it (for people who don't run the configure script). At the very least,
it should be enabled by default for Linux.

> +#
>  # Define NO_SETITIMER if you don't have setitimer.
>  GIT_CHECK_FUNC(setitimer,
>  [NO_SETITIMER=],
> diff --git a/trace.c b/trace.c
> index 4778608..bfbd48f 100644
> --- a/trace.c
> +++ b/trace.c
> @@ -324,7 +324,7 @@ int trace_want(struct trace_key *key)
>         return !!get_trace_fd(key);
>  }
>
> -#ifdef HAVE_CLOCK_GETTIME
> +#if defined(HAVE_CLOCK_GETTIME) && defined(HAVE_CLOCK_MONOTONIC)
>
>  static inline uint64_t highres_nanos(void)
>  {
> --
> 2.2.0.GIT

Re: [PATCH 3/3] configure.ac,imap-send.c: check HMAC_CTX_cleanup

[Eric Sunshine]

On Sun, Dec 21, 2014 at 1:53 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
> Older versions of OpenSSL have HMAC_cleanup, but not HMAC_CTX_cleanup.
> This change checks for both, uses HMAC_CTX_cleanup on platforms which
> have it, otherwise falls back to HMAC_cleanup.

On this project, imperative mood is preferred. Also, this _is_ a patch
(implying "change"), so it's not necessary to say "This change". An
alternate way to write the above might be:

    Check for both, and use HMAC_CTX_cleanup when available,
    otherwise HMAC_cleanup.

> This changes makes building GIT on older platforms less tedious.

This is implied by the first paragraph, so no need to state it here explicitly.

More below.

> ---
> diff --git a/Makefile b/Makefile
> index 7482a4d..a495d94 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1059,6 +1059,12 @@ ifndef NO_OPENSSL
>         ifdef NEEDS_CRYPTO_WITH_SSL
>                 OPENSSL_LIBSSL += -lcrypto
>         endif
> +       ifdef HAVE_HMAC_CTX_CLEANUP
> +               BASIC_CFLAGS += -DHAVE_HMAC_CTX_CLEANUP
> +       endif
> +       ifdef HAVE_HMAC_CLEANUP
> +               BASIC_CFLAGS += -DHAVE_HMAC_CLEANUP
> +       endif

See discussion below regarding build breakage due to this incomplete change.

>  else
>         BASIC_CFLAGS += -DNO_OPENSSL
>         BLK_SHA1 = 1
> diff --git a/configure.ac b/configure.ac
> index 3900044..b22788c 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -899,6 +899,7 @@ GIT_CONF_SUBST([SNPRINTF_RETURNS_BOGUS])
>  ## Checks for library functions.
>  ## (in default C library and libraries checked by AC_CHECK_LIB)
>  AC_MSG_NOTICE([CHECKS for library functions])
> +

Sneaking in unrelated whitespace change.

>  #
>  # Define NO_LIBGEN_H if you don't have libgen.h.
>  AC_CHECK_HEADER([libgen.h],
> @@ -932,6 +933,18 @@ AC_CHECK_LIB([iconv], [locale_charset],
>                       [CHARSET_LIB=-lcharset])])
>  GIT_CONF_SUBST([CHARSET_LIB])
>  #
> +# Define HAVE_HMAC_CTX_CLEANUP=Yes if we have the newer HMAC cleanup function

s/Yes/YesPease/
s/$/./

> +AC_CHECK_LIB([crypto], [HMAC_CTX_cleanup],
> +       [HAVE_HMAC_CTX_CLEANUP=Yes],
> +       [], [])

No need to pass trailing empty [] arguments explicitly in m4.

> +GIT_CONF_SUBST([HAVE_HMAC_CTX_CLEANUP])

You need to document this new variable in the comment block at the top
of Makefile so that people who build without running the configure
script will know that they may need to tweak it.

> +#
> +# Define HAVE_HMAC_CLEANUP=Yes if we have the older HMAC cleanup function

s/Yes/YesPease/
s/$/./

> +AC_CHECK_LIB([crypto], [HMAC_cleanup],
> +       [HAVE_HMAC_CLEANUP=Yes],
> +       [], [])
> +GIT_CONF_SUBST([HAVE_HMAC_CLEANUP])

This test for HMAC_cleanup() is likely overkill until we encounter a
system which has neither HMAC_CTX_cleanup() nor HMAC_cleanup(). For
the present, it's probably safe to assume that the older
HMAC_cleanup() is available everywhere when HMAC_CTX_cleanup() is not.

> +#
>  # Define NO_CLOCK_GETTIME if you don't have clock_gettime.
>  GIT_CHECK_FUNC(clock_gettime,
>  [HAVE_CLOCK_GETTIME=Yes],
> diff --git a/imap-send.c b/imap-send.c
> index 70bcc7a..eec2378 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -861,7 +861,13 @@ static char *cram(const char *challenge_64, const char *user, const char *pass)
>         HMAC_Init(&hmac, (unsigned char *)pass, strlen(pass), EVP_md5());
>         HMAC_Update(&hmac, (unsigned char *)challenge, decoded_len);
>         HMAC_Final(&hmac, hash, NULL);
> +#if defined(HAVE_HMAC_CTX_CLEANUP)
>         HMAC_CTX_cleanup(&hmac);
> +#elif defined(HAVE_HMAC_CLEANUP)
> +       HMAC_cleanup(&hmac);
> +#else
> +# error "no HMAC_cleanup function"
> +#endif

Several issues:

This change utterly breaks the build for people who do not run the
configure script since neither HAVE_HMAC_CTX_CLEANUP nor
HAVE_HMAC_CLEANUP Makefile variable is set (which means that the
corresponding C #defines are never made).

Somehow, you need to make sure that Makefile variable
HAVE_HMAC_CTX_CLEANUP is set by default since HMAC_CTX_cleanup() is
available on most platforms, and only a few much older ones lack it.
Typically, you would do this in config.make.uname, however, this is
one of those instances in which it is easier to disable a feature for
the rare case than to enable it for the common case. Therefore, it
would be more appropriate to name this variable NO_HMAC_CTX_CLEANUP
(and document it appropriately in Makefile).

Finally, rather than polluting imap-send.c with #if conditionals,
relegate the ugliness to git-compat-util.h. For instance, within the
'#ifndef NO_OPENSSL' conditional, you might add:

    #ifdef NO_HMAC_CTX_CLEANUP
    #define HMAC_CTX_cleanup HMAC_cleanup
    #endif

and then you don't have to touch imap-send.c at all.

>         hex[32] = 0;
>         for (i = 0; i < 16; i++) {
> --
> 2.2.0.GIT

Re: [PATCH 1/3] configure.ac: check tv_nsec field in struct stat

[Eric Sunshine]

On Sun, Dec 21, 2014 at 3:20 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Dec 21, 2014 at 1:53 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
>> +AC_CHECK_MEMBER([struct stat.st_mtim.tv_nsec],
>> +[HAVE_ST_MTIM=Yes],
>> +[HAVE_ST_MTIM=No],
>
> In Autoconf, it's customary to use lowercase values (such as "yes"
> rather than "Yes") for these temporary variables. A "no" value is
> usually represented by an empty assignment (HAVE_ST_MTIM=). However,
> AC_CHECK_MEMBER() assigns the test result automatically to a shell
> variable (in this case, named
> ac_cv_member_struct_stat_st_mtim_tv_nsec), so there is no need to
> invent your own (HAVE_ST_MTIM).
>
> So, you can reduce this to:
>
>    AC_CHECK_MEMBER([struct stat.st_mtim.tv_nsec])
>
> and later check the value with
>
>     test x$ac_cv_member_struct_stat_st_mtim_tv_nsec = yes

Apple auto-correction strikes again: s/yes/xyes/

Re: [PATCH 2/3] configure.ac,trace.c: check for CLOCK_MONOTONIC

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 871 bytes --]

On Sun, Dec 21, 2014 at 10:53:35AM -0800, Reuben Hawkins wrote:
> CLOCK_MONOTONIC isn't available on RHEL3, but there are still RHEL3
> systems being used in production.  This change makes compiling git
> less tedious on older platforms.

While I'm not opposed to this change, I expect that you'll find there's
lots of subtle breakage when trying to compile (and run the testsuite
for) git on RHEL 3.  I've spoken up before to prevent breakage on
RHEL/CentOS 5 (since $DAYJOB still supports it), but I'm not sure
anyone's looking out for something as old as RHEL 3.  I expect you'll
probably have some pain points with perl and curl, among others.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] configure.ac,trace.c: check for CLOCK_MONOTONIC

[Reuben Hawkins]

On Sun, Dec 21, 2014 at 8:12 PM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On Sun, Dec 21, 2014 at 10:53:35AM -0800, Reuben Hawkins wrote:
>> CLOCK_MONOTONIC isn't available on RHEL3, but there are still RHEL3
>> systems being used in production.  This change makes compiling git
>> less tedious on older platforms.
>
> While I'm not opposed to this change, I expect that you'll find there's
> lots of subtle breakage when trying to compile (and run the testsuite
> for) git on RHEL 3.  I've spoken up before to prevent breakage on
> RHEL/CentOS 5 (since $DAYJOB still supports it), but I'm not sure
> anyone's looking out for something as old as RHEL 3.  I expect you'll
> probably have some pain points with perl and curl, among others.

Yes, there are pain points with perl and curl.  Those I've disable
with other compile options when building on RHEL3, but reducing the
number of options I have to set manually and increasing the number of
automatic checks with configure is helpful.   Sometime over the next
few days I'll submit a v2 of the patches with Eric's comments taken
into account.

> --
> brian m. carlson / brian with sandals: Houston, Texas, US
> +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
> OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

v2 patches for fixes on RHEL3

[Reuben Hawkins]

These patches add a few autoconfig checks for nanosecond resolution fields in
struct stat, CLOCK_MONOTONIC, and HMAC_CTX_cleanup.  I'm fairly sure I've fixed
the concerns/doubts the first set of patched raised.

(I rarely use git-send-email so forgive me if I botch this)

[PATCH 1/3] configure.ac: check tv_nsec field in struct stat

[Reuben Hawkins]

This check will automatically set the correct NO_NSEC setting.
---
 configure.ac | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/configure.ac b/configure.ac
index 6af9647..dcc4bf0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -754,6 +754,18 @@ AC_CHECK_TYPES([struct itimerval],
 [#include <sys/time.h>])
 GIT_CONF_SUBST([NO_STRUCT_ITIMERVAL])
 #
+# Define USE_ST_TIMESPEC=YesPlease when stat.st_mtimespec.tv_nsec exist
+# Define NO_NSEC=YesPlease when neither stat.st_mtim.tv_nsec nor stat.st_mtimespec.tv_nsec exist
+AC_CHECK_MEMBER([struct stat.st_mtimespec.tv_nsec])
+AC_CHECK_MEMBER([struct stat.st_mtim.tv_nsec])
+if test x$ac_cv_member_struct_stat_st_mtimespec_tv_nsec = xyes ; then
+	USE_ST_TIMESPEC=YesPlease
+	GIT_CONF_SUBST([USE_ST_TIMESPEC])
+elif test x$ac_cv_member_struct_stat_st_mtim_tv_nsec != xyes ; then
+	NO_NSEC=YesPlease
+	GIT_CONF_SUBST([NO_NSEC])
+fi
+#
 # Define NO_D_INO_IN_DIRENT if you don't have d_ino in your struct dirent.
 AC_CHECK_MEMBER(struct dirent.d_ino,
 [NO_D_INO_IN_DIRENT=],
-- 
2.2.0.68.g8f72f0c.dirty

[PATCH 2/3] configure.ac: check for clock_gettime and CLOCK_MONOTONIC

[Reuben Hawkins]

CLOCK_MONOTONIC isn't available on RHEL3, but there are still RHEL3
systems being used in production.  This change makes compiling git
less tedious on older platforms without CLOCK_MONOTONIC.
---
 Makefile         |  4 ++++
 config.mak.uname |  1 +
 configure.ac     | 22 ++++++++++++++++++++++
 trace.c          |  2 +-
 4 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 7482a4d..af551a0 100644
--- a/Makefile
+++ b/Makefile
@@ -1382,6 +1382,10 @@ ifdef HAVE_CLOCK_GETTIME
 	EXTLIBS += -lrt
 endif
 
+ifdef HAVE_CLOCK_MONOTONIC
+	BASIC_CFLAGS += -DHAVE_CLOCK_MONOTONIC
+endif
+
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK = NoThanks
 endif
diff --git a/config.mak.uname b/config.mak.uname
index a2f380f..926773e 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -35,6 +35,7 @@ ifeq ($(uname_S),Linux)
 	LIBC_CONTAINS_LIBINTL = YesPlease
 	HAVE_DEV_TTY = YesPlease
 	HAVE_CLOCK_GETTIME = YesPlease
+	HAVE_CLOCK_MONOTONIC = YesPlease
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease
diff --git a/configure.ac b/configure.ac
index dcc4bf0..424dec5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -923,6 +923,28 @@ AC_CHECK_LIB([iconv], [locale_charset],
                      [CHARSET_LIB=-lcharset])])
 GIT_CONF_SUBST([CHARSET_LIB])
 #
+# Define HAVE_CLOCK_GETTIME=YesPlease if clock_gettime is available.
+GIT_CHECK_FUNC(clock_gettime,
+[HAVE_CLOCK_GETTIME=YesPlease],
+[HAVE_CLOCK_GETTIME=])
+GIT_CONF_SUBST([HAVE_CLOCK_GETTIME])
+
+AC_DEFUN([CLOCK_MONOTONIC_SRC], [
+AC_LANG_PROGRAM([[
+#include <time.h>
+clockid_t id = CLOCK_MONOTONIC;
+]], [])])
+
+#
+# Define HAVE_CLOCK_MONOTONIC=YesPlease if CLOCK_MONOTONIC is available.
+AC_MSG_CHECKING([for CLOCK_MONOTONIC])
+AC_COMPILE_IFELSE([CLOCK_MONOTONIC_SRC],
+	[AC_MSG_RESULT([yes])
+	HAVE_CLOCK_MONOTONIC=YesPlease],
+	[AC_MSG_RESULT([no])
+	HAVE_CLOCK_MONOTONIC=])
+GIT_CONF_SUBST([HAVE_CLOCK_MONOTONIC])
+#
 # Define NO_SETITIMER if you don't have setitimer.
 GIT_CHECK_FUNC(setitimer,
 [NO_SETITIMER=],
diff --git a/trace.c b/trace.c
index 4778608..bfbd48f 100644
--- a/trace.c
+++ b/trace.c
@@ -324,7 +324,7 @@ int trace_want(struct trace_key *key)
 	return !!get_trace_fd(key);
 }
 
-#ifdef HAVE_CLOCK_GETTIME
+#if defined(HAVE_CLOCK_GETTIME) && defined(HAVE_CLOCK_MONOTONIC)
 
 static inline uint64_t highres_nanos(void)
 {
-- 
2.2.0.68.g8f72f0c.dirty

[PATCH 3/3] configure.ac: check for HMAC_CTX_cleanup

[Reuben Hawkins]

OpenSSL version 0.9.6b and before defined the function HMAC_cleanup.
Newer versions define HMAC_CTX_cleanup.  Check for HMAC_CTX_cleanup and
fall back to HMAC_cleanup when the newer function is missing.
---
 Makefile          | 3 +++
 configure.ac      | 7 +++++++
 git-compat-util.h | 3 +++
 3 files changed, 13 insertions(+)

diff --git a/Makefile b/Makefile
index af551a0..d3c2b58 100644
--- a/Makefile
+++ b/Makefile
@@ -1059,6 +1059,9 @@ ifndef NO_OPENSSL
 	ifdef NEEDS_CRYPTO_WITH_SSL
 		OPENSSL_LIBSSL += -lcrypto
 	endif
+	ifdef NO_HMAC_CTX_CLEANUP
+		BASIC_CFLAGS += -DNO_HMAC_CTX_CLEANUP
+	endif
 else
 	BASIC_CFLAGS += -DNO_OPENSSL
 	BLK_SHA1 = 1
diff --git a/configure.ac b/configure.ac
index 424dec5..c282663 100644
--- a/configure.ac
+++ b/configure.ac
@@ -923,6 +923,13 @@ AC_CHECK_LIB([iconv], [locale_charset],
                      [CHARSET_LIB=-lcharset])])
 GIT_CONF_SUBST([CHARSET_LIB])
 #
+# Define NO_HMAC_CTX_CLEANUP=YesPlease if HMAC_CTX_cleanup is missing.
+AC_CHECK_LIB([crypto], [HMAC_CTX_cleanup],
+	[NO_HMAC_CTX_CLEANUP=],
+	[NO_HMAC_CTX_CLEANUP=YesPlease],
+	[])
+GIT_CONF_SUBST([NO_HMAC_CTX_CLEANUP])
+#
 # Define HAVE_CLOCK_GETTIME=YesPlease if clock_gettime is available.
 GIT_CHECK_FUNC(clock_gettime,
 [HAVE_CLOCK_GETTIME=YesPlease],
diff --git a/git-compat-util.h b/git-compat-util.h
index 400e921..2fdca2d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -213,6 +213,9 @@ extern char *gitbasename(char *);
 #ifndef NO_OPENSSL
 #include <openssl/ssl.h>
 #include <openssl/err.h>
+#ifdef NO_HMAC_CTX_CLEANUP
+#define HMAC_CTX_cleanup HMAC_cleanup
+#endif
 #endif
 
 /* On most systems <netdb.h> would have given us this, but
-- 
2.2.0.68.g8f72f0c.dirty

Re: [PATCH 1/3] configure.ac: check tv_nsec field in struct stat

[Eric Sunshine]

On Wed, Jan 7, 2015 at 3:23 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
> This check will automatically set the correct NO_NSEC setting.

This commit message neglects to mention the important point that
you're also now setting USE_ST_TIMESPEC when detected. You might
revise the message like this:

    Detect 'tv_nsec' field in 'struct stat' and set Makefile variable
    NO_NSEC appropriately.

    A side-effect of the above detection is that we also determine
    whether 'stat.st_mtimespec' is available, so, as a bonus, set the
    Makefile variable USE_ST_TIMESPEC, as well.

Also, your sign-off is missing (as mentioned in my previous review[1]).

[1]: http://article.gmane.org/gmane.comp.version-control.git/261626

> ---
>  configure.ac | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/configure.ac b/configure.ac
> index 6af9647..dcc4bf0 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -754,6 +754,18 @@ AC_CHECK_TYPES([struct itimerval],
>  [#include <sys/time.h>])
>  GIT_CONF_SUBST([NO_STRUCT_ITIMERVAL])
>  #
> +# Define USE_ST_TIMESPEC=YesPlease when stat.st_mtimespec.tv_nsec exist

It would be slightly more accurate to drop the ".tv_nsec" bit from this comment.

Also: s/exist/exists./

> +# Define NO_NSEC=YesPlease when neither stat.st_mtim.tv_nsec nor stat.st_mtimespec.tv_nsec exist

Perhaps wrap this long comment over two lines.

Also: s/exist/exist./

> +AC_CHECK_MEMBER([struct stat.st_mtimespec.tv_nsec])
> +AC_CHECK_MEMBER([struct stat.st_mtim.tv_nsec])
> +if test x$ac_cv_member_struct_stat_st_mtimespec_tv_nsec = xyes ; then
> +       USE_ST_TIMESPEC=YesPlease
> +       GIT_CONF_SUBST([USE_ST_TIMESPEC])
> +elif test x$ac_cv_member_struct_stat_st_mtim_tv_nsec != xyes ; then
> +       NO_NSEC=YesPlease
> +       GIT_CONF_SUBST([NO_NSEC])
> +fi
> +#
>  # Define NO_D_INO_IN_DIRENT if you don't have d_ino in your struct dirent.
>  AC_CHECK_MEMBER(struct dirent.d_ino,
>  [NO_D_INO_IN_DIRENT=],
> --
> 2.2.0.68.g8f72f0c.dirty

Re: [PATCH 1/3] configure.ac: check tv_nsec field in struct stat

[Reuben Hawkins]

On Wed, Jan 7, 2015 at 1:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jan 7, 2015 at 3:23 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
>> This check will automatically set the correct NO_NSEC setting.
>
> This commit message neglects to mention the important point that
> you're also now setting USE_ST_TIMESPEC when detected. You might
> revise the message like this:
>
>     Detect 'tv_nsec' field in 'struct stat' and set Makefile variable
>     NO_NSEC appropriately.
>
>     A side-effect of the above detection is that we also determine
>     whether 'stat.st_mtimespec' is available, so, as a bonus, set the
>     Makefile variable USE_ST_TIMESPEC, as well.
>
> Also, your sign-off is missing (as mentioned in my previous review[1]).
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/261626
>
>> ---
>>  configure.ac | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 6af9647..dcc4bf0 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -754,6 +754,18 @@ AC_CHECK_TYPES([struct itimerval],
>>  [#include <sys/time.h>])
>>  GIT_CONF_SUBST([NO_STRUCT_ITIMERVAL])
>>  #
>> +# Define USE_ST_TIMESPEC=YesPlease when stat.st_mtimespec.tv_nsec exist
>
> It would be slightly more accurate to drop the ".tv_nsec" bit from this comment.

The AC_CHECK_MEMBER is checking for st_mtimespec.tv_nsec.  If I drop
tv_nsec from the comment should I also drop it in the check?

I thought it was better to be very explicit because the code using the
check is using that .tv_nsec field...I figured the check may as well
do exactly what the code is doing...


>
> Also: s/exist/exists./
>
>> +# Define NO_NSEC=YesPlease when neither stat.st_mtim.tv_nsec nor stat.st_mtimespec.tv_nsec exist
>
> Perhaps wrap this long comment over two lines.
>
> Also: s/exist/exist./
>
>> +AC_CHECK_MEMBER([struct stat.st_mtimespec.tv_nsec])
>> +AC_CHECK_MEMBER([struct stat.st_mtim.tv_nsec])
>> +if test x$ac_cv_member_struct_stat_st_mtimespec_tv_nsec = xyes ; then
>> +       USE_ST_TIMESPEC=YesPlease
>> +       GIT_CONF_SUBST([USE_ST_TIMESPEC])
>> +elif test x$ac_cv_member_struct_stat_st_mtim_tv_nsec != xyes ; then
>> +       NO_NSEC=YesPlease
>> +       GIT_CONF_SUBST([NO_NSEC])
>> +fi
>> +#
>>  # Define NO_D_INO_IN_DIRENT if you don't have d_ino in your struct dirent.
>>  AC_CHECK_MEMBER(struct dirent.d_ino,
>>  [NO_D_INO_IN_DIRENT=],
>> --
>> 2.2.0.68.g8f72f0c.dirty

Re: [PATCH 2/3] configure.ac: check for clock_gettime and CLOCK_MONOTONIC

[Eric Sunshine]

On Wed, Jan 7, 2015 at 3:23 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
> CLOCK_MONOTONIC isn't available on RHEL3, but there are still RHEL3
> systems being used in production.  This change makes compiling git
> less tedious on older platforms without CLOCK_MONOTONIC.

The second sentence is implied by the very presence of this patch,
thus adds no value. I, personally, would drop it.

Also, your sign-off is missing (as mentioned in my previous review[1]).

> ---
> diff --git a/Makefile b/Makefile
> index 7482a4d..af551a0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1382,6 +1382,10 @@ ifdef HAVE_CLOCK_GETTIME
>         EXTLIBS += -lrt
>  endif
>
> +ifdef HAVE_CLOCK_MONOTONIC
> +       BASIC_CFLAGS += -DHAVE_CLOCK_MONOTONIC
> +endif

You need to document this new Makefile variable (HAVE_CLOCK_MONOTONIC)
at the top of Makefile (as mentioned in my previous review[1]), so
that people who build without running 'configure' will know that they
may need to tweak it.

>  ifeq ($(TCLTK_PATH),)
>  NO_TCLTK = NoThanks
>  endif
> diff --git a/configure.ac b/configure.ac
> index dcc4bf0..424dec5 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -923,6 +923,28 @@ AC_CHECK_LIB([iconv], [locale_charset],
>                       [CHARSET_LIB=-lcharset])])
>  GIT_CONF_SUBST([CHARSET_LIB])
>  #
> +# Define HAVE_CLOCK_GETTIME=YesPlease if clock_gettime is available.
> +GIT_CHECK_FUNC(clock_gettime,
> +[HAVE_CLOCK_GETTIME=YesPlease],
> +[HAVE_CLOCK_GETTIME=])
> +GIT_CONF_SUBST([HAVE_CLOCK_GETTIME])

You could simplify the above four lines to this one-liner:

    GIT_CHECK_FUNC(clock_gettime,
        GIT_CONF_SUBST([HAVE_CLOCK_GETTIME], [YesPlease]))

> +
> +AC_DEFUN([CLOCK_MONOTONIC_SRC], [
> +AC_LANG_PROGRAM([[
> +#include <time.h>
> +clockid_t id = CLOCK_MONOTONIC;
> +]], [])])

No need to pass empty trailing arguments in m4. It's customary to drop
them altogether (since they are implied).

> +#
> +# Define HAVE_CLOCK_MONOTONIC=YesPlease if CLOCK_MONOTONIC is available.
> +AC_MSG_CHECKING([for CLOCK_MONOTONIC])
> +AC_COMPILE_IFELSE([CLOCK_MONOTONIC_SRC],
> +       [AC_MSG_RESULT([yes])
> +       HAVE_CLOCK_MONOTONIC=YesPlease],
> +       [AC_MSG_RESULT([no])
> +       HAVE_CLOCK_MONOTONIC=])
> +GIT_CONF_SUBST([HAVE_CLOCK_MONOTONIC])

Ditto regarding simplification:

    AC_MSG_CHECKING([for CLOCK_MONOTONIC])
    AC_COMPILE_IFELSE([CLOCK_MONOTONIC_SRC],
        [AC_MSG_RESULT([yes])
        GIT_CONF_SUBST([HAVE_CLOCK_MONOTONIC], [YesPlease])],
        [AC_MSG_RESULT([no])])

> +#
>  # Define NO_SETITIMER if you don't have setitimer.
>  GIT_CHECK_FUNC(setitimer,
>  [NO_SETITIMER=],

[1]: http://article.gmane.org/gmane.comp.version-control.git/261630

Re: [PATCH 3/3] configure.ac: check for HMAC_CTX_cleanup

[Eric Sunshine]

On Wed, Jan 7, 2015 at 3:23 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
> OpenSSL version 0.9.6b and before defined the function HMAC_cleanup.
> Newer versions define HMAC_CTX_cleanup.  Check for HMAC_CTX_cleanup and
> fall back to HMAC_cleanup when the newer function is missing.

Missing sign-off.

Overall, these patches are nicely improved from the previous round. A
few more comments below...

> ---
>  Makefile          | 3 +++
>  configure.ac      | 7 +++++++
>  git-compat-util.h | 3 +++
>  3 files changed, 13 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index af551a0..d3c2b58 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1059,6 +1059,9 @@ ifndef NO_OPENSSL
>         ifdef NEEDS_CRYPTO_WITH_SSL
>                 OPENSSL_LIBSSL += -lcrypto
>         endif
> +       ifdef NO_HMAC_CTX_CLEANUP
> +               BASIC_CFLAGS += -DNO_HMAC_CTX_CLEANUP
> +       endif

You need to document this new Makefile variable (NO_HMAC_CTX_CLEANUP)
at the top of Makefile (as mentioned in my previous review[1]).

[1]: http://article.gmane.org/gmane.comp.version-control.git/261631

>  else
>         BASIC_CFLAGS += -DNO_OPENSSL
>         BLK_SHA1 = 1
> diff --git a/configure.ac b/configure.ac
> index 424dec5..c282663 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -923,6 +923,13 @@ AC_CHECK_LIB([iconv], [locale_charset],
>                       [CHARSET_LIB=-lcharset])])
>  GIT_CONF_SUBST([CHARSET_LIB])
>  #
> +# Define NO_HMAC_CTX_CLEANUP=YesPlease if HMAC_CTX_cleanup is missing.
> +AC_CHECK_LIB([crypto], [HMAC_CTX_cleanup],
> +       [NO_HMAC_CTX_CLEANUP=],
> +       [NO_HMAC_CTX_CLEANUP=YesPlease],
> +       [])
> +GIT_CONF_SUBST([NO_HMAC_CTX_CLEANUP])

It is customary to drop empty trailing arguments in m4.

Also, you can simplify this entire check to:

    AC_CHECK_LIB([crypto], [HMAC_CTX_cleanup],
        [], [GIT_CONF_SUBST([NO_HMAC_CTX_CLEANUP], [YesPlease])])

>  # Define HAVE_CLOCK_GETTIME=YesPlease if clock_gettime is available.
>  GIT_CHECK_FUNC(clock_gettime,
>  [HAVE_CLOCK_GETTIME=YesPlease],

Re: [PATCH 1/3] configure.ac: check tv_nsec field in struct stat

[Eric Sunshine]

On Wed, Jan 7, 2015 at 4:33 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
> On Wed, Jan 7, 2015 at 1:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Jan 7, 2015 at 3:23 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
>>> +# Define USE_ST_TIMESPEC=YesPlease when stat.st_mtimespec.tv_nsec exist
>>
>> It would be slightly more accurate to drop the ".tv_nsec" bit from this comment.
>
> The AC_CHECK_MEMBER is checking for st_mtimespec.tv_nsec.  If I drop
> tv_nsec from the comment should I also drop it in the check?

No. My observation was just about the comment.

> I thought it was better to be very explicit because the code using the
> check is using that .tv_nsec field...I figured the check may as well
> do exactly what the code is doing...

Indeed, the check and code should agree. However, from the perspective
of the person reading comment, the ".tv_nsec" is just an
implementation detail of the check itself. The final outcome (the
setting of USE_ST_TIMESPEC) is independent of how that check was made:
it matters only that 'stat.st_mtimespec' was detected _somehow_.

Anyhow, it's just a minor observation, hence my qualification of it as
"_slightly_ more accurate". If you feel strongly that it should remain
as is, then that's fine.

Re: [PATCH 1/3] configure.ac: check tv_nsec field in struct stat

[Reuben Hawkins]

On Wed, Jan 7, 2015 at 1:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jan 7, 2015 at 3:23 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
>> This check will automatically set the correct NO_NSEC setting.
>
> This commit message neglects to mention the important point that
> you're also now setting USE_ST_TIMESPEC when detected. You might
> revise the message like this:
>
>     Detect 'tv_nsec' field in 'struct stat' and set Makefile variable
>     NO_NSEC appropriately.
>
>     A side-effect of the above detection is that we also determine
>     whether 'stat.st_mtimespec' is available, so, as a bonus, set the
>     Makefile variable USE_ST_TIMESPEC, as well.

I see you're single quoted 'tv_nsec' and 'struct stat'.  Should I also
use single quotes in the first line of the commit msg like this...

configure.ac: check 'tv_nsec' field in 'struct stat'

?

>
> Also, your sign-off is missing (as mentioned in my previous review[1]).
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/261626
>
>> ---
>>  configure.ac | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 6af9647..dcc4bf0 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -754,6 +754,18 @@ AC_CHECK_TYPES([struct itimerval],
>>  [#include <sys/time.h>])
>>  GIT_CONF_SUBST([NO_STRUCT_ITIMERVAL])
>>  #
>> +# Define USE_ST_TIMESPEC=YesPlease when stat.st_mtimespec.tv_nsec exist
>
> It would be slightly more accurate to drop the ".tv_nsec" bit from this comment.
>
> Also: s/exist/exists./
>
>> +# Define NO_NSEC=YesPlease when neither stat.st_mtim.tv_nsec nor stat.st_mtimespec.tv_nsec exist
>
> Perhaps wrap this long comment over two lines.
>
> Also: s/exist/exist./
>
>> +AC_CHECK_MEMBER([struct stat.st_mtimespec.tv_nsec])
>> +AC_CHECK_MEMBER([struct stat.st_mtim.tv_nsec])
>> +if test x$ac_cv_member_struct_stat_st_mtimespec_tv_nsec = xyes ; then
>> +       USE_ST_TIMESPEC=YesPlease
>> +       GIT_CONF_SUBST([USE_ST_TIMESPEC])
>> +elif test x$ac_cv_member_struct_stat_st_mtim_tv_nsec != xyes ; then
>> +       NO_NSEC=YesPlease
>> +       GIT_CONF_SUBST([NO_NSEC])
>> +fi
>> +#
>>  # Define NO_D_INO_IN_DIRENT if you don't have d_ino in your struct dirent.
>>  AC_CHECK_MEMBER(struct dirent.d_ino,
>>  [NO_D_INO_IN_DIRENT=],
>> --
>> 2.2.0.68.g8f72f0c.dirty

Re: [PATCH 2/3] configure.ac: check for clock_gettime and CLOCK_MONOTONIC

[Reuben Hawkins]

On Wed, Jan 7, 2015 at 1:37 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jan 7, 2015 at 3:23 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
>> CLOCK_MONOTONIC isn't available on RHEL3, but there are still RHEL3
>> systems being used in production.  This change makes compiling git
>> less tedious on older platforms without CLOCK_MONOTONIC.
>
> The second sentence is implied by the very presence of this patch,
> thus adds no value. I, personally, would drop it.
>
> Also, your sign-off is missing (as mentioned in my previous review[1]).
>
>> ---
>> diff --git a/Makefile b/Makefile
>> index 7482a4d..af551a0 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1382,6 +1382,10 @@ ifdef HAVE_CLOCK_GETTIME
>>         EXTLIBS += -lrt
>>  endif
>>
>> +ifdef HAVE_CLOCK_MONOTONIC
>> +       BASIC_CFLAGS += -DHAVE_CLOCK_MONOTONIC
>> +endif
>
> You need to document this new Makefile variable (HAVE_CLOCK_MONOTONIC)
> at the top of Makefile (as mentioned in my previous review[1]), so
> that people who build without running 'configure' will know that they
> may need to tweak it.
>
>>  ifeq ($(TCLTK_PATH),)
>>  NO_TCLTK = NoThanks
>>  endif
>> diff --git a/configure.ac b/configure.ac
>> index dcc4bf0..424dec5 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -923,6 +923,28 @@ AC_CHECK_LIB([iconv], [locale_charset],
>>                       [CHARSET_LIB=-lcharset])])
>>  GIT_CONF_SUBST([CHARSET_LIB])
>>  #
>> +# Define HAVE_CLOCK_GETTIME=YesPlease if clock_gettime is available.
>> +GIT_CHECK_FUNC(clock_gettime,
>> +[HAVE_CLOCK_GETTIME=YesPlease],
>> +[HAVE_CLOCK_GETTIME=])
>> +GIT_CONF_SUBST([HAVE_CLOCK_GETTIME])
>
> You could simplify the above four lines to this one-liner:
>
>     GIT_CHECK_FUNC(clock_gettime,
>         GIT_CONF_SUBST([HAVE_CLOCK_GETTIME], [YesPlease]))
>
>> +
>> +AC_DEFUN([CLOCK_MONOTONIC_SRC], [
>> +AC_LANG_PROGRAM([[
>> +#include <time.h>
>> +clockid_t id = CLOCK_MONOTONIC;
>> +]], [])])
>
> No need to pass empty trailing arguments in m4. It's customary to drop
> them altogether (since they are implied).
>
>> +#
>> +# Define HAVE_CLOCK_MONOTONIC=YesPlease if CLOCK_MONOTONIC is available.
>> +AC_MSG_CHECKING([for CLOCK_MONOTONIC])
>> +AC_COMPILE_IFELSE([CLOCK_MONOTONIC_SRC],
>> +       [AC_MSG_RESULT([yes])
>> +       HAVE_CLOCK_MONOTONIC=YesPlease],
>> +       [AC_MSG_RESULT([no])
>> +       HAVE_CLOCK_MONOTONIC=])
>> +GIT_CONF_SUBST([HAVE_CLOCK_MONOTONIC])
>
> Ditto regarding simplification:
>
>     AC_MSG_CHECKING([for CLOCK_MONOTONIC])
>     AC_COMPILE_IFELSE([CLOCK_MONOTONIC_SRC],
>         [AC_MSG_RESULT([yes])
>         GIT_CONF_SUBST([HAVE_CLOCK_MONOTONIC], [YesPlease])],
>         [AC_MSG_RESULT([no])])

I *think* there's an issue with this simplification as used right
here.  In the 'no' case, HAVE_CLOCK_MONOTONIC *must* be undefined by
setting it equal to nothing

HAVE_CLOCK_MONOTONIC=

So that the setting in config.mak.uname 'HAVE_CLOCK_MONOTINIC =
YesPlease' will be overridden.

So this one needs to stay as is.

>
>> +#
>>  # Define NO_SETITIMER if you don't have setitimer.
>>  GIT_CHECK_FUNC(setitimer,
>>  [NO_SETITIMER=],
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/261630

Re: [PATCH 1/3] configure.ac: check tv_nsec field in struct stat

[Eric Sunshine]

On Wed, Jan 7, 2015 at 5:19 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
> On Wed, Jan 7, 2015 at 1:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Jan 7, 2015 at 3:23 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
>>> This check will automatically set the correct NO_NSEC setting.
>>
>> This commit message neglects to mention the important point that
>> you're also now setting USE_ST_TIMESPEC when detected. You might
>> revise the message like this:
>>
>>     Detect 'tv_nsec' field in 'struct stat' and set Makefile variable
>>     NO_NSEC appropriately.
>>
>>     A side-effect of the above detection is that we also determine
>>     whether 'stat.st_mtimespec' is available, so, as a bonus, set the
>>     Makefile variable USE_ST_TIMESPEC, as well.
>
> I see you're single quoted 'tv_nsec' and 'struct stat'.  Should I also
> use single quotes in the first line of the commit msg like this...
>
> configure.ac: check 'tv_nsec' field in 'struct stat'

Quoting them was just my personal taste, however, consistency of
formatting between subject and the body of the message would be nice.
Use whatever seems correct to you.

Re: [PATCH 2/3] configure.ac: check for clock_gettime and CLOCK_MONOTONIC

[Eric Sunshine]

On Wed, Jan 7, 2015 at 5:31 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
> On Wed, Jan 7, 2015 at 1:37 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Jan 7, 2015 at 3:23 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
>>> +GIT_CHECK_FUNC(clock_gettime,
>>> +[HAVE_CLOCK_GETTIME=YesPlease],
>>> +[HAVE_CLOCK_GETTIME=])
>>> +GIT_CONF_SUBST([HAVE_CLOCK_GETTIME])
>>
>> You could simplify the above four lines to this one-liner:
>>
>>     GIT_CHECK_FUNC(clock_gettime,
>>         GIT_CONF_SUBST([HAVE_CLOCK_GETTIME], [YesPlease]))
>>
>>> +AC_MSG_CHECKING([for CLOCK_MONOTONIC])
>>> +AC_COMPILE_IFELSE([CLOCK_MONOTONIC_SRC],
>>> +       [AC_MSG_RESULT([yes])
>>> +       HAVE_CLOCK_MONOTONIC=YesPlease],
>>> +       [AC_MSG_RESULT([no])
>>> +       HAVE_CLOCK_MONOTONIC=])
>>> +GIT_CONF_SUBST([HAVE_CLOCK_MONOTONIC])
>>
>> Ditto regarding simplification:
>>
>>     AC_MSG_CHECKING([for CLOCK_MONOTONIC])
>>     AC_COMPILE_IFELSE([CLOCK_MONOTONIC_SRC],
>>         [AC_MSG_RESULT([yes])
>>         GIT_CONF_SUBST([HAVE_CLOCK_MONOTONIC], [YesPlease])],
>>         [AC_MSG_RESULT([no])])
>
> I *think* there's an issue with this simplification as used right
> here.  In the 'no' case, HAVE_CLOCK_MONOTONIC *must* be undefined by
> setting it equal to nothing
>
> HAVE_CLOCK_MONOTONIC=
>
> So that the setting in config.mak.uname 'HAVE_CLOCK_MONOTINIC =
> YesPlease' will be overridden.
>
> So this one needs to stay as is.

Yes, you're right. That means that the HAVE_CLOCK_GETTIME
simplification also suffers the same shortcoming. So, neither
simplification is appropriate in this instance. Sorry for the noise.

[PATCH 3/3] configure.ac: check for HMAC_CTX_cleanup

[Reuben Hawkins]

OpenSSL version 0.9.6b and before defined the function HMAC_cleanup.
Newer versions define HMAC_CTX_cleanup.  Check for HMAC_CTX_cleanup and
fall back to HMAC_cleanup when the newer function is missing.

Signed-off-by: Reuben Hawkins <reubenhwk@gmail.com>
---
 Makefile          | 6 ++++++
 configure.ac      | 4 ++++
 git-compat-util.h | 3 +++
 3 files changed, 13 insertions(+)

diff --git a/Makefile b/Makefile
index 57e33f2..2ce1f1f 100644
--- a/Makefile
+++ b/Makefile
@@ -341,6 +341,9 @@ all::
 # Define HAVE_CLOCK_GETTIME if your platform has clock_gettime in librt.
 #
 # Define HAVE_CLOCK_MONOTONIC if your platform has CLOCK_MONOTONIC in librt.
+#
+# Define NO_HMAC_CTX_CLEANUP if your OpenSSL is version 0.9.6b or earlier to
+# cleanup the HMAC context with the older HMAC_cleanup function.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1061,6 +1064,9 @@ ifndef NO_OPENSSL
 	ifdef NEEDS_CRYPTO_WITH_SSL
 		OPENSSL_LIBSSL += -lcrypto
 	endif
+	ifdef NO_HMAC_CTX_CLEANUP
+		BASIC_CFLAGS += -DNO_HMAC_CTX_CLEANUP
+	endif
 else
 	BASIC_CFLAGS += -DNO_OPENSSL
 	BLK_SHA1 = 1
diff --git a/configure.ac b/configure.ac
index c3293b9..9c66c3e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -924,6 +924,10 @@ AC_CHECK_LIB([iconv], [locale_charset],
                      [CHARSET_LIB=-lcharset])])
 GIT_CONF_SUBST([CHARSET_LIB])
 #
+# Define NO_HMAC_CTX_CLEANUP=YesPlease if HMAC_CTX_cleanup is missing.
+AC_CHECK_LIB([crypto], [HMAC_CTX_cleanup],
+	[], [GIT_CONF_SUBST([NO_HMAC_CTX_CLEANUP], [YesPlease])])
+#
 # Define HAVE_CLOCK_GETTIME=YesPlease if clock_gettime is available.
 GIT_CHECK_FUNC(clock_gettime,
 	[HAVE_CLOCK_GETTIME=YesPlease],
diff --git a/git-compat-util.h b/git-compat-util.h
index 400e921..2fdca2d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -213,6 +213,9 @@ extern char *gitbasename(char *);
 #ifndef NO_OPENSSL
 #include <openssl/ssl.h>
 #include <openssl/err.h>
+#ifdef NO_HMAC_CTX_CLEANUP
+#define HMAC_CTX_cleanup HMAC_cleanup
+#endif
 #endif
 
 /* On most systems <netdb.h> would have given us this, but
-- 
2.2.0.68.g8f72f0c.dirty

Re: [PATCH 3/3] configure.ac: check for HMAC_CTX_cleanup

[Eric Sunshine]

On Thu, Jan 8, 2015 at 3:00 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
> OpenSSL version 0.9.6b and before defined the function HMAC_cleanup.
> Newer versions define HMAC_CTX_cleanup.  Check for HMAC_CTX_cleanup and
> fall back to HMAC_cleanup when the newer function is missing.
>
> Signed-off-by: Reuben Hawkins <reubenhwk@gmail.com>

Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

Note that this patch has a minor and simple-to-resolve conflict with
b195aa00c1 (git-compat-util: suppress unavoidable Apple-specific
deprecation warnings; 2014-12-16) which was just promoted to master.
Junio may resolve it locally or ask you to re-send.

> ---
> diff --git a/Makefile b/Makefile
> index 57e33f2..2ce1f1f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -341,6 +341,9 @@ all::
>  # Define HAVE_CLOCK_GETTIME if your platform has clock_gettime in librt.
>  #
>  # Define HAVE_CLOCK_MONOTONIC if your platform has CLOCK_MONOTONIC in librt.
> +#
> +# Define NO_HMAC_CTX_CLEANUP if your OpenSSL is version 0.9.6b or earlier to
> +# cleanup the HMAC context with the older HMAC_cleanup function.
>
>  GIT-VERSION-FILE: FORCE
>         @$(SHELL_PATH) ./GIT-VERSION-GEN
> @@ -1061,6 +1064,9 @@ ifndef NO_OPENSSL
>         ifdef NEEDS_CRYPTO_WITH_SSL
>                 OPENSSL_LIBSSL += -lcrypto
>         endif
> +       ifdef NO_HMAC_CTX_CLEANUP
> +               BASIC_CFLAGS += -DNO_HMAC_CTX_CLEANUP
> +       endif
>  else
>         BASIC_CFLAGS += -DNO_OPENSSL
>         BLK_SHA1 = 1
> diff --git a/configure.ac b/configure.ac
> index c3293b9..9c66c3e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -924,6 +924,10 @@ AC_CHECK_LIB([iconv], [locale_charset],
>                       [CHARSET_LIB=-lcharset])])
>  GIT_CONF_SUBST([CHARSET_LIB])
>  #
> +# Define NO_HMAC_CTX_CLEANUP=YesPlease if HMAC_CTX_cleanup is missing.
> +AC_CHECK_LIB([crypto], [HMAC_CTX_cleanup],
> +       [], [GIT_CONF_SUBST([NO_HMAC_CTX_CLEANUP], [YesPlease])])
> +#
>  # Define HAVE_CLOCK_GETTIME=YesPlease if clock_gettime is available.
>  GIT_CHECK_FUNC(clock_gettime,
>         [HAVE_CLOCK_GETTIME=YesPlease],
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 400e921..2fdca2d 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -213,6 +213,9 @@ extern char *gitbasename(char *);
>  #ifndef NO_OPENSSL
>  #include <openssl/ssl.h>
>  #include <openssl/err.h>
> +#ifdef NO_HMAC_CTX_CLEANUP
> +#define HMAC_CTX_cleanup HMAC_cleanup
> +#endif
>  #endif
>
>  /* On most systems <netdb.h> would have given us this, but
> --
> 2.2.0.68.g8f72f0c.dirty