"Joachim Schmitz" [off-list ref] writes:
I haven't found any other to be needed. Well, poll, maybe, but with only
minor tweaks for the win32 one works for me (and those tweaks are compatible
with win32
quoted
A separate file, compat/tandem/mkdir.c, is fine, though.
If you wouldn't have dozens of them, so compat/tandem/mkdir.c is not
suitable; compat/tandem.c would be good, then.
quoted
quoted
I'll go for git_mkdir(), similar to other git wrappers, (like for
mmap, pread, fopen, snprintf, vsnprintf, qsort).
Again, no. Your breakage is that having underlying system mkdir that does
not
quoted
understand trailing slash, which may not be specific to __TANDEM, but
still is
quoted
_not_ the only possible mode of breakage.
Well, it is the only one GNUlib's mkdir caters for and I'd regard that an
authoritative source...
I suspect that you may be misunderstanding what compat/ is about, so
let's try again.
Platform difference in mkdir may not be limited to "on this
platform, the underlying one supplied by the system does not like
path ending with a slash".
What I am saying is that it is unacceptable to call something that
caters to that specific kind of difference from what the codebase
expects with a generic name such as "git_mkdir()". Look at mingw's
replacement. The platform difference over there is that the one
from the system does not take mode parameter. Imagine that one
was already called "git_mkdir()". Now we have two different kind of
differences, and one has more officially-looking "git_mkdir()" name;
yours cannot take it---what would you do in that case? Neither kind
of difference is more officially sanctioned difference; don't call
yours any more official/generic than necessary.
Your wrapper is not limited to tandem, but is applicable to ancient
BSDs, so it is fine to call it as compat_mkdir_wo_trailing_slash(),
so that it can be shared among platforms whose mkdir do not want to
see trailing slashes. If you are going that route, the function
should live in its own file (without any other wrapper), and not be
named after specific platform (should be named after the specific
difference from what we expect, instead). I am perfectly fine with
that approach as well.
quoted
Squatting on a generic "git_mkdir()" name makes it harder for other people
to
quoted
name their compat mkdir functions to tweak for the breakage on their
platforms. The examples you listed are all "the platform does not offer
it, so
quoted
we implement the whole thing" kind, so it is in a different genre.
Nope, git_fopen() definitly is a wrapper for fopen(), as is git_vsnprintf()
for vsnprintf().
I was talking more about mmap() and pread().
For the two you mentioned, ideally they should have been named after
the specific breakages they cover (fopen that does not error out on
directories is primarily AIX thing IIRC, and snprintf returns bogus
result are shared between HPUX and Windows), but over these years we
haven't seen any other kind of differences from various platforms, so
the need to rename them away is very low.
On the other hand, we already know there are at least two kinds of
platform mkdir() that need different compat/ layer support, so
calling one "git_mkdir()" to cover one particular kind of difference
does not make any sense.
Besides, an earlier mistake is not a valid excuse to add new
mistakes.
From: Junio C Hamano [mailto:gitster@pobox.com]
Sent: Monday, August 20, 2012 6:54 PM
To: Joachim Schmitz
Cc: 'Shawn Pearce'; git@vger.kernel.org; rsbecker@nexbridge.com
Subject: Re: Porting git to HP NonStop
"Joachim Schmitz" [off-list ref] writes:
quoted
I haven't found any other to be needed. Well, poll, maybe, but with
only minor tweaks for the win32 one works for me (and those tweaks are
compatible with win32
quoted
A separate file, compat/tandem/mkdir.c, is fine, though.
If you wouldn't have dozens of them, so compat/tandem/mkdir.c is not
suitable;
compat/tandem.c would be good, then.
quoted
quoted
quoted
I'll go for git_mkdir(), similar to other git wrappers, (like for
mmap, pread, fopen, snprintf, vsnprintf, qsort).
Again, no. Your breakage is that having underlying system mkdir that
does
not
quoted
understand trailing slash, which may not be specific to __TANDEM, but
still is
quoted
_not_ the only possible mode of breakage.
True.
quoted
Well, it is the only one GNUlib's mkdir caters for and I'd regard that
an authoritative source...
I suspect that you may be misunderstanding what compat/ is about
I don't think so, it server the same purpose for git as gnulib does for
others.
, so let's try again.
Platform difference in mkdir may not be limited to "on this platform, the
underlying one supplied by the system does not like path ending with a
slash".
What I am saying is that it is unacceptable to call something that caters
to that
specific kind of difference from what the codebase expects with a generic
name such as "git_mkdir()". Look at mingw's replacement. The platform
difference over there is that the one from the system does not take mode
parameter. Imagine that one was already called "git_mkdir()". Now we
have
two different kind of differences, and one has more officially-looking
"git_mkdir()" name; yours cannot take it---what would you do in that case?
Neither kind of difference is more officially sanctioned difference; don't
call
yours any more official/generic than necessary.
Gnulib's rpl_mkdir caters for 3 possible problems, the WIN32 one which mkdir
taking only one argument, the trailing slash one discussed here (victims
being at least NetBSD 1.5.2 and current HP NonStop) and a trailing dot one
(that allegedly Cygwin 1.5 suffered from).
As far as I can see git will not suffer from the latter, but even if, at
that time a git_mkdir() could be expanded to cater for this to, just like
gnulib's one does, there it is an additional section inside their
rpl_mkdir().
And the WIN32 one is already being taken care of in compat/mingw.h. However,
this could as easily get integrated into
a git_mkdir(), just like in gnulib.
Your wrapper is not limited to tandem, but is applicable to ancient BSDs,
so it is
fine to call it as compat_mkdir_wo_trailing_slash(), so that it can be
shared
among platforms whose mkdir do not want to see trailing slashes. If you
are
going that route, the function should live in its own file (without any
other
wrapper), and not be named after specific platform (should be named after
the
specific difference from what we expect, instead). I am perfectly fine
with that
approach as well.
quoted
quoted
Squatting on a generic "git_mkdir()" name makes it harder for other
people
to
quoted
name their compat mkdir functions to tweak for the breakage on their
platforms. The examples you listed are all "the platform does not
offer
it, so
quoted
we implement the whole thing" kind, so it is in a different genre.
Nope, git_fopen() definitly is a wrapper for fopen(), as is
git_vsnprintf() for vsnprintf().
I was talking more about mmap() and pread().
For the two you mentioned, ideally they should have been named after the
specific breakages they cover (fopen that does not error out on
directories is
primarily AIX thing IIRC, and snprintf returns bogus result are shared
between
HPUX and Windows), but over these years we haven't seen any other kind of
differences from various platforms, so the need to rename them away is
very
low.
On the other hand, we already know there are at least two kinds of
platform
mkdir() that need different compat/ layer support, so calling one
"git_mkdir()"
to cover one particular kind of difference does not make any sense.
Besides, an earlier mistake is not a valid excuse to add new mistakes.
OK, so how about this:
/usr/local/bin/diff -EBbu ./compat/mkdir.c.orig ./compat/mkdir.c
--- ./compat/mkdir.c.orig 2012-08-21 05:02:11 -0500
+++ ./compat/mkdir.c 2012-08-21 05:02:11 -0500
@@ -0,0 +1,24 @@
+#include "../git-compat-util.h"
+#undef mkdir
+
+/* for platforms that can't deal with a trailing '/' */
+int compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode)
+{
+ int retval;
+ char *tmp_dir = NULL;
+ size_t len = strlen(dir);
+
+ if (len && dir[len-1] == '/') {
+ if ((tmp_dir = strdup(dir)) == NULL)
+ return -1;
+ tmp_dir[len-1] = '\0';
+ }
+ else
+ tmp_dir = (char *)dir;
+
+ retval = mkdir(tmp_dir, mode);
+ if (tmp_dir != dir)
+ free(tmp_dir);
+
+ return retval;
+}
BTW: I've just today reported that mkdir bug to HP NonStop development.
Let's see how fast they fix it and whether at all, but I'd be pretty
surprised if that happened earlier than 6 months from now.
Bye, Jojo
On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz
[off-list ref] wrote:
quoted hunk
OK, so how about this:
/usr/local/bin/diff -EBbu ./compat/mkdir.c.orig ./compat/mkdir.c
--- ./compat/mkdir.c.orig 2012-08-21 05:02:11 -0500
+++ ./compat/mkdir.c 2012-08-21 05:02:11 -0500
@@ -0,0 +1,24 @@
+#include "../git-compat-util.h"
+#undef mkdir
+
+/* for platforms that can't deal with a trailing '/' */
+int compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode)
+{
+ int retval;
+ char *tmp_dir = NULL;
+ size_t len = strlen(dir);
+
+ if (len && dir[len-1] == '/') {
+ if ((tmp_dir = strdup(dir)) == NULL)
+ return -1;
+ tmp_dir[len-1] = '\0';
+ }
+ else
+ tmp_dir = (char *)dir;
+
+ retval = mkdir(tmp_dir, mode);
+ if (tmp_dir != dir)
+ free(tmp_dir);
+
+ return retval;
+}
Why not rearrange this so that you assign to dir the value of tmp_dir
and then just pass dir to mkdir. Then you can avoid the recast of dir
to (char*) in the else branch. Later, just call free(tmp_dir). Also,
we have xstrndup. So I think the body of your function can become
something like:
if (len && dir[len-1] == '/')
dir = tmp_dir = xstrndup(dir, len-1);
retval = mkdir(dir, mode);
free(tmp_dir);
-Brandon
On Wed, Aug 22, 2012 at 10:00 AM, Brandon Casey [off-list ref] wrote:
Also, we have xstrndup. So I think the body of your function can become
something like:
if (len && dir[len-1] == '/')
dir = tmp_dir = xstrndup(dir, len-1);
retval = mkdir(dir, mode);
free(tmp_dir);
Actually, xmemdupz could be used in place of xstrndup since we've
already called strlen.
-Brandon
From: Brandon Casey [mailto:drafnel@gmail.com]
Sent: Wednesday, August 22, 2012 7:01 PM
To: Joachim Schmitz
Cc: Junio C Hamano; Shawn Pearce; git@vger.kernel.org;
rsbecker@nexbridge.com
Subject: Re: Porting git to HP NonStop
On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz [off-list ref]
wrote:
quoted
OK, so how about this:
/usr/local/bin/diff -EBbu ./compat/mkdir.c.orig ./compat/mkdir.c
--- ./compat/mkdir.c.orig 2012-08-21 05:02:11 -0500
+++ ./compat/mkdir.c 2012-08-21 05:02:11 -0500
@@ -0,0 +1,24 @@
+#include "../git-compat-util.h"
+#undef mkdir
+
+/* for platforms that can't deal with a trailing '/' */ int
+compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode) {
+ int retval;
+ char *tmp_dir = NULL;
+ size_t len = strlen(dir);
+
+ if (len && dir[len-1] == '/') {
+ if ((tmp_dir = strdup(dir)) == NULL)
+ return -1;
+ tmp_dir[len-1] = '\0';
+ }
+ else
+ tmp_dir = (char *)dir;
+
+ retval = mkdir(tmp_dir, mode);
+ if (tmp_dir != dir)
+ free(tmp_dir);
+
+ return retval;
+}
Why not rearrange this so that you assign to dir the value of tmp_dir and then
just pass dir to mkdir. Then you can avoid the recast of dir to (char*) in the
else branch. Later, just call free(tmp_dir). Also, we have xstrndup. So I think
the body of your function can become something like:
if (len && dir[len-1] == '/')
dir = tmp_dir = xstrndup(dir, len-1);
xstndup() can't fail?
retval = mkdir(dir, mode);
free(tmp_dir);
-Brandon
Bye, Jojo
From: Brandon Casey [mailto:drafnel@gmail.com]
Sent: Wednesday, August 22, 2012 7:23 PM
To: Joachim Schmitz
Cc: Junio C Hamano; Shawn Pearce; git@vger.kernel.org;
rsbecker@nexbridge.com
Subject: Re: Porting git to HP NonStop
On Wed, Aug 22, 2012 at 10:18 AM, Joachim Schmitz [off-list ref]
wrote:
quoted
quoted
From: Brandon Casey [mailto:drafnel@gmail.com]
Sent: Wednesday, August 22, 2012 7:01 PM
To: Joachim Schmitz
Cc: Junio C Hamano; Shawn Pearce; git@vger.kernel.org;
rsbecker@nexbridge.com
Subject: Re: Porting git to HP NonStop
On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz
[off-list ref]
wrote:
quoted
OK, so how about this:
/usr/local/bin/diff -EBbu ./compat/mkdir.c.orig ./compat/mkdir.c
--- ./compat/mkdir.c.orig 2012-08-21 05:02:11 -0500
+++ ./compat/mkdir.c 2012-08-21 05:02:11 -0500
@@ -0,0 +1,24 @@
+#include "../git-compat-util.h"
+#undef mkdir
+
+/* for platforms that can't deal with a trailing '/' */ int
+compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode) {
+ int retval;
+ char *tmp_dir = NULL;
+ size_t len = strlen(dir);
+
+ if (len && dir[len-1] == '/') {
+ if ((tmp_dir = strdup(dir)) == NULL)
+ return -1;
+ tmp_dir[len-1] = '\0';
+ }
+ else
+ tmp_dir = (char *)dir;
+
+ retval = mkdir(tmp_dir, mode);
+ if (tmp_dir != dir)
+ free(tmp_dir);
+
+ return retval;
+}
Why not rearrange this so that you assign to dir the value of tmp_dir
and then just pass dir to mkdir. Then you can avoid the recast of
dir to (char*) in the else branch. Later, just call free(tmp_dir).
Also, we have xstrndup. So I think the body of your function can become
something like:
quoted
quoted
if (len && dir[len-1] == '/')
dir = tmp_dir = xstrndup(dir, len-1);
xstndup() can't fail?
Correct. It will either succeed or die. It will also try to free up some memory
used by git if possible.
OK. So let's use that then.
Bye, Jojo
Am 22.08.2012 19:00, schrieb Brandon Casey:
So I think the body of [compat_mkdir] can become
something like:
if (len && dir[len-1] == '/')
dir = tmp_dir = xstrndup(dir, len-1);
Don't use x* wrappers in the compat layer, at least not those that
allocate memory: They behave unpredictably due to try_to_free_routine
and may lead to recursive invocations.
retval = mkdir(dir, mode);
free(tmp_dir);
-- Hannes
-----Original Message-----
From: Johannes Sixt [mailto:j6t@kdbg.org]
Sent: Wednesday, August 22, 2012 7:41 PM
To: Brandon Casey
Cc: Joachim Schmitz; Junio C Hamano; Shawn Pearce; git@vger.kernel.org;
rsbecker@nexbridge.com
Subject: Re: Porting git to HP NonStop
Am 22.08.2012 19:00, schrieb Brandon Casey:
quoted
So I think the body of [compat_mkdir] can become something like:
if (len && dir[len-1] == '/')
dir = tmp_dir = xstrndup(dir, len-1);
Don't use x* wrappers in the compat layer, at least not those that allocate
memory: They behave unpredictably due to try_to_free_routine and may lead
to recursive invocations.
I was just following orders ;-)
What about the other proposal, xmemdupz? Same story I guess?
Bye, Jojo
Am 22.08.2012 20:02, schrieb Joachim Schmitz:
quoted
From: Johannes Sixt [mailto:j6t@kdbg.org]
Don't use x* wrappers in the compat layer, at least not those that allocate
memory: They behave unpredictably due to try_to_free_routine and may lead
to recursive invocations.
I was just following orders ;-)
What about the other proposal, xmemdupz? Same story I guess?
xmemdupz calls xmalloc, so, yes, same story.
-- Hannes
On Wed, Aug 22, 2012 at 10:41 AM, Johannes Sixt [off-list ref] wrote:
Am 22.08.2012 19:00, schrieb Brandon Casey:
quoted
So I think the body of [compat_mkdir] can become
something like:
if (len && dir[len-1] == '/')
dir = tmp_dir = xstrndup(dir, len-1);
Don't use x* wrappers in the compat layer, at least not those that
allocate memory: They behave unpredictably due to try_to_free_routine
and may lead to recursive invocations.
I thought that rule only applied to die handlers. i.e. don't use the
x* wrappers to allocate memory in a die handler like
compat/win32/syslog.c. At least that's what I wrote in 040a6551 when
you pointed out this issue back then.
Admittedly, it could get pretty sticky trying to trace the die
handlers to ensure they don't invoke your new compat/ function. So,
yeah, adopting this rule of not using x* wrappers that allocate memory
in compat/ generally seems like a good idea.
Should we also try to detect recursive invocation of die and friends?
In theory recursion could be triggered by any die handler that makes
use of a code path that calls an x* wrapper that allocates memory,
couldn't it?
-Brandon
-----Original Message-----
From: Johannes Sixt [mailto:j6t@kdbg.org]
Sent: Wednesday, August 22, 2012 8:09 PM
To: Joachim Schmitz
Cc: 'Brandon Casey'; 'Junio C Hamano'; 'Shawn Pearce'; git@vger.kernel.org;
rsbecker@nexbridge.com
Subject: Re: Porting git to HP NonStop
Am 22.08.2012 20:02, schrieb Joachim Schmitz:
quoted
quoted
From: Johannes Sixt [mailto:j6t@kdbg.org] Don't use x* wrappers in
the compat layer, at least not those that allocate
memory: They behave unpredictably due to try_to_free_routine and may
lead to recursive invocations.
I was just following orders ;-)
What about the other proposal, xmemdupz? Same story I guess?
xmemdupz calls xmalloc, so, yes, same story.
So back to my original patch, using strdup, check the return value, etc.
Bye, Jojo
On Wed, Aug 22, 2012 at 11:09 AM, Brandon Casey [off-list ref] wrote:
On Wed, Aug 22, 2012 at 10:41 AM, Johannes Sixt [off-list ref] wrote:
quoted
Don't use x* wrappers in the compat layer, at least not those that
allocate memory: They behave unpredictably due to try_to_free_routine
and may lead to recursive invocations.
I thought that rule only applied to die handlers. i.e. don't use the
x* wrappers to allocate memory in a die handler like
compat/win32/syslog.c. At least that's what I wrote in 040a6551 when
you pointed out this issue back then.
Admittedly, it could get pretty sticky trying to trace the die
handlers to ensure they don't invoke your new compat/ function. So,
yeah, adopting this rule of not using x* wrappers that allocate memory
in compat/ generally seems like a good idea.
Should we also try to detect recursive invocation of die and friends?
In theory recursion could be triggered by any die handler that makes
use of a code path that calls an x* wrapper that allocates memory,
couldn't it?
Perhaps something like:
diff --git a/usage.c b/usage.c
index a2a6678..2d0ff35 100644
--- a/usage.c
+++ b/usage.c
@@ -80,8 +80,15 @@ void NORETURN usage(const char *err)
void NORETURN die(const char *err, ...)
{
+ static int dying;
va_list params;
+ if (dying) {
+ fputs("fatal: recursion detected in die handler\n", stderr);
+ exit(128);
+ }
+ dying = 1;
+
va_start(params, err);
die_routine(err, params);
va_end(params);@@ -89,11 +96,18 @@ void NORETURN die(const char *err, ...)
void NORETURN die_errno(const char *fmt, ...)
{
+ static int dying;
va_list params;
char fmt_with_err[1024];
char str_error[256], *err;
int i, j;
+ if (dying) {
+ fputs("fatal: recursion detected in die handler\n", stderr);
+ exit(128);
+ }
+ dying = 1;
+
err = strerror(errno);
for (i = j = 0; err[i] && j < sizeof(str_error) - 1; ) {
if ((str_error[j++] = err[i++]) != '%')
-Brandon