[PATCH] compat: convert modes to use portable file type values

[PATCH] compat: convert modes to use portable file type values

[David Michael]

This adds simple wrapper functions around calls to stat(), fstat(),
and lstat() that translate the operating system's native file type
bits to those used by most operating systems.  It also rewrites the
S_IF* macros to the common values, so all file type processing is
performed using the translated modes.  This makes projects portable
across operating systems that use different file type definitions.

Only the file type bits may be affected by these compatibility
functions; the file permission bits are assumed to be 07777 and are
passed through unchanged.

Signed-off-by: David Michael <fedora.dm0@gmail.com>
---


Hi,

This is my most recent attempt at solving the problem of z/OS using
different file type values than every other OS.  I believe it should be
safe as long as the file type bits don't ever need to be converted back
to their native values (and I didn't see any instances of that).

I've been testing it by making commits to the same repositories on
different operating systems and pushing those changes around, and so far
there have been no issues.

Can anyone foresee any problems with this method?

Thanks.

David


 Makefile          |  8 ++++++++
 cache.h           |  7 -------
 compat/stat.c     | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 configure.ac      | 22 ++++++++++++++++++++++
 git-compat-util.h | 32 ++++++++++++++++++++++++++++++++
 5 files changed, 111 insertions(+), 7 deletions(-)
 create mode 100644 compat/stat.c

diff --git a/Makefile b/Makefile
index 827006b..cba3be1 100644
--- a/Makefile
+++ b/Makefile
@@ -191,6 +191,10 @@ all::
 # Define NO_TRUSTABLE_FILEMODE if your filesystem may claim to support
 # the executable mode bit, but doesn't really do so.
 #
+# Define NEEDS_MODE_TRANSLATION if your OS strays from the typical file type
+# bits in mode values (e.g. z/OS defines I_SFMT to 0xFF000000 as opposed to the
+# usual 0xF000).
+#
 # Define NO_IPV6 if you lack IPv6 support and getaddrinfo().
 #
 # Define NO_UNIX_SOCKETS if your system does not offer unix sockets.
@@ -1230,6 +1234,10 @@ endif
 ifdef NO_TRUSTABLE_FILEMODE
 	BASIC_CFLAGS += -DNO_TRUSTABLE_FILEMODE
 endif
+ifdef NEEDS_MODE_TRANSLATION
+	COMPAT_CFLAGS += -DNEEDS_MODE_TRANSLATION
+	COMPAT_OBJS += compat/stat.o
+endif
 ifdef NO_IPV6
 	BASIC_CFLAGS += -DNO_IPV6
 endif
diff --git a/cache.h b/cache.h
index 99ed096..f8174fe 100644
--- a/cache.h
+++ b/cache.h
@@ -65,13 +65,6 @@ unsigned long git_deflate_bound(git_zstream *, unsigned long);
  *
  * The value 0160000 is not normally a valid mode, and
  * also just happens to be S_IFDIR + S_IFLNK
- *
- * NOTE! We *really* shouldn't depend on the S_IFxxx macros
- * always having the same values everywhere. We should use
- * our internal git values for these things, and then we can
- * translate that to the OS-specific value. It just so
- * happens that everybody shares the same bit representation
- * in the UNIX world (and apparently wider too..)
  */
 #define S_IFGITLINK	0160000
 #define S_ISGITLINK(m)	(((m) & S_IFMT) == S_IFGITLINK)
diff --git a/compat/stat.c b/compat/stat.c
new file mode 100644
index 0000000..0ff1f2f
--- /dev/null
+++ b/compat/stat.c
@@ -0,0 +1,49 @@
+#define _POSIX_SOURCE
+#include <stddef.h>    /* NULL         */
+#include <sys/stat.h>  /* *stat, S_IS* */
+#include <sys/types.h> /* mode_t       */
+
+static inline mode_t mode_native_to_git(mode_t native_mode)
+{
+	if (S_ISREG(native_mode))
+		return 0100000 | (native_mode & 07777);
+	else if (S_ISDIR(native_mode))
+		return 0040000 | (native_mode & 07777);
+	else if (S_ISLNK(native_mode))
+		return 0120000 | (native_mode & 07777);
+	else if (S_ISBLK(native_mode))
+		return 0060000 | (native_mode & 07777);
+	else if (S_ISCHR(native_mode))
+		return 0020000 | (native_mode & 07777);
+	else if (S_ISFIFO(native_mode))
+		return 0010000 | (native_mode & 07777);
+	else /* Non-standard type bits were given. */
+		return native_mode & 07777;
+}
+
+int git_stat(const char *path, struct stat *buf)
+{
+	int rc;
+	rc = stat(path, buf);
+	if (buf != NULL)
+		buf->st_mode = mode_native_to_git(buf->st_mode);
+	return rc;
+}
+
+int git_fstat(int fd, struct stat *buf)
+{
+	int rc;
+	rc = fstat(fd, buf);
+	if (buf != NULL)
+		buf->st_mode = mode_native_to_git(buf->st_mode);
+	return rc;
+}
+
+int git_lstat(const char *path, struct stat *buf)
+{
+	int rc;
+	rc = lstat(path, buf);
+	if (buf != NULL)
+		buf->st_mode = mode_native_to_git(buf->st_mode);
+	return rc;
+}
diff --git a/configure.ac b/configure.ac
index 6af9647..b8eced4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -873,6 +873,28 @@ else
 	SNPRINTF_RETURNS_BOGUS=
 fi
 GIT_CONF_SUBST([SNPRINTF_RETURNS_BOGUS])
+#
+# Define NEEDS_MODE_TRANSLATION if your OS strays from the typical file type
+# bits in mode values.
+AC_CACHE_CHECK([whether the platform uses typical file type bits],
+ [ac_cv_sane_mode_bits], [
+AC_EGREP_CPP(yippeeyeswehaveit,
+	AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT],
+[#if S_IFMT == 0170000 && \
+	S_IFREG == 0100000 && S_IFDIR == 0040000 && S_IFLNK == 0120000 && \
+	S_IFBLK == 0060000 && S_IFCHR == 0020000 && S_IFIFO == 0010000
+yippeeyeswehaveit
+#endif
+]),
+	[ac_cv_sane_mode_bits=yes],
+	[ac_cv_sane_mode_bits=no])
+])
+if test $ac_cv_sane_mode_bits = yes; then
+	NEEDS_MODE_TRANSLATION=
+else
+	NEEDS_MODE_TRANSLATION=UnfortunatelyYes
+fi
+GIT_CONF_SUBST([NEEDS_MODE_TRANSLATION])
 
 
 ## Checks for library functions.
