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

From: Reuben Hawkins <reubenhwk@gmail.com>
Date: Jan 7, 2015, 22:19 UTC
Message-ID: <CAD_8n+RkCWzz0VSPb+3XkR-qbms1csGBbFRJtc-HwMp823QdCQ@mail.gmail.com>
In-Reply-To: <CAPig+cSrht0fovEWhEknxPPzwZhA7vzeLcM+omaM7PR752GfGw@mail.gmail.com>

On Wed, Jan 7, 2015 at 1:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 13 quoted lines

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

Show 44 quoted lines

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