Re: [PATCH] compat: convert modes to use portable file type values
On Mon, Dec 1, 2014 at 12:55 AM, Torsten Bögershausen tbo...@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 tbo...@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 0;
if (S_ISREG(native_mode))
return 010 | perm_bits;
if (S_ISDIR(native_mode))
return 004 | perm_bits;
if (S_ISLNK(native_mode))
return 012 | perm_bits;
if (S_ISBLK(native_mode))
return 006 | perm_bits;
if (S_ISCHR(native_mode))
return 002 | perm_bits;
if (S_ISFIFO(native_mode))
return 001 | 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
elses.
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 0;
if (S_ISREG(native_mode))
return 010 | perm_bits;
if (S_ISDIR(native_mode))
return 004 | perm_bits;
if (S_ISLNK(native_mode))
return 012 | 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
0xFF00, 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] compat: convert modes to use portable file type values
On Sun, Nov 30, 2014 at 9:41 AM, David Michael fedora@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] compat: convert modes to use portable file type values
On Mon, Dec 1, 2014 at 7:48 AM, David Michael fedora@gmail.com wrote:
On Mon, Dec 1, 2014 at 12:55 AM, Torsten Bögershausen tbo...@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 tbo...@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 0;
if (S_ISREG(native_mode))
return 010 | perm_bits;
if (S_ISDIR(native_mode))
return 004 | perm_bits;
if (S_ISLNK(native_mode))
return 012 | perm_bits;
if (S_ISBLK(native_mode))
return 006 | perm_bits;
if (S_ISCHR(native_mode))
return 002 | perm_bits;
if (S_ISFIFO(native_mode))
return 001 | 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
elses.
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 0;
if (S_ISREG(native_mode))
return 010 | perm_bits;
if (S_ISDIR(native_mode))
return 004 | perm_bits;
if (S_ISLNK(native_mode))
return 012 | 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
0xFF00, 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] compat: convert modes to use portable file type values
On Mon, Dec 1, 2014 at 9:44 AM, Duy Nguyen pclo...@gmail.com wrote:
On Sun, Nov 30, 2014 at 9:41 AM, David Michael fedora@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] compat: convert modes to use portable file type values
David Michael fedora@gmail.com writes:
On Mon, Dec 1, 2014 at 9:44 AM, Duy Nguyen pclo...@gmail.com wrote:
On Sun, Nov 30, 2014 at 9:41 AM, David Michael fedora@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?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] compat: convert modes to use portable file type values
On Mon, Dec 1, 2014 at 12:57 PM, Junio C Hamano gits...@pobox.com wrote:
David Michael fedora@gmail.com writes:
On Mon, Dec 1, 2014 at 9:44 AM, Duy Nguyen pclo...@gmail.com wrote:
On Sun, Nov 30, 2014 at 9:41 AM, David Michael fedora@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] compat: convert modes to use portable file type values
David Michael fedora@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] compat: convert modes to use portable file type values
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 010 | (native_mode 0);
+ else if (S_ISDIR(native_mode))
+ return 004 | (native_mode 0);
+ else if (S_ISLNK(native_mode))
+ return 012 | (native_mode 0);
+ else if (S_ISBLK(native_mode))
+ return 006 | (native_mode 0);
+ else if (S_ISCHR(native_mode))
+ return 002 | (native_mode 0);
+ else if (S_ISFIFO(native_mode))
+ return 001 | (native_mode 0);
+ else /* Non-standard type bits were given. */
+ return native_mode 0;
+}
Could the code be more human-readable ?
static inline mode_t mode_native_to_git(mode_t native_mode)
{
int perm_bits = native_mode 0;
if (S_ISREG(native_mode))
return 010 | perm_bits;
if (S_ISDIR(native_mode))
return 004 | perm_bits;
if (S_ISLNK(native_mode))
return 012 | perm_bits;
if (S_ISBLK(native_mode))
return 006 | perm_bits;
if (S_ISCHR(native_mode))
return 002 | perm_bits;
if (S_ISFIFO(native_mode))
return 001 | perm_bits;
/* Non-standard type bits were given. */
/* Shouldn't we die() here ?? */
return perm_bits;
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] compat: convert modes to use portable file type values
David Michael fedora@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 000..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 010 | (native_mode 0);
+ else if (S_ISDIR(native_mode))
+ return 004 | (native_mode 0);
+ else if (S_ISLNK(native_mode))
+ return 012 | (native_mode 0);
+ else if (S_ISBLK(native_mode))
+ return 006 | (native_mode 0);
+ else if (S_ISCHR(native_mode))
+ return 002 | (native_mode 0);
+ else if (S_ISFIFO(native_mode))
+ return 001 | (native_mode 0);
+ else /* Non-standard type bits were given. */
+ return native_mode 0;
+}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] compat: convert modes to use portable file type values
On Sun, Nov 30, 2014 at 3:16 PM, Torsten Bögershausen tbo...@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 0;
if (S_ISREG(native_mode))
return 010 | perm_bits;
if (S_ISDIR(native_mode))
return 004 | perm_bits;
if (S_ISLNK(native_mode))
return 012 | perm_bits;
if (S_ISBLK(native_mode))
return 006 | perm_bits;
if (S_ISCHR(native_mode))
return 002 | perm_bits;
if (S_ISFIFO(native_mode))
return 001 | 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 elses.
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] compat: convert modes to use portable file type values
On 12/01/2014 04:40 AM, David Michael wrote:
On Sun, Nov 30, 2014 at 3:16 PM, Torsten Bögershausen tbo...@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 0;
if (S_ISREG(native_mode))
return 010 | perm_bits;
if (S_ISDIR(native_mode))
return 004 | perm_bits;
if (S_ISLNK(native_mode))
return 012 | perm_bits;
if (S_ISBLK(native_mode))
return 006 | perm_bits;
if (S_ISCHR(native_mode))
return 002 | perm_bits;
if (S_ISFIFO(native_mode))
return 001 | 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 elses.
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 0;
if (S_ISREG(native_mode))
return 010 | perm_bits;
if (S_ISDIR(native_mode))
return 004 | perm_bits;
if (S_ISLNK(native_mode))
return 012 | perm_bits;
/* Git does not track S_IFCHR, S_IFBLK, S_IFIFO, S_IFSOCK */
return perm_bits;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