diff --git a/git-compat-util.h b/git-compat-util.h
index 400e921..48f6910 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -474,6 +474,38 @@ extern int git_munmap(void *start, size_t length);
 #define on_disk_bytes(st) ((st).st_blocks * 512)
 #endif
 
+#ifdef NEEDS_MODE_TRANSLATION
+#undef S_IFMT
+#undef S_IFREG
+#undef S_IFDIR
+#undef S_IFLNK
+#undef S_IFBLK
+#undef S_IFCHR
+#undef S_IFIFO
+#define S_IFMT  0170000
+#define S_IFREG 0100000
+#define S_IFDIR 0040000
+#define S_IFLNK 0120000
+#define S_IFBLK 0060000
+#define S_IFCHR 0020000
+#define S_IFIFO 0010000
+#ifdef stat
+#undef stat
+#endif
+#define stat(path, buf) git_stat(path, buf)
+extern int git_stat(const char *, struct stat *);
+#ifdef fstat
+#undef fstat
+#endif
+#define fstat(fd, buf) git_fstat(fd, buf)
+extern int git_fstat(int, struct stat *);
+#ifdef lstat
+#undef lstat
+#endif
+#define lstat(path, buf) git_lstat(path, buf)
+extern int git_lstat(const char *, struct stat *);
+#endif
+
 #define DEFAULT_PACKED_GIT_LIMIT \
 	((1024L * 1024L) * (sizeof(void*) >= 8 ? 8192 : 256))
 
-- 
1.9.3

