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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 21, 2014, 20:20 UTC
Message-ID: <CAPig+cQWpq12axtJNsjoiKLKTpi6_xreS1fYDXYBtNv0Bj=T2Q@mail.gmail.com>
In-Reply-To: <1419188016-26134-1-git-send-email-reubenhwk@gmail.com>

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.

Show 10 quoted lines

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

Show 7 quoted lines

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

Show 8 quoted lines

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