compat: convert modes to use portable file type values

compat/stat.c new file +7 -0

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

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

On Sun, Nov 30, 2014 at 3:16 PM, Torsten Bögershausen <[email protected]> wrote:
[snip]

Quoted 12 lines

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;

Quoted 8 lines

         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

Quoted 35 lines · David Michael

On Sun, Nov 30, 2014 at 3:16 PM, Torsten Bögershausen <[email protected]> wrote:
[snip]

Quoted 13 lines

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;

Quoted 8 lines

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

Quoted 3 lines

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;

Quoted 54 lines · Torsten Bögershausen

Quoted 35 lines · David Michael

On Sun, Nov 30, 2014 at 3:16 PM, Torsten Bögershausen <[email protected]> wrote:
[snip]

Quoted 13 lines

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;

Quoted 8 lines

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

Quoted 3 lines

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:

Quoted 9 lines

        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.

compat/stat.c new file +3 -0

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

compat/stat.c line 28

Quoted 5 lines · David Michael

compat/stat.c new file +5 -0

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

Quoted 8 lines · Duy Nguyen

compat/stat.c line 28

Quoted 5 lines · David Michael

compat/stat.c new file +5 -0

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

Quoted 10 lines · David Michael

Quoted 8 lines · Duy Nguyen

compat/stat.c line 28

Quoted 5 lines · David Michael

compat/stat.c new file +5 -0

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

Quoted 7 lines

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?

Quoted 9 lines

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;

Quoted 3 lines

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.

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?

compat/stat.c new file +22 -0

new file mode 100644
index 0000000..0ff1f2f
@@ -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;
+}