Re: [PATCH] compat: convert modes to use portable file type values

[Torsten Bögershausen]

On 2014-11-30 03.41, David Michael wrote:
Some minor comments:
> +static inline mode_t mode_native_to_git(mode_t native_mode)
> +{
> +	if (S_ISREG(native_mode))
> +		return 0100000 | (native_mode & 07777);
> +	else if (S_ISDIR(native_mode))
> +		return 0040000 | (native_mode & 07777);
> +	else if (S_ISLNK(native_mode))
> +		return 0120000 | (native_mode & 07777);
> +	else if (S_ISBLK(native_mode))
> +		return 0060000 | (native_mode & 07777);
> +	else if (S_ISCHR(native_mode))
> +		return 0020000 | (native_mode & 07777);
> +	else if (S_ISFIFO(native_mode))
> +		return 0010000 | (native_mode & 07777);
> +	else /* Non-standard type bits were given. */
> +		return native_mode & 07777;
> +}
Could the code be more human-readable ?
static inline mode_t mode_native_to_git(mode_t native_mode)
{
	int perm_bits = native_mode & 07777;
	if (S_ISREG(native_mode))
		return 0100000 | perm_bits;
	if (S_ISDIR(native_mode))
		return 0040000 | perm_bits;
	if (S_ISLNK(native_mode))
		return 0120000 | perm_bits;
	if (S_ISBLK(native_mode))
		return 0060000 | perm_bits;
	if (S_ISCHR(native_mode))
		return 0020000 | perm_bits;
	if (S_ISFIFO(native_mode))
		return 0010000 | perm_bits;
	/* Non-standard type bits were given. */
	/* Shouldn't we die() here ?? */
		return perm_bits;
}

Re: [PATCH] compat: convert modes to use portable file type values

[Junio C Hamano]

David Michael <fedora.dm0@gmail.com> writes:

> This is my most recent attempt at solving the problem of z/OS using
> different file type values than every other OS.  I believe it should be
> safe as long as the file type bits don't ever need to be converted back
> to their native values (and I didn't see any instances of that).
>
> I've been testing it by making commits to the same repositories on
> different operating systems and pushing those changes around, and so far
> there have been no issues.
>
> Can anyone foresee any problems with this method?

I cannot offhand comment on the last question above, but the
reliance on exact S_IFxxx bit assignment was identified as a
potential problem from very early days of Git that we have known
about but didn't have need to address on any system that mattered.

This is a long overdue issue and I am happy to see it getting
tackled.  The patch seems to be a sensible implementation of your
design decision to use the one-way conversion.

> diff --git a/compat/stat.c b/compat/stat.c
> new file mode 100644
> index 0000000..0ff1f2f
> --- /dev/null
> +++ b/compat/stat.c
> @@ -0,0 +1,49 @@
> +#define _POSIX_SOURCE
> +#include <stddef.h>    /* NULL         */
> +#include <sys/stat.h>  /* *stat, S_IS* */
> +#include <sys/types.h> /* mode_t       */
> +
> +static inline mode_t mode_native_to_git(mode_t native_mode)
> +{
> +	if (S_ISREG(native_mode))
> +		return 0100000 | (native_mode & 07777);
> +	else if (S_ISDIR(native_mode))
> +		return 0040000 | (native_mode & 07777);
> +	else if (S_ISLNK(native_mode))
> +		return 0120000 | (native_mode & 07777);
> +	else if (S_ISBLK(native_mode))
> +		return 0060000 | (native_mode & 07777);
> +	else if (S_ISCHR(native_mode))
> +		return 0020000 | (native_mode & 07777);
> +	else if (S_ISFIFO(native_mode))
> +		return 0010000 | (native_mode & 07777);
> +	else /* Non-standard type bits were given. */
> +		return native_mode & 07777;
> +}

Re: [PATCH] compat: convert modes to use portable file type values

[David Michael]

On Sun, Nov 30, 2014 at 3:16 PM, Torsten Bögershausen <tboegi@web.de> wrote:
[snip]
> Could the code be more human-readable ?
> static inline mode_t mode_native_to_git(mode_t native_mode)
> {
>         int perm_bits = native_mode & 07777;
>         if (S_ISREG(native_mode))
>                 return 0100000 | perm_bits;
>         if (S_ISDIR(native_mode))
>                 return 0040000 | perm_bits;
>         if (S_ISLNK(native_mode))
>                 return 0120000 | perm_bits;
>         if (S_ISBLK(native_mode))
>                 return 0060000 | perm_bits;
>         if (S_ISCHR(native_mode))
>                 return 0020000 | perm_bits;
>         if (S_ISFIFO(native_mode))
>                 return 0010000 | perm_bits;
>         /* Non-standard type bits were given. */
>         /* Shouldn't we die() here ?? */
>                 return perm_bits;
> }

Sure, I can send an updated patch with the new variable and without the "else"s.

Regarding the question in the last comment:  I was assuming if this
case was ever reached, Git would handle the returned mode the same way
as if it encountered an unknown/non-standard file type on a normal
operating system, which could die() if needed in the function that
called stat().

Should I send an updated patch that die()s there?

David

Re: [PATCH] compat: convert modes to use portable file type values

[Torsten Bögershausen]

On 12/01/2014 04:40 AM, David Michael wrote:
> On Sun, Nov 30, 2014 at 3:16 PM, Torsten Bögershausen <tboegi@web.de> wrote:
> [snip]
>> Could the code be more human-readable ?
>> static inline mode_t mode_native_to_git(mode_t native_mode)
>> {
>>          int perm_bits = native_mode & 07777;
>>          if (S_ISREG(native_mode))
>>                  return 0100000 | perm_bits;
>>          if (S_ISDIR(native_mode))
>>                  return 0040000 | perm_bits;
>>          if (S_ISLNK(native_mode))
>>                  return 0120000 | perm_bits;
>>          if (S_ISBLK(native_mode))
>>                  return 0060000 | perm_bits;
>>          if (S_ISCHR(native_mode))
>>                  return 0020000 | perm_bits;
>>          if (S_ISFIFO(native_mode))
>>                  return 0010000 | perm_bits;
>>          /* Non-standard type bits were given. */
>>          /* Shouldn't we die() here ?? */
>>                  return perm_bits;
>> }
> Sure, I can send an updated patch with the new variable and without the "else"s.
>
> Regarding the question in the last comment:  I was assuming if this
> case was ever reached, Git would handle the returned mode the same way
> as if it encountered an unknown/non-standard file type on a normal
> operating system, which could die() if needed in the function that
> called stat().
>
> Should I send an updated patch that die()s there?
>
> David
Not yet, please wait with a V2 patch until I finished my thinking ;-)

I take back the suggestion with the die(). I was thinking how to handle
unforeseen types, which may show up on the z/OS some day,
So die() is not a good idea, it is better to ignore them, what the code 
does.

Knowing that Git does not track block devices, nor character devices nor sockets,
the above code could be simplyfied even more, by mapping everything which
is not a directory, a file or a softlink to "device type 0)

This is just a suggestion, I want to here from others as well:

         int perm_bits = native_mode & 07777;
         if (S_ISREG(native_mode))
                 return 0100000 | perm_bits;
         if (S_ISDIR(native_mode))
                 return 0040000 | perm_bits;
         if (S_ISLNK(native_mode))
                 return 0120000 | perm_bits;
         /* Git does not track S_IFCHR, S_IFBLK, S_IFIFO, S_IFSOCK  */
                 return perm_bits;

Re: [PATCH] compat: convert modes to use portable file type values

[David Michael]

On Mon, Dec 1, 2014 at 12:55 AM, Torsten Bögershausen <tboegi@web.de> wrote:
> On 12/01/2014 04:40 AM, David Michael wrote:
>>
>> On Sun, Nov 30, 2014 at 3:16 PM, Torsten Bögershausen <tboegi@web.de>
>> wrote:
>> [snip]
>>>
>>> Could the code be more human-readable ?
>>> static inline mode_t mode_native_to_git(mode_t native_mode)
>>> {
>>>          int perm_bits = native_mode & 07777;
>>>          if (S_ISREG(native_mode))
>>>                  return 0100000 | perm_bits;
>>>          if (S_ISDIR(native_mode))
>>>                  return 0040000 | perm_bits;
>>>          if (S_ISLNK(native_mode))
>>>                  return 0120000 | perm_bits;
>>>          if (S_ISBLK(native_mode))
>>>                  return 0060000 | perm_bits;
>>>          if (S_ISCHR(native_mode))
>>>                  return 0020000 | perm_bits;
>>>          if (S_ISFIFO(native_mode))
>>>                  return 0010000 | perm_bits;
>>>          /* Non-standard type bits were given. */
>>>          /* Shouldn't we die() here ?? */
>>>                  return perm_bits;
>>> }
>>
>> Sure, I can send an updated patch with the new variable and without the
>> "else"s.
>>
>> Regarding the question in the last comment:  I was assuming if this
>> case was ever reached, Git would handle the returned mode the same way
>> as if it encountered an unknown/non-standard file type on a normal
>> operating system, which could die() if needed in the function that
>> called stat().
>>
>> Should I send an updated patch that die()s there?
>>
>> David
>
> Not yet, please wait with a V2 patch until I finished my thinking ;-)
>
> I take back the suggestion with the die(). I was thinking how to handle
> unforeseen types, which may show up on the z/OS some day,
> So die() is not a good idea, it is better to ignore them, what the code
> does.
>
> Knowing that Git does not track block devices, nor character devices nor
> sockets,
> the above code could be simplyfied even more, by mapping everything which
> is not a directory, a file or a softlink to "device type 0)
>
> This is just a suggestion, I want to here from others as well:
>
>         int perm_bits = native_mode & 07777;
>         if (S_ISREG(native_mode))
>                 return 0100000 | perm_bits;
>         if (S_ISDIR(native_mode))
>                 return 0040000 | perm_bits;
>         if (S_ISLNK(native_mode))
>                 return 0120000 | perm_bits;
>         /* Git does not track S_IFCHR, S_IFBLK, S_IFIFO, S_IFSOCK  */
>                 return perm_bits;

I had considered omitting those three as well at first, but in this
case it would mean that they will be unusable all together.

The z/OS S_IFMT definition (i.e. the file type bit mask) is
0xFF000000, and the common/translated S_IFMT definition is 0xF000.
Since the S_ISxxx macros use the typical ((mode & S_IFMT) == S_IFxxx)
definition, they would never match a native z/OS mode after redefining
S_IFMT.

So translating those types isn't just for tracking files, it's to
support any use of S_ISxxx anywhere in the code.  It should be okay to
remove any of those types if we know that Git will never need to use
them.

David

Re: [PATCH] compat: convert modes to use portable file type values

[Duy Nguyen]

On Sun, Nov 30, 2014 at 9:41 AM, David Michael <fedora.dm0@gmail.com> wrote:
> +int git_stat(const char *path, struct stat *buf)
> +{
> +       int rc;
> +       rc = stat(path, buf);
> +       if (buf != NULL)

It's a minor thing, but maybe test "!rc" instead of "buf != NULL"?

> +               buf->st_mode = mode_native_to_git(buf->st_mode);
> +       return rc;
> +}
-- 
Duy

Re: [PATCH] compat: convert modes to use portable file type values

[David Michael]

On Mon, Dec 1, 2014 at 7:48 AM, David Michael <fedora.dm0@gmail.com> wrote:
> On Mon, Dec 1, 2014 at 12:55 AM, Torsten Bögershausen <tboegi@web.de> wrote:
>> On 12/01/2014 04:40 AM, David Michael wrote:
>>>
>>> On Sun, Nov 30, 2014 at 3:16 PM, Torsten Bögershausen <tboegi@web.de>
>>> wrote:
>>> [snip]
>>>>
>>>> Could the code be more human-readable ?
>>>> static inline mode_t mode_native_to_git(mode_t native_mode)
>>>> {
>>>>          int perm_bits = native_mode & 07777;
>>>>          if (S_ISREG(native_mode))
>>>>                  return 0100000 | perm_bits;
>>>>          if (S_ISDIR(native_mode))
>>>>                  return 0040000 | perm_bits;
>>>>          if (S_ISLNK(native_mode))
>>>>                  return 0120000 | perm_bits;
>>>>          if (S_ISBLK(native_mode))
>>>>                  return 0060000 | perm_bits;
>>>>          if (S_ISCHR(native_mode))
>>>>                  return 0020000 | perm_bits;
>>>>          if (S_ISFIFO(native_mode))
>>>>                  return 0010000 | perm_bits;
>>>>          /* Non-standard type bits were given. */
>>>>          /* Shouldn't we die() here ?? */
>>>>                  return perm_bits;
>>>> }
>>>
>>> Sure, I can send an updated patch with the new variable and without the
>>> "else"s.
>>>
>>> Regarding the question in the last comment:  I was assuming if this
>>> case was ever reached, Git would handle the returned mode the same way
>>> as if it encountered an unknown/non-standard file type on a normal
>>> operating system, which could die() if needed in the function that
>>> called stat().
>>>
>>> Should I send an updated patch that die()s there?
>>>
>>> David
>>
>> Not yet, please wait with a V2 patch until I finished my thinking ;-)
>>
>> I take back the suggestion with the die(). I was thinking how to handle
>> unforeseen types, which may show up on the z/OS some day,
>> So die() is not a good idea, it is better to ignore them, what the code
>> does.
>>
>> Knowing that Git does not track block devices, nor character devices nor
>> sockets,
>> the above code could be simplyfied even more, by mapping everything which
>> is not a directory, a file or a softlink to "device type 0)
>>
>> This is just a suggestion, I want to here from others as well:
>>
>>         int perm_bits = native_mode & 07777;
>>         if (S_ISREG(native_mode))
>>                 return 0100000 | perm_bits;
>>         if (S_ISDIR(native_mode))
>>                 return 0040000 | perm_bits;
>>         if (S_ISLNK(native_mode))
>>                 return 0120000 | perm_bits;
>>         /* Git does not track S_IFCHR, S_IFBLK, S_IFIFO, S_IFSOCK  */
>>                 return perm_bits;
>
> I had considered omitting those three as well at first, but in this
> case it would mean that they will be unusable all together.
>
> The z/OS S_IFMT definition (i.e. the file type bit mask) is
> 0xFF000000, and the common/translated S_IFMT definition is 0xF000.
> Since the S_ISxxx macros use the typical ((mode & S_IFMT) == S_IFxxx)
> definition, they would never match a native z/OS mode after redefining
> S_IFMT.
>
> So translating those types isn't just for tracking files, it's to
> support any use of S_ISxxx anywhere in the code.  It should be okay to
> remove any of those types if we know that Git will never need to use
> them.

Apologies, in my pre-coffee reply, I confused myself into thinking the
omitted types were going to be returned unchanged as opposed to being
changed to zero.  That second paragraph is irrelevant.

But regarding the last paragraph: a quick grep for instances of using
those file types in the Git source found S_ISFIFO and S_ISSOCK in
git.c.

I just noticed that I copied the list of standard file type macros
from SUSv2, and S_IFSOCK was added after that.  z/OS does implement
S_IFSOCK, so I think I should add it to the v2 patch to support the
test in git.c.

Another grep found no instances of testing for block or character
devices, so it should be okay to remove those from the patch if Git is
unlikely to use them in the future (unless we just want to provide all
7 types from http://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/stat.h.html
).

David

Re: [PATCH] compat: convert modes to use portable file type values

[David Michael]

On Mon, Dec 1, 2014 at 9:44 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Nov 30, 2014 at 9:41 AM, David Michael <fedora.dm0@gmail.com> wrote:
>> +int git_stat(const char *path, struct stat *buf)
>> +{
>> +       int rc;
>> +       rc = stat(path, buf);
>> +       if (buf != NULL)
>
> It's a minor thing, but maybe test "!rc" instead of "buf != NULL"?

Okay, it makes sense to only do the conversion for a successful return code.

Should it test for both a zero return code and a non-null pointer?  I
don't know if there are any cases where passing a null pointer is
legal.  The standard doesn't seem to explicitly forbid it.  z/OS
returns -1 and sets errno to EFAULT when stat() is given NULL, but
this patch should be able to be used on any platform.

David

Re: [PATCH] compat: convert modes to use portable file type values

[Junio C Hamano]

David Michael <fedora.dm0@gmail.com> writes:

> On Mon, Dec 1, 2014 at 9:44 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Sun, Nov 30, 2014 at 9:41 AM, David Michael <fedora.dm0@gmail.com> wrote:
>>> +int git_stat(const char *path, struct stat *buf)
>>> +{
>>> +       int rc;
>>> +       rc = stat(path, buf);
>>> +       if (buf != NULL)
>>
>> It's a minor thing, but maybe test "!rc" instead of "buf != NULL"?
>
> Okay, it makes sense to only do the conversion for a successful return code.
>
> Should it test for both a zero return code and a non-null pointer?  I
> don't know if there are any cases where passing a null pointer is
> legal.  The standard doesn't seem to explicitly forbid it.  z/OS
> returns -1 and sets errno to EFAULT when stat() is given NULL, but
> this patch should be able to be used on any platform.

Huh?  I am confused.  Since when is it legal to give NULL as statbuf
to (l)stat(2)?

Wouldn't something like this be sufficient and necessary?

	int rc = stat(path, buf);
        if (rc)
		return rc;

That is, let the underlying stat(2) diagnose any and all problems
(and leave clues in errno) and parrot its return value to the caller
to signal the failure?

Re: [PATCH] compat: convert modes to use portable file type values

[David Michael]

On Mon, Dec 1, 2014 at 12:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Michael <fedora.dm0@gmail.com> writes:
>
>> On Mon, Dec 1, 2014 at 9:44 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> On Sun, Nov 30, 2014 at 9:41 AM, David Michael <fedora.dm0@gmail.com> wrote:
>>>> +int git_stat(const char *path, struct stat *buf)
>>>> +{
>>>> +       int rc;
>>>> +       rc = stat(path, buf);
>>>> +       if (buf != NULL)
>>>
>>> It's a minor thing, but maybe test "!rc" instead of "buf != NULL"?
>>
>> Okay, it makes sense to only do the conversion for a successful return code.
>>
>> Should it test for both a zero return code and a non-null pointer?  I
>> don't know if there are any cases where passing a null pointer is
>> legal.  The standard doesn't seem to explicitly forbid it.  z/OS
>> returns -1 and sets errno to EFAULT when stat() is given NULL, but
>> this patch should be able to be used on any platform.
>
> Huh?  I am confused.  Since when is it legal to give NULL as statbuf
> to (l)stat(2)?
>
> Wouldn't something like this be sufficient and necessary?
>
>         int rc = stat(path, buf);
>         if (rc)
>                 return rc;
>
> That is, let the underlying stat(2) diagnose any and all problems
> (and leave clues in errno) and parrot its return value to the caller
> to signal the failure?

Alright, it wasn't immediately clear to me from the OpenGroup page on
stat() if that would always be safe.  I will just test the return code
in v2.

Thanks.

David

Re: [PATCH] compat: convert modes to use portable file type values

[Junio C Hamano]

David Michael <fedora.dm0@gmail.com> writes:

>> Huh?  I am confused.  Since when is it legal to give NULL as statbuf
>> to (l)stat(2)?
>>
>> Wouldn't something like this be sufficient and necessary?
>>
>>         int rc = stat(path, buf);
>>         if (rc)
>>                 return rc;
>>
>> That is, let the underlying stat(2) diagnose any and all problems
>> (and leave clues in errno) and parrot its return value to the caller
>> to signal the failure?
>
> Alright, it wasn't immediately clear to me from the OpenGroup page on
> stat() if that would always be safe.  I will just test the return code
> in v2.

It is irrelevant if that is safe or not.  As long as we are
emulating the underlying stat(), whether it is unsafe for stat(), we
should just throw whatever the user throws at us at the underlying
stat() and let it behave as if our emulation layer were not there.

Which is what the above snippet does.