Re: git on HP NonStop
On 08/22/2012 06:38 PM, Joachim Schmitz wrote: -Original Message- From: Junio C Hamano [mailto:gits...@pobox.com] Sent: Tuesday, August 21, 2012 4:06 AM To: Joachim Schmitz Cc: 'Johannes Sixt'; 'Jan Engelhardt'; git@vger.kernel.org Subject: Re: git on HP NonStop Joachim Schmitz j...@schmitz-digital.de writes: OK, so let's have a look at code, current git, builtin/cat-file.c, line 196: void *contents = contents; This variable is set later in an if branch (if (print_contents == BATCH), but not in the else branch. It is later used always under the same condition as the one under which it is set. Apparently is is malloc_d storage (there a free(content);), so there's no harm al all in initializing it with NULL, even if it only appeases a stupid compiler. It actually is harmful. See below. Harmful to initialize with NULL or to use that undefined behavoir? I checked what our compiler does here: after having warned about vlues us used before it is set: it actually dies seem to have initializes the value to 0 resp. NULL. So here there's no harm done in avoiding undefined behavior and set it to 0 resp NULL in the first place. There is harm in tricking future programmers into thinking that the initialization actually means something, which some of them do. It's unlikely that you're the one to maintain that code forever, and the var = var idiom is used widely within git with a clear meaning as a hint to programmers who read a bit of git code. If they aren't used to that idiom, they usually investigate it in the code and pretty quickly realize that what it means. -- Andreas Ericsson andreas.erics...@op5.se OP5 AB www.op5.se Tel: +46 8-230225 Fax: +46 8-230231 Considering the successes of the wars on alcohol, poverty, drugs and terror, I think we should give some serious thought to declaring war on peace. -- 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: git on HP NonStop
From: Andreas Ericsson [mailto:a...@op5.se] Sent: Thursday, August 23, 2012 10:24 AM To: Joachim Schmitz Cc: 'Junio C Hamano'; 'Johannes Sixt'; 'Jan Engelhardt'; git@vger.kernel.org Subject: Re: git on HP NonStop On 08/22/2012 06:38 PM, Joachim Schmitz wrote: -Original Message- From: Junio C Hamano [mailto:gits...@pobox.com] Sent: Tuesday, August 21, 2012 4:06 AM To: Joachim Schmitz Cc: 'Johannes Sixt'; 'Jan Engelhardt'; git@vger.kernel.org Subject: Re: git on HP NonStop Joachim Schmitz j...@schmitz-digital.de writes: OK, so let's have a look at code, current git, builtin/cat-file.c, line 196: void *contents = contents; This variable is set later in an if branch (if (print_contents == BATCH), but not in the else branch. It is later used always under the same condition as the one under which it is set. Apparently is is malloc_d storage (there a free(content);), so there's no harm al all in initializing it with NULL, even if it only appeases a stupid compiler. It actually is harmful. See below. Harmful to initialize with NULL or to use that undefined behavoir? I checked what our compiler does here: after having warned about vlues us used before it is set: it actually dies seem to have initializes the value to 0 resp. NULL. So here there's no harm done in avoiding undefined behavior and set it to 0 resp NULL in the first place. There is harm in tricking future programmers into thinking that the initialization actually means something, which some of them do. Hmm, OK, I can agree to that. It's unlikely that you're the one to maintain that code forever, It is unlike for me to ever have to maintain this code. Currently that's Junio's job and I won't apply for in ;-) and the var = var idiom is used widely within git This is overstating it a bit. I went thru the entire code and reported all places I could find in an earlier email. I went back and counted: It is used in 11 files, at 15 places, for 21 variables. OK, I may have missed a few more that were in code paths my compiler didn't see, but still some 21+ isn't really much. with a clear meaning Only if you call undefined behavior a 'clear meaning! as a hint to programmers who read a bit of git code. If they aren't used to that idiom, they usually investigate it in the code and pretty quickly realize that what it means. Whether I realize what it means, is irrelevant, my compiler does not and warns about it, and as per the ANSI/ICO C standard it invokes undefined behavior. If a proper initialization is meaningless for these cases, don't do them at all, let the stupid compiler complain about it and the clever programmer check whether the warning is useful, but don't avoid a compiler warning on one compiler by introducing undefined behavior and provoke a compiler warning on another. Bye, Jojo -- 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: Porting git to HP NonStop
From: Junio C Hamano [mailto:gits...@pobox.com]
Sent: Monday, August 20, 2012 6:54 PM
To: Joachim Schmitz
Cc: 'Shawn Pearce'; git@vger.kernel.org; rsbec...@nexbridge.com
Subject: Re: Porting git to HP NonStop
Joachim Schmitz j...@schmitz-digital.de 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
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.
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
understand trailing slash, which may not be specific to __TANDEM, but
still is
_not_ the only possible mode of breakage.
True.
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.
Squatting on a generic git_mkdir() name makes it harder for other
people
to
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
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.c2012-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
RE: git on HP NonStop
-Original Message- From: Junio C Hamano [mailto:gits...@pobox.com] Sent: Tuesday, August 21, 2012 4:06 AM To: Joachim Schmitz Cc: 'Johannes Sixt'; 'Jan Engelhardt'; git@vger.kernel.org Subject: Re: git on HP NonStop Joachim Schmitz j...@schmitz-digital.de writes: OK, so let's have a look at code, current git, builtin/cat-file.c, line 196: void *contents = contents; This variable is set later in an if branch (if (print_contents == BATCH), but not in the else branch. It is later used always under the same condition as the one under which it is set. Apparently is is malloc_d storage (there a free(content);), so there's no harm al all in initializing it with NULL, even if it only appeases a stupid compiler. It actually is harmful. See below. Harmful to initialize with NULL or to use that undefined behavoir? I checked what our compiler does here: after having warned about vlues us used before it is set: it actually dies seem to have initializes the value to 0 resp. NULL. So here there's no harm done in avoiding undefined behavior and set it to 0 resp NULL in the first place. The next one, builtin/fast-export.c, line 486: struct commit *commit = commit; it is set in a switch statement, but not in every case, as far as I can see. Hmm, maybe it is, and I just get lost in the code And it is used directly after the switch, hopefully set to something reasonable. Why take the risk and not set it to NULL? Ditto. Next one, builtin/rev-list.c, line 390: int reaches = reaches, all = all; revs.commits = find_bisection(revs.commits, reaches, all, bisect_find_all); Seem pretty pointless to initialize them, provided find_bisection doesn't read them. Does it? That is why they are not initializations but marks to the compiler to signal you may be stupid enough to think they are used before initialized or assigned, but that is not the case. Initializing them would be pointless. Next one, fast-import.c, line 2268: struct object_entry *oe = oe; os gets set in en if and an else branch, but not in then intermediate else if branch! Look again. If the recent code is too complex for you to understand, go back to 10e8d68 (Correct compiler warnings in fast-import., 2007-02-06) and read the function. The control flow of the early part of that function dictates that either oe is assigned *or* inline_data is set to 1. When inline_data is false, oe is always set. The compiler was too stupid to read that, and that is why the (confusing) idiom to mark it for the stupid compiler was used. There are a few reasons why I do not think this self-assignment idiom or initializing the variable to an innocuous-looking random value is a particularly good thing to do when you see compiler warnings. If the compiler suspects the variable might be unused, you should always look at it and follow the flow yourself. Once you know it is a false alarm, you can use the idiom to squelch the warning, and it at the same serves as a note to others that you verified the flow and made sure it is a false warning. When the next person wants to touch the code, if the person knows the use of the idiom, it only serves as a warning to be extra careful not to introduce a new codepath that reads the variable without setting, as the compiler no longer helps him. If the person who touches the code is as clueless as the compiler and cannot follow the codepath to see the variable is never used uninitialized, the result will be a lot worse. That is the reason why I do not think the idiom to squelch the compiler is such a good thing. Careless people touch the code, so oe = oe initialization carefully placed in the original version does not necessarily stay as a useful documentation. But if you use oe = NULL as a way to squelch the warning in the first place, it is no better than oe = oe. In a sense, it is even worse, as it just looks like any other initialization and gives a false impression that the remainder of the code is written in such a way that it tolerates oe being NULL in any codepath, or there is some assignment before that before the code reaches places where oe cannot be NULL. That is different from what oe = oe initializaion documents-- -in the codepath protected by if (inline_data), it isn't just oe can safely be NULL there; instead it is oe can safely be *any* value there, because we don't use it. Of course, if you explicitly initialized oe to NULL, even if you introduce a codepath where oe cannot be NULL later, you won't get a warning from the compiler, so it is no better than oe = oe. And that is the reason why I do not think initialization to an innocuous-looking random value (e.g. NULL) is a good answer, either. When both are not good, replacing oe = oe with oe = NULL didn't make much sense
Re: Porting git to HP NonStop
On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz
j...@schmitz-digital.de wrote:
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.c2012-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
--
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: Porting git to HP NonStop
On Wed, Aug 22, 2012 at 10:00 AM, Brandon Casey draf...@gmail.com 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 -- 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: Porting git to HP NonStop
From: Brandon Casey [mailto:draf...@gmail.com]
Sent: Wednesday, August 22, 2012 7:01 PM
To: Joachim Schmitz
Cc: Junio C Hamano; Shawn Pearce; git@vger.kernel.org;
rsbec...@nexbridge.com
Subject: Re: Porting git to HP NonStop
On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz j...@schmitz-digital.de
wrote:
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.c2012-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
--
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: Porting git to HP NonStop
On Wed, Aug 22, 2012 at 10:18 AM, Joachim Schmitz
j...@schmitz-digital.de wrote:
From: Brandon Casey [mailto:draf...@gmail.com]
Sent: Wednesday, August 22, 2012 7:01 PM
To: Joachim Schmitz
Cc: Junio C Hamano; Shawn Pearce; git@vger.kernel.org;
rsbec...@nexbridge.com
Subject: Re: Porting git to HP NonStop
On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz j...@schmitz-digital.de
wrote:
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.c2012-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?
Correct. It will either succeed or die. It will also try to free up
some memory used by git if possible.
-Brandon
--
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: Porting git to HP NonStop
Brandon Casey draf...@gmail.com writes:
On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz
j...@schmitz-digital.de wrote:
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.c2012-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);
+ ...
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);
Nice. And we have xmemdupz() would be even better as you
followed-up.
Thanks.
--
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: Porting git to HP NonStop
From: Brandon Casey [mailto:draf...@gmail.com]
Sent: Wednesday, August 22, 2012 7:23 PM
To: Joachim Schmitz
Cc: Junio C Hamano; Shawn Pearce; git@vger.kernel.org;
rsbec...@nexbridge.com
Subject: Re: Porting git to HP NonStop
On Wed, Aug 22, 2012 at 10:18 AM, Joachim Schmitz j...@schmitz-digital.de
wrote:
From: Brandon Casey [mailto:draf...@gmail.com]
Sent: Wednesday, August 22, 2012 7:01 PM
To: Joachim Schmitz
Cc: Junio C Hamano; Shawn Pearce; git@vger.kernel.org;
rsbec...@nexbridge.com
Subject: Re: Porting git to HP NonStop
On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz
j...@schmitz-digital.de
wrote:
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.c2012-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?
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
--
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: Porting git to HP NonStop
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 -- 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: Porting git to HP NonStop
Am 22.08.2012 20:02, schrieb Joachim Schmitz: From: Johannes Sixt [mailto:j...@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 -- 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: Porting git to HP NonStop
On Wed, Aug 22, 2012 at 10:41 AM, Johannes Sixt j...@kdbg.org wrote: 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. 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 -- 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: Porting git to HP NonStop
-Original Message- From: Johannes Sixt [mailto:j...@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; rsbec...@nexbridge.com Subject: Re: Porting git to HP NonStop Am 22.08.2012 20:02, schrieb Joachim Schmitz: From: Johannes Sixt [mailto:j...@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 -- 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: Porting git to HP NonStop
On Wed, Aug 22, 2012 at 11:09 AM, Brandon Casey draf...@gmail.com wrote:
On Wed, Aug 22, 2012 at 10:41 AM, Johannes Sixt j...@kdbg.org wrote:
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
--
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: Porting git to HP NonStop
Joachim Schmitz j...@schmitz-digital.de writes:
Nice. And we have xmemdupz() would be even better as you followed-up.
How's that one used?
I forgot that we frown upon use of any xallocate() wrapper in the
compat/ layer as J6t mentioned.
So probably something along these lines...
int retval;
char *dir_to_free = NULL;
size_t len = strlen(dir);
if (len dir[len - 1] == '/') {
dir_to_free = malloc(len);
if (!dir_to_free) {
fprintf(stderr, malloc failed!\n);
exit(1);
}
memcpy(dir_to_free, dir, len - 1);
dir_to_free[len - 1] = '\0';
dir = dir_to_free;
}
retval = mkdir(dir, mode);
free(dir_to_free);
return retval;
It might be possible to for the error path to get away with
something like:
if (!dir_to_free)
return -1;
if we know the callers are prepared to see mkdir() failing with
ENOMEM, but that is not very likely.
--
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: Porting git to HP NonStop
Brandon Casey draf...@gmail.com writes: On Wed, Aug 22, 2012 at 10:41 AM, Johannes Sixt j...@kdbg.org wrote: 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. 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? Correct, but at that point we will end up dying anyway, so... -- 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: Porting git to HP NonStop
Brandon Casey draf...@gmail.com writes:
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++]) != '%')
With two function-scope static, you can go like this:
die()
- die_routine()
- xsomething()
- die_errno()
- die_routine()
- xsomethingelse()
- die() or die_errno()
Not that we probably care too deeply about, as at least we won't
infinitely recurse and die out of stack space.
--
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: Porting git to HP NonStop
On Wed, Aug 22, 2012 at 11:33 AM, Junio C Hamano gits...@pobox.com wrote:
Brandon Casey draf...@gmail.com writes:
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++]) != '%')
With two function-scope static, you can go like this:
die()
- die_routine()
- xsomething()
- die_errno()
- die_routine()
- xsomethingelse()
- die() or die_errno()
Not that we probably care too deeply about, as at least we won't
infinitely recurse and die out of stack space.
Yeah, I noticed that, but didn't think it was important or likely.
But there's no reason not to make dying a global.
-Brandon
--
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: Porting git to HP NonStop
From: Junio C Hamano [mailto:gits...@pobox.com]
Sent: Wednesday, August 22, 2012 8:25 PM
To: Joachim Schmitz
Cc: 'Brandon Casey'; 'Shawn Pearce'; git@vger.kernel.org;
rsbec...@nexbridge.com
Subject: Re: Porting git to HP NonStop
Joachim Schmitz j...@schmitz-digital.de writes:
Nice. And we have xmemdupz() would be even better as you followed-up.
How's that one used?
I forgot that we frown upon use of any xallocate() wrapper in the
compat/
layer as J6t mentioned.
So probably something along these lines...
int retval;
char *dir_to_free = NULL;
size_t len = strlen(dir);
if (len dir[len - 1] == '/') {
dir_to_free = malloc(len);
if (!dir_to_free) {
fprintf(stderr, malloc failed!\n);
exit(1);
}
memcpy(dir_to_free, dir, len - 1);
dir_to_free[len - 1] = '\0';
dir = dir_to_free;
}
retval = mkdir(dir, mode);
free(dir_to_free);
return retval;
So why not just strdup? I stole the idea from gnulib...
int
rpl_mkdir (char const *dir, mode_t mode maybe_unused)
{
int ret_val;
char *tmp_dir;
size_t len = strlen (dir);
if (len dir[len - 1] == '/')
{
tmp_dir = strdup (dir);
if (!tmp_dir)
{
/* Rather than rely on strdup-posix, we set errno ourselves. */
errno = ENOMEM;
return -1;
}
strip_trailing_slashes (tmp_dir);
}
else
{
tmp_dir = (char *) dir;
}
They strip more than one trailing slash, but for git's purpose I believed
this to be too much overhead. Also the errno stuff doesn't seem to be really
needed IMHO. Same for the following code
#if FUNC_MKDIR_DOT_BUG
/* Additionally, cygwin 1.5 mistakenly creates a directory d/./. */
{
char *last = last_component (tmp_dir);
if (*last == '.' (last[1] == '\0'
|| (last[1] == '.' last[2] == '\0')))
{
struct stat st;
if (stat (tmp_dir, st) == 0)
errno = EEXIST;
return -1;
}
}
#endif /* FUNC_MKDIR_DOT_BUG */
Then it goes on like mine:
ret_val = mkdir (tmp_dir, mode);
if (tmp_dir != dir)
free (tmp_dir);
return ret_val;
}
Compare:
$ cat compat/mkdir.c
#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;
}
Bye, Jojo
--
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: Porting git to HP NonStop
Hi folks
There another API missing on HP NonStop and that is setitimer(), used in
progress.c and build/log.c
I do have a homebrewed implementation, on top of alarm(), it goes like this:
#include ../git-compat-util.h
#undef getitimer
#undef setitimer
int
git_getitimer(int which, struct itimerval *value)
{
int ret = 0;
switch (which) {
case ITIMER_REAL:
value-it_value.tv_usec = 0;
value-it_value.tv_sec = alarm(0);
ret = 0; /* if alarm() fails we get a SIGLIMIT */
break;
case ITIMER_VIRTUAL: /* FALLTHRU */
case ITIMER_PROF: errno = ENOTSUP; ret = -1; break;
default: errno = EINVAL; ret = -1;
}
return ret;
}
int
git_setitimer(int which, const struct itimerval *value,
struct itimerval *ovalue)
{
int ret = 0;
if (!value
|| value-it_value.tv_usec 0
|| value-it_value.tv_usec 100
|| value-it_value.tv_sec 0) {
errno = EINVAL;
return -1;
}
else if (ovalue)
if (!git_getitimer(which, ovalue))
return -1; /* errno set in git_getitimer() */
else
switch (which) {
case ITIMER_REAL:
alarm(value-it_value.tv_sec +
(value-it_value.tv_usec 0) ? 1 : 0);
ret = 0; /* if alarm() fails we get a SIGLIMIT */
break;
case ITIMER_VIRTUAL: /* FALLTHRU */
case ITIMER_PROF: errno = ENOTSUP; ret = -1; break;
default: errno = EINVAL; ret = -1;
}
return ret;
}
Worth being added to compat/, e.g. as setitimer.c, or, as itimer.c (as a
by-product, it has getitimer() too)?
Bye, Jojo
--
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: Porting git to HP NonStop
Joachim Schmitz j...@schmitz-digital.de writes:
Hi folks
There another API missing on HP NonStop and that is setitimer(), used in
progress.c and build/log.c
I do have a homebrewed implementation, on top of alarm(), it goes like this:
#include ../git-compat-util.h
#undef getitimer
#undef setitimer
int
git_getitimer(int which, struct itimerval *value)
See Documentation/CodingGuidelines for style nits.
{
int ret = 0;
switch (which) {
case ITIMER_REAL:
value-it_value.tv_usec = 0;
value-it_value.tv_sec = alarm(0);
ret = 0; /* if alarm() fails we get a SIGLIMIT */
break;
case ITIMER_VIRTUAL: /* FALLTHRU */
case ITIMER_PROF: errno = ENOTSUP; ret = -1; break;
default: errno = EINVAL; ret = -1;
}
return ret;
}
int
git_setitimer(int which, const struct itimerval *value,
struct itimerval *ovalue)
{
int ret = 0;
if (!value
|| value-it_value.tv_usec 0
|| value-it_value.tv_usec 100
|| value-it_value.tv_sec 0) {
errno = EINVAL;
return -1;
}
else if (ovalue)
if (!git_getitimer(which, ovalue))
return -1; /* errno set in git_getitimer() */
else
switch (which) {
case ITIMER_REAL:
alarm(value-it_value.tv_sec +
(value-it_value.tv_usec 0) ? 1 : 0);
ret = 0; /* if alarm() fails we get a SIGLIMIT */
break;
case ITIMER_VIRTUAL: /* FALLTHRU */
case ITIMER_PROF: errno = ENOTSUP; ret = -1; break;
default: errno = EINVAL; ret = -1;
}
return ret;
}
Worth being added to compat/, e.g. as setitimer.c, or, as itimer.c
(as a by-product, it has getitimer() too)?
If it helps your port, compat/itimer.c sounds like a good place.
Doesn't it need a new header file to introduce structures and
constants, too?
--
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: Porting git to HP NonStop
From: Junio C Hamano [mailto:gits...@pobox.com] Sent: Wednesday, August 22, 2012 10:50 PM To: Joachim Schmitz Cc: git@vger.kernel.org; rsbec...@nexbridge.com Subject: Re: Porting git to HP NonStop Joachim Schmitz j...@schmitz-digital.de writes: Hi folks There another API missing on HP NonStop and that is setitimer(), used in progress.c and build/log.c I do have a homebrewed implementation, on top of alarm(), it goes like this: #include ../git-compat-util.h #undef getitimer #undef setitimer int git_getitimer(int which, struct itimerval *value) See Documentation/CodingGuidelines for style nits. Will do and adjust code accordingly. Here I was more concerned about content though ;-) ... Worth being added to compat/, e.g. as setitimer.c, or, as itimer.c (as a by-product, it has getitimer() too)? If it helps your port, compat/itimer.c sounds like a good place. Doesn't it need a new header file to introduce structures and constants, too? You mean the ITIMER_* and struct itimerval, right? On NonStop these are available in sys/time.h, so here's no need to add them. Bye, Jojo -- 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: Porting git to HP NonStop
Joachim Schmitz j...@schmitz-digital.de writes: If it helps your port, compat/itimer.c sounds like a good place. Doesn't it need a new header file to introduce structures and constants, too? You mean the ITIMER_* and struct itimerval, right? On NonStop these are available in sys/time.h, so here's no need to add them. At least you would need a header to declare these two functions and make them visible so that the remainder of the codebase will not have to know about git_setitimer(), no? Or does your header files on NonStop declare setitimer() but does not implement it? As your proposed name is not compat/tandem.c but more generic sounding compat/itimer.c, we would have to plan for systems other than NonStop, so we may later have to introduce makefile variables to ask that header file to declare the structure and define the constants that are missing from such a system. While you are porting to NonStop, you may not have to define/declare them, but knowing that these files are the place to later do so is part of the planning. -- 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: Porting git to HP NonStop
-Original Message- From: Junio C Hamano [mailto:gits...@pobox.com] Sent: Wednesday, August 22, 2012 11:12 PM To: Joachim Schmitz Cc: git@vger.kernel.org; rsbec...@nexbridge.com Subject: Re: Porting git to HP NonStop Joachim Schmitz j...@schmitz-digital.de writes: If it helps your port, compat/itimer.c sounds like a good place. Doesn't it need a new header file to introduce structures and constants, too? You mean the ITIMER_* and struct itimerval, right? On NonStop these are available in sys/time.h, so here's no need to add them. At least you would need a header to declare these two functions and make them visible so that the remainder of the codebase will not have to know about git_setitimer(), no? Or does your header files on NonStop declare setitimer() but does not implement it? No it doesn't, at least not if a form visible to a compiler... As your proposed name is not compat/tandem.c but more generic sounding compat/itimer.c, we would have to plan for systems other than NonStop, so we may later have to introduce makefile variables to ask that header file to declare the structure and define the constants that are missing from such a system. While you are porting to NonStop, you may not have to define/declare them, but knowing that these files are the place to later do so is part of the planning. I thought of having the function decclaration in git-compat-util.h, just like for eg. setenv, gitmkdtemp, etc. Bye, Jojo -- 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: Porting git to HP NonStop
Joachim Schmitz j...@schmitz-digital.de writes: I thought of having the function decclaration in git-compat-util.h, just like for eg. setenv, gitmkdtemp, etc. Yeah, that's also fine, especially if you do not have to declare structures and constants. Once you start having to declare other things in order to declare the function missing on the system, it won't be like setenv where a pair of #ifdef NO_SETENV/#endif just surrounds a single line. At that point, a separate header file to hold them together would become easier to read. It's a judgement call; we'll see how it turns out (we do not have to get everything right in our first attempt). -- 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: Porting git to HP NonStop
From: Junio C Hamano [mailto:gits...@pobox.com]
Sent: Sunday, August 19, 2012 7:23 PM
To: Joachim Schmitz
Cc: 'Shawn Pearce'; git@vger.kernel.org; rsbec...@nexbridge.com
Subject: Re: Porting git to HP NonStop
Joachim Schmitz j...@schmitz-digital.de writes:
Found the problem: our mkdir(dir,flags) fails with ENOENT when dir
ends with a '/'.
Not sure whether this us a bug on out platform or just allowed by
POSIX and as such a wrong assumption in git though?
[shortly after]
A bit of googleing revealed that there is a GNUlib solution for this,
which claims that at least NetBSD 1.5.2 has the same problem.
(http://www.opensource.apple.com/source/gpatch/gpatch-2/patch/mkdir.c)
And apparently this has been discussed on the git mailing list too, 2
years
ago:
http://lists-archives.com/git/728359-git-s-use-of-mkdir-2.html,
there's a patch too.
Given that newer BSDs have fixed libc to accept directory name with a
trailing
slash, and that we use mkdir(2) in many places, I think the right way to
do so is
still what I suggested in that old thread in the last paragraph of my
message
http://thread.gmane.org/gmane.comp.version-
control.git/155812/focus=155876
That is, have compat/tandem.c and define a replacement mkdir(2) in a way
similar to how MinGW does so.
OK, I'll go for a compat/mkdir.c though.
We shouldn't call it tandem.c as Tandem, the Company, doesn't exist anymore
and since more than a decade (bough by Compaq, then HP), only the __TANDEM
survived in our compiler and headers/libraries. Could call it NonStop.c, but
I don't really like that idea either, I'd rather keep it more generic, just
in case someone else might need it too, or that issue someday gets fixed for
NonStop.
For now I've fixed it like this:
/usr/local/bin/diff -EBbu ./builtin/init-db.c.orig ./builtin/init-db.c
--- ./builtin/init-db.c.orig2012-08-19 03:55:50 -0500
+++ ./builtin/init-db.c 2012-08-19 03:39:57 -0500
@@ -25,7 +25,16 @@
static void safe_create_dir(const char *dir, int share) {
+#ifdef __TANDEM /* our mkdir() can't cope with a trailing '/' */
+ char mydir[PATH_MAX];
+
+ strcpy(mydir,dir);
+ if (dir[strlen(dir)-1] == '/')
+ mydir[strlen(dir)-1] = '\0';
+ if (mkdir(mydir, 0777) 0) {
+#else
if (mkdir(dir, 0777) 0) {
+#endif
Move that part inside #ifdef __TANDEM to define
int tandem_mkdir(const char *dir, mode_t mode)
{
...
}
I'll go for git_mkdir(), similar to other git wrappers, (like for mmap,
pread, fopen, snprintf, vsnprintf, qsort). Could call it gitmkdir() too
(like for basename, setenv, mkdtemp, mkstemps, unsetenv, strcasestr,
strlcpy, strtoumax, strtoimax, strtok_r, hstrerror, memmem, strchrnul,
memcpy), Opinions?
It seems the ones without the _ are for missing APIs and the ones with _
to wrap existing APIs (not sure about mmap and pread)?
Here it's current state:
$ cat compat/mkdir.c
#include ../git-compat-util.h
#undef mkdir
/* for platforms that can't deal with a trailing '/' */
int git_mkdir(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;
}
$
There is room for improvement though: it only removes one trailing slash. By
far not as advanced and generic as GNUlib's mkdir wrapper, but should be
good enough for git's usage.
in your new file compat/tandem.c, add
#ifdef __TANDEM
#define mkdir(a,b) tandem_mkdir((a), (b))
#endif
to git-compat-util.h
Again, git_mkdir, see above
and then add compat/tandem.o to COMPAT_OBJS in the
top-level Makefile.
For now I've added it to the (new) NOSTOP_KERNEL section.
We may want it to go along with some
MKDIR_DISLIKES_TRAILING_SLASH or MKDIR_BOGUS_TRAILING_SLASH some such.
Opinions, Ideas?
That way we do not have to keep an ugly platform specific ifdef in the
very
generic codepath.
Agreed, it was my quick and dirty fix for it.
Bye, Jojo
--
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: git on HP NonStop
From: Jan Engelhardt [mailto:jeng...@inai.de] Sent: Sunday, August 19, 2012 6:26 PM To: Joachim Schmitz Cc: 'Junio C Hamano'; git@vger.kernel.org Subject: RE: git on HP NonStop On Tuesday 2012-08-14 17:52, Joachim Schmitz wrote: @@ -98,6 +99,11 @@ #include stdlib.h #include stdarg.h #include string.h +#ifdef __TANDEM +# include strings.h /* for strcasecmp() */ + typedef int intptr_t; /* not int * ?!? */ + typedef unsigned int uintptr_t; /* not unsigned int * ?!? */ Of course not. intptr_t is an integral value capable of holding a pointer; it is not a pointer to int (because that would really be redundant to int*.) OK, thanks for the clarification. Another issue I stumbled across: There are numerous places (well, some 10) were something like the following is done int var = var; char *othervar = othervar; Here this leads to Compiler warnings 'variable var is used before its value is set' on NonStop. This self-initialization seems to be a GCC extension (?), but even gcc has a -Winit-self option to warn about this. Shouldn't that better be like the following? int var = 0; char *othervar = NULL; What is the reason for using that self-init stuff? I don't think it is really portable, is it? Bye, Jojo -- 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: git on HP NonStop
Am 8/20/2012 12:36, schrieb Joachim Schmitz: int var = var; char *othervar = othervar; ... What is the reason for using that self-init stuff? I don't think it is really portable, is it? It is used to avoid var may be used uninitialized warnings for some compilers. Officially (according to the C standard), it is undefined behavior. But I've observed considerable resistance by Junio to fix this properly. Therefore, unless you can show that your compiler generates unusable code you better live with the self-initialization warnings. -- Hannes -- 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: git on HP NonStop
From: Johannes Sixt [mailto:j.s...@viscovery.net] Sent: Monday, August 20, 2012 12:57 PM To: Joachim Schmitz Cc: 'Jan Engelhardt'; 'Junio C Hamano'; git@vger.kernel.org Subject: Re: git on HP NonStop Am 8/20/2012 12:36, schrieb Joachim Schmitz: int var = var; char *othervar = othervar; ... What is the reason for using that self-init stuff? I don't think it is really portable, is it? It is used to avoid var may be used uninitialized warnings for some compilers. Well, it results in a similar warning on NonStop. var is used before it is set and I think this is equally bad. In either case we don't know what the content of that var is. E.g. in wt_status.c the variable 'status' is set at only one place, but later it is switched on. If lucky we get to the default case and die. So why not just int status = 0; Officially (according to the C standard), it is undefined behavior. Yes, I had that suspicion. Not good to rely in this... But I've observed considerable resistance by Junio to fix this properly. What's the reason behind that? Therefore, unless you can show that your compiler generates unusable code you better live with the self-initialization warnings. So far I can't, so I guess I'll have to live with the warnings, but don't quite like it. -- Hannes Bye, Jojo -- 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: Porting git to HP NonStop
Joachim Schmitz j...@schmitz-digital.de writes: OK, I'll go for a compat/mkdir.c though. No. See below. We shouldn't call it tandem.c as Tandem, the Company, doesn't exist anymore and since more than a decade (bough by Compaq, then HP), only the __TANDEM survived in our compiler and headers/libraries. Could call it NonStop.c, but I don't really like that idea either, I'd rather keep it more generic, just in case someone else might need it too, or that issue someday gets fixed for NonStop. compat/hp_nonstop.c is also fine, but I think matching #ifdef __TANDEM is the most sensible. And I wouldn't call it just mkdir, as it is more likely than not that we will find other incompatibilities that needs to be absorbed in the compat/ layer, and we can add it to compat/tandem.c, but not to compat/mkdir.c, as that will be another nonstop specific tweak. A separate file, compat/tandem/mkdir.c, is fine, though. 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 understand trailing slash, which may not be specific to __TANDEM, but still is _not_ the only possible mode of breakage. Squatting on a generic git_mkdir() name makes it harder for other people to 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 we implement the whole thing kind, so it is in a different genre. -- 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: git on HP NonStop
Johannes Sixt j.s...@viscovery.net writes: Am 8/20/2012 12:36, schrieb Joachim Schmitz: int var = var; char *othervar = othervar; ... What is the reason for using that self-init stuff? I don't think it is really portable, is it? It is used to avoid var may be used uninitialized warnings for some compilers. Officially (according to the C standard), it is undefined behavior. But I've observed considerable resistance by Junio to fix this properly. I had resisted - int foo = foo; + int foo = 0; in the past. If some compiler is not seeing that foo is never used uninitialized, such a compiler will generate an unnecessary initialization, so it is not a _proper_ fix anyway (in fact, I do not think a proper fix exists, short of simplifying the code so that less sophisticated compilers can see that foo is never used uninitialized). So, no, I never resisted a proper fix. I resisted swapping an unsatisfactory workaround with another. Between the two unsatisfactory workarounds, the latter (explicit and unnecessary assignment to an innocuous value) is lessor of two evils, so I do not particularly mind it, though. Indeed, I think more recent history shows that we have such changes. -- 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: Porting git to HP NonStop
Joachim Schmitz j...@schmitz-digital.de 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 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. 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 understand trailing slash, which may not be specific to __TANDEM, but still is _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. Squatting on a generic git_mkdir() name makes it harder for other people to 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 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. -- 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: git on HP NonStop
From: Junio C Hamano [mailto:gits...@pobox.com]
Sent: Monday, August 20, 2012 6:30 PM
To: Johannes Sixt
Cc: Joachim Schmitz; 'Jan Engelhardt'; git@vger.kernel.org
Subject: Re: git on HP NonStop
Johannes Sixt j.s...@viscovery.net writes:
Am 8/20/2012 12:36, schrieb Joachim Schmitz:
int var = var;
char *othervar = othervar;
...
What is the reason for using that self-init stuff? I don't think it
is really portable, is it?
It is used to avoid var may be used uninitialized warnings for some
compilers. Officially (according to the C standard), it is undefined
behavior. But I've observed considerable resistance by Junio to fix
this properly.
I had resisted
- int foo = foo;
+ int foo = 0;
in the past. If some compiler is not seeing that foo is never used
uninitialized,
such a compiler will generate an unnecessary initialization, so it is not
a
_proper_ fix anyway (in fact, I do not think a proper fix exists, short of
simplifying the code so that less sophisticated compilers can see that
foo is
never used uninitialized).
So, no, I never resisted a proper fix. I resisted swapping an
unsatisfactory
workaround with another.
Between the two unsatisfactory workarounds, the latter (explicit and
unnecessary assignment to an innocuous value) is lessor of two evils, so I
do not
particularly mind it, though. Indeed, I think more recent history shows
that we
have such changes.
OK, so let's have a look at code, current git,
builtin/cat-file.c, line 196:
void *contents = contents;
This variable is set later in an if branch (if (print_contents == BATCH),
but not in the else branch. It is later used always under the same condition
as the one under which it is set.
Apparently is is malloc_d storage (there a free(content);), so there's no
harm al all in initializing it with NULL, even if it only appeases a stupid
compiler.
The next one, builtin/fast-export.c, line 486:
struct commit *commit = commit;
it is set in a switch statement, but not in every case, as far as I can see.
Hmm, maybe it is, and I just get lost in the code
And it is used directly after the switch, hopefully set to something
reasonable.
Why take the risk and not set it to NULL?
Next one, builtin/rev-list.c, line 390:
int reaches = reaches, all = all;
revs.commits = find_bisection(revs.commits, reaches, all,
bisect_find_all);
Seem pretty pointless to initialize them, provided find_bisection doesn't
read them. Does it?
I'm too Next one, lazy to check... I'd just set them to 0 and stop worrying.
Next one, fast-import.c, line 2268:
struct object_entry *oe = oe;
os gets set in en if and an else branch, but not in then intermediate else
if branch!
It is checked for !NULL later, so it should really get initialized to NULL
in the first place!
Same file, line 2437, same variable name, same story!
Same file, line 2616, variable e, it is used in an if branch but set after
that!
Same file again, line 2917, variable oe again. Same story as above.
Next file, ll-merge.c, line
static const struct ll_merge_options default_opts;
Somewhat different story here, compiler warning claims const variable
default_opts requires an initializer
Possible fix:
static const struct ll_merge_options default_opts = {0};
next file, match-trees.c, line 75ff:
const unsigned char *elem1 = elem1;
const unsigned char *elem2 = elem2;
const char *path1 = path1;
const char *path2 = path2;
unsigned mode1 = mode1;
unsigned mode2 = mode2;
Some get set, some not, depending on code path, but all get used, with
possibly bogus content.
Next file, merge-recursive.c, line 1903:
struct tree *mrtree = mrtree;
passed on my address to another function, which hopefully knows how to treat
it. It woult be learer and simpler to just have
struct tree *mrtree = NULL;
wouldn't it?
Next file, run-command.c, line 272:
int failed_errno = failed_errno;
Set deeply nested in some cases. Seems to be used reasonably, but again, why
take chanses= 0 is a goot errno ;-)
Next file, submodule.c, line 265:
struct commit *left = left, *right = right;
As far as I can see it is not set properly before it gets used in some
cases.
Next filen, transport.c, line 106:
int cmp = cmp, len;
I seem to see code paths whet it is used without being set properly
Next file, vcs-svn/svndiff.c, line 299 oh, that one has been fixed and
initialized to -1.
Next (and last) file, wt-status.c, line 267:
int status = status;
It apparently does get set properly before use. So here it once may once
again just make a compiler happy to set it to 0.
Bye, Jojo
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More
Re: git on HP NonStop
Joachim Schmitz j...@schmitz-digital.de writes: OK, so let's have a look at code, current git, builtin/cat-file.c, line 196: void *contents = contents; This variable is set later in an if branch (if (print_contents == BATCH), but not in the else branch. It is later used always under the same condition as the one under which it is set. Apparently is is malloc_d storage (there a free(content);), so there's no harm al all in initializing it with NULL, even if it only appeases a stupid compiler. It actually is harmful. See below. The next one, builtin/fast-export.c, line 486: struct commit *commit = commit; it is set in a switch statement, but not in every case, as far as I can see. Hmm, maybe it is, and I just get lost in the code And it is used directly after the switch, hopefully set to something reasonable. Why take the risk and not set it to NULL? Ditto. Next one, builtin/rev-list.c, line 390: int reaches = reaches, all = all; revs.commits = find_bisection(revs.commits, reaches, all, bisect_find_all); Seem pretty pointless to initialize them, provided find_bisection doesn't read them. Does it? That is why they are not initializations but marks to the compiler to signal you may be stupid enough to think they are used before initialized or assigned, but that is not the case. Initializing them would be pointless. Next one, fast-import.c, line 2268: struct object_entry *oe = oe; os gets set in en if and an else branch, but not in then intermediate else if branch! Look again. If the recent code is too complex for you to understand, go back to 10e8d68 (Correct compiler warnings in fast-import., 2007-02-06) and read the function. The control flow of the early part of that function dictates that either oe is assigned *or* inline_data is set to 1. When inline_data is false, oe is always set. The compiler was too stupid to read that, and that is why the (confusing) idiom to mark it for the stupid compiler was used. There are a few reasons why I do not think this self-assignment idiom or initializing the variable to an innocuous-looking random value is a particularly good thing to do when you see compiler warnings. If the compiler suspects the variable might be unused, you should always look at it and follow the flow yourself. Once you know it is a false alarm, you can use the idiom to squelch the warning, and it at the same serves as a note to others that you verified the flow and made sure it is a false warning. When the next person wants to touch the code, if the person knows the use of the idiom, it only serves as a warning to be extra careful not to introduce a new codepath that reads the variable without setting, as the compiler no longer helps him. If the person who touches the code is as clueless as the compiler and cannot follow the codepath to see the variable is never used uninitialized, the result will be a lot worse. That is the reason why I do not think the idiom to squelch the compiler is such a good thing. Careless people touch the code, so oe = oe initialization carefully placed in the original version does not necessarily stay as a useful documentation. But if you use oe = NULL as a way to squelch the warning in the first place, it is no better than oe = oe. In a sense, it is even worse, as it just looks like any other initialization and gives a false impression that the remainder of the code is written in such a way that it tolerates oe being NULL in any codepath, or there is some assignment before that before the code reaches places where oe cannot be NULL. That is different from what oe = oe initializaion documents---in the codepath protected by if (inline_data), it isn't just oe can safely be NULL there; instead it is oe can safely be *any* value there, because we don't use it. Of course, if you explicitly initialized oe to NULL, even if you introduce a codepath where oe cannot be NULL later, you won't get a warning from the compiler, so it is no better than oe = oe. And that is the reason why I do not think initialization to an innocuous-looking random value (e.g. NULL) is a good answer, either. When both are not good, replacing oe = oe with oe = NULL didn't make much sense, especially when the former _could_ be used better by more careful people. And that is the resistance J6t remembers. But when recent compilers started to warn oe = oe that itself is undefined, the equation changed. The idiom ceased to be a way to squelch the incorrect compiler warning (which was the primary point of its use---the documentation value is secondary, as what we document is we are squelching a false alarm, but we no longer are squelching anything). See 4a5de8d (vcs-svn: avoid self-assignment in dummy initialization of pre_off, 2012-06-01), and 58ebd98 (vcs-svn/svndiff.c: squelch false unused warning from gcc, 2012-01-27) that it updated, for
RE: Porting git to HP NonStop
From: Shawn Pearce [mailto:spea...@spearce.org]
Sent: Friday, August 10, 2012 7:38 PM
To: Joachim Schmitz
Cc: git@vger.kernel.org; rsbec...@nexbridge.com
Subject: Re: Porting git to HP NonStop
On Fri, Aug 10, 2012 at 10:32 AM, Joachim Schmitz
j...@schmitz-digital.de
wrote:
then use `git init --bare` in a new directory to copy in the
templates,
and see if
its the template copying code that is making an incorrect copy.
git init --bare gives the same error. It isn't copying any of the
subdirectories, only the file 'description'
Time to start debugging copy_templates_1 in builtin/init-db.c. :-(
Found the problem: our mkdir(dir,flags) fails with ENOENT when dir ends with
a '/'.
Not sure whether this us a bug on out platform or just allowed by POSIX and
as such a wrong assumption in git though?
[shortly after]
A bit of googleing revealed that there is a GNUlib solution for this, which
claims that at least NetBSD 1.5.2 has the same problem.
(http://www.opensource.apple.com/source/gpatch/gpatch-2/patch/mkdir.c)
And apparently this has been discussed on the git mailing list too, 2 years
ago:
http://lists-archives.com/git/728359-git-s-use-of-mkdir-2.html, there's a
patch too.
For now I've fixed it like this:
/usr/local/bin/diff -EBbu ./builtin/init-db.c.orig ./builtin/init-db.c
--- ./builtin/init-db.c.orig2012-08-19 03:55:50 -0500
+++ ./builtin/init-db.c 2012-08-19 03:39:57 -0500
@@ -25,7 +25,16 @@
static void safe_create_dir(const char *dir, int share)
{
+#ifdef __TANDEM /* our mkdir() can't cope with a trailing '/' */
+ char mydir[PATH_MAX];
+
+ strcpy(mydir,dir);
+ if (dir[strlen(dir)-1] == '/')
+ mydir[strlen(dir)-1] = '\0';
+ if (mkdir(mydir, 0777) 0) {
+#else
if (mkdir(dir, 0777) 0) {
+#endif
if (errno != EEXIST) {
perror(dir);
exit(1);
Bye, Jojo
--
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: git on HP NonStop
On Tuesday 2012-08-14 17:52, Joachim Schmitz wrote: @@ -98,6 +99,11 @@ #include stdlib.h #include stdarg.h #include string.h +#ifdef __TANDEM +# include strings.h /* for strcasecmp() */ + typedef int intptr_t; /* not int * ?!? */ + typedef unsigned int uintptr_t; /* not unsigned int * ?!? */ Of course not. intptr_t is an integral value capable of holding a pointer; it is not a pointer to int (because that would really be redundant to int*.) -- 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: Porting git to HP NonStop
Joachim Schmitz j...@schmitz-digital.de writes:
Found the problem: our mkdir(dir,flags) fails with ENOENT when dir ends with
a '/'.
Not sure whether this us a bug on out platform or just allowed by POSIX and
as such a wrong assumption in git though?
[shortly after]
A bit of googleing revealed that there is a GNUlib solution for this, which
claims that at least NetBSD 1.5.2 has the same problem.
(http://www.opensource.apple.com/source/gpatch/gpatch-2/patch/mkdir.c)
And apparently this has been discussed on the git mailing list too, 2 years
ago:
http://lists-archives.com/git/728359-git-s-use-of-mkdir-2.html, there's a
patch too.
Given that newer BSDs have fixed libc to accept directory name with
a trailing slash, and that we use mkdir(2) in many places, I think
the right way to do so is still what I suggested in that old thread
in the last paragraph of my message
http://thread.gmane.org/gmane.comp.version-control.git/155812/focus=155876
That is, have compat/tandem.c and define a replacement mkdir(2) in a
way similar to how MinGW does so.
For now I've fixed it like this:
/usr/local/bin/diff -EBbu ./builtin/init-db.c.orig ./builtin/init-db.c
--- ./builtin/init-db.c.orig2012-08-19 03:55:50 -0500
+++ ./builtin/init-db.c 2012-08-19 03:39:57 -0500
@@ -25,7 +25,16 @@
static void safe_create_dir(const char *dir, int share)
{
+#ifdef __TANDEM /* our mkdir() can't cope with a trailing '/' */
+ char mydir[PATH_MAX];
+
+ strcpy(mydir,dir);
+ if (dir[strlen(dir)-1] == '/')
+ mydir[strlen(dir)-1] = '\0';
+ if (mkdir(mydir, 0777) 0) {
+#else
if (mkdir(dir, 0777) 0) {
+#endif
Move that part inside #ifdef __TANDEM to define
int tandem_mkdir(const char *dir, mode_t mode)
{
...
}
in your new file compat/tandem.c, add
#ifdef __TANDEM
#define mkdir(a,b) tandem_mkdir((a), (b))
#endif
to git-compat-util.h and then add compat/tandem.o to COMPAT_OBJS in
the top-level Makefile.
That way we do not have to keep an ugly platform specific ifdef in
the very generic codepath.
if (errno != EEXIST) {
perror(dir);
exit(1);
Bye, Jojo
--
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: Porting git to HP NonStop
From: Joachim Schmitz [mailto:j...@schmitz-digital.de] Sent: Friday, August 10, 2012 10:09 PM To: 'Shawn Pearce' Cc: 'git@vger.kernel.org'; 'rsbec...@nexbridge.com' Subject: RE: Porting git to HP NonStop From: Joachim Schmitz [mailto:j...@schmitz-digital.de] Sent: Friday, August 10, 2012 7:33 PM To: 'Shawn Pearce' Cc: 'git@vger.kernel.org'; 'rsbec...@nexbridge.com' Subject: RE: Porting git to HP NonStop From: Shawn Pearce [mailto:spea...@spearce.org] Sent: Friday, August 10, 2012 6:28 PM To: Joachim Schmitz Cc: git@vger.kernel.org; rsbec...@nexbridge.com Subject: Re: Porting git to HP NonStop On Fri, Aug 10, 2012 at 8:04 AM, Joachim Schmitz j...@schmitz-digital.de wrote: snip - HP NonStop doesn't have stat.st_?time.nsec, there are several places what an #ifdef USE_NSEC is missing, I can provide a diff if needed (offending files: builtin/fetch-pack.c and read-cache.c). I think this would be appreciated by anyone else that has a similar problem where the platform lacks nsec. Will do. OK, here we go: /usr/local/bin/diff -EBbu ./builtin/fetch-pack.c.orig ./builtin/fetch-pack.c snip Sorry, this is not needed if I just set NO_NSEC, so just forget about it (and thanks to Junio for telling be) /usr/local/bin/diff -EBbu ./git-compat-util.h.orig ./git-compat-util.h --- ./git-compat-util.h.orig2012-07-30 15:50:38 -0500 +++ ./git-compat-util.h 2012-08-10 09:59:56 -0500 @@ -74,7 +74,8 @@ # define _XOPEN_SOURCE 500 # endif #elif !defined(__APPLE__) !defined(__FreeBSD__) !defined(__USLC__) \ - !defined(_M_UNIX) !defined(__sgi) !defined(__DragonFly__) + !defined(_M_UNIX) !defined(__sgi) !defined(__DragonFly__) \ + !defined(__TANDEM) #define _XOPEN_SOURCE 600 /* glibc2 and AIX 5.3L need 500, OpenBSD needs 600 for S_ISLNK() */ #define _XOPEN_SOURCE_EXTENDED 1 /* AIX 5.3L needs this */ #endif @@ -98,6 +99,11 @@ #include stdlib.h #include stdarg.h #include string.h +#ifdef __TANDEM +# include strings.h /* for strcasecmp() */ + typedef long int intptr_t; + typedef unsigned long int uintptr_t; +#endif #include errno.h #include limits.h #include sys/param.h This one still stands though, unless someone can come up with a better idea? Bye, Jojo -- 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: Porting git to HP NonStop
Joachim Schmitz j...@schmitz-digital.de writes: /usr/local/bin/diff -EBbu ./git-compat-util.h.orig ./git-compat-util.h --- ./git-compat-util.h.orig2012-07-30 15:50:38 -0500 +++ ./git-compat-util.h 2012-08-10 09:59:56 -0500 @@ -74,7 +74,8 @@ # define _XOPEN_SOURCE 500 # endif #elif !defined(__APPLE__) !defined(__FreeBSD__) !defined(__USLC__) \ - !defined(_M_UNIX) !defined(__sgi) !defined(__DragonFly__) + !defined(_M_UNIX) !defined(__sgi) !defined(__DragonFly__) \ + !defined(__TANDEM) #define _XOPEN_SOURCE 600 /* glibc2 and AIX 5.3L need 500, OpenBSD needs 600 for S_ISLNK() */ #define _XOPEN_SOURCE_EXTENDED 1 /* AIX 5.3L needs this */ #endif @@ -98,6 +99,11 @@ #include stdlib.h #include stdarg.h #include string.h +#ifdef __TANDEM +# include strings.h /* for strcasecmp() */ + typedef long int intptr_t; + typedef unsigned long int uintptr_t; +#endif #include errno.h #include limits.h #include sys/param.h This one still stands though, unless someone can come up with a better idea? This hunk looks unobtrusive and obviously will not impact other platforms, which is good. -- 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: git on HP NonStop
From: Junio C Hamano [mailto:gits...@pobox.com]
Sent: Tuesday, August 14, 2012 4:44 PM
To: Joachim Schmitz
Subject: Re: git on HP NonStop
Joachim Schmitz j...@schmitz-digital.de writes:
Interesting, I never mentioned Tandem did I, But still you recognized
HP NonStop as that.
No, *you* did in your patch #ifdef.
Ah, I see.
Well, I do care about that platform, but if you don't, there's not
much point in me trying to get Tandem specific patches applied, is it?
As long as the change is isolated (i.e. compilation without #define
TANDEM
__TANDEM actually
for other people will produce byte-for-byte identical result as before),
and
cleanly made (i.e. the resulting source code is not littered with #ifdef
TANDEM in many places), I do not think there is a reason not to have such
a
patchset.
It isn't in many places, only 2 places in git-compat-util.h so far:
/usr/local/bin/diff -EBbu ./git-compat-util.h.orig ./git-compat-util.h
--- ./git-compat-util.h.orig2012-07-30 15:50:38 -0500
+++ ./git-compat-util.h 2012-08-12 11:26:46 -0500
@@ -74,7 +74,8 @@
# define _XOPEN_SOURCE 500
# endif
#elif !defined(__APPLE__) !defined(__FreeBSD__) !defined(__USLC__)
\
- !defined(_M_UNIX) !defined(__sgi) !defined(__DragonFly__)
+ !defined(_M_UNIX) !defined(__sgi) !defined(__DragonFly__) \
+ !defined(__TANDEM)
#define _XOPEN_SOURCE 600 /* glibc2 and AIX 5.3L need 500, OpenBSD needs
600 for S_ISLNK() */
#define _XOPEN_SOURCE_EXTENDED 1 /* AIX 5.3L needs this */
#endif
@@ -98,6 +99,11 @@
#include stdlib.h
#include stdarg.h
#include string.h
+#ifdef __TANDEM
+# include strings.h /* for strcasecmp() */
+ typedef int intptr_t; /* not int * ?!? */
+ typedef unsigned int uintptr_t; /* not unsigned int * ?!? */
+#endif
#include errno.h
#include limits.h
#include sys/param.h
Too much? The 2nd part is not necessary NonStop specific, any idea for a
better way?
There there's Makefile:
/usr/local/bin/diff -EBbu ./Makefile.orig ./Makefile
--- ./Makefile.orig 2012-07-30 15:50:38 -0500
+++ ./Makefile 2012-08-14 06:07:16 -0500
@@ -1297,6 +1297,45 @@
NO_CURL =
NO_EXPAT =
endif
+ifeq ($(uname_S),NONSTOP_KERNEL)
+ CC = cc -c99 # needs some C99 features, inline is just one of them
+ CFLAGS= -g -O
+ prefix = /usr/local
+
+ # as detected by './configure'
+ #NO_CURL = YesPlease # missdetected, disabled, see below
+ NEEDS_SSL_WITH_CURL = YesPlease # added manually, see above
+ HAVE_LIBCHARSET_H=YesPlease
+ NEEDS_LIBICONV = YesPlease # needs libiconv first, changed further
down
+ NO_SYS_SELECT_H=UnfortunatelyYes
+ NO_D_TYPE_IN_DIRENT = YesPlease
+ NO_HSTRERROR=YesPlease
+ NO_STRCASESTR=YesPlease
+ NO_FNMATCH_CASEFOLD = YesPlease
+ NO_MEMMEM = YesPlease
+ NO_STRLCPY = YesPlease
+ NO_SETENV = YesPlease
+ NO_UNSETENV = YesPlease
+ NO_MKDTEMP = YesPlease
+ NO_MKSTEMPS = YesPlease
+ OLD_ICONV=UnfortunatelyYes # currently libiconv-1.9.1
+ NO_REGEX=YesPlease # Why? ToDo?
+ NO_PTHREADS=UnfortunatelyYes # ToDo? Using PUT, maybe?
+
+ # our's are in ${prefix}/bin
+ PERL_PATH = ${prefix}/bin/perl
+ PYTHON_PATH = ${prefix}/bin/python
+
+ # not detected (nor checked for) by './configure'
+ COMPAT_CFLAGS += -DSA_RESTART=0 # we don't have SA_RESTART on
NonStop
+ COMPAT_CFLAGS += -DHAVE_STRING_H=1 # needed in
compat/fnmatch/fnmatch.c
+ NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease # we don't have that on
NonStop
+ NO_NSEC = YesPlease # we don't have that on NonStop
+ NO_PREAD = YesPlease # we could use floss_pread though?
+ NO_MMAP = YesPlease # we could use floss_mmap though?
+ # newly implemented further down
+ NO_POLL = YesPlease # we could use floss_poll though?
+endif
ifneq (,$(findstring MINGW,$(uname_S)))
pathsep = ;
NO_PREAD = YesPlease
@@ -1526,6 +1565,11 @@
LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto
endif
endif
+ifndef NO_GETTEXT
+ifndef LIBC_CONTAINS_LIBINTL
+ EXTLIBS += -lintl
+endif
+endif
ifdef NEEDS_LIBICONV
ifdef ICONVDIR
BASIC_CFLAGS += -I$(ICONVDIR)/include
@@ -1538,11 +1582,6 @@
ifdef NEEDS_LIBGEN
EXTLIBS += -lgen
Endif
-ifndef NO_GETTEXT
-ifndef LIBC_CONTAINS_LIBINTL
- EXTLIBS += -lintl
-endif
-endif
ifdef NEEDS_SOCKET
EXTLIBS += -lsocket
endif
@@ -1591,6 +1630,11 @@
BASIC_CFLAGS += -DNO_GETTEXT
USE_GETTEXT_SCHEME ?= fallthrough
endif
+ifdef NO_POLL
+ NO_SYS_POLL_H = YesPlease
+ COMPAT_CFLAGS += -DNO_POLL -Icompat/win32 # so it finds poll.h
+ COMPAT_OBJS += compat/win32/poll.o
+endif
ifdef NO_STRCASESTR
COMPAT_CFLAGS += -DNO_STRCASESTR
COMPAT_OBJS += compat/strcasestr.o
How das that look to you?
Finally, I would prefer to see any message that is addressed to me as
the project maintainer to be Cc'ed to the list. I feel 80% of my
Re: Porting git to HP NonStop
Am 10.08.2012 18:27, schrieb Shawn Pearce: There is no need to define your own mmap(). Define NO_MMAP=1 in the Makefile. Git already has its own fake mmap and knows how to write it back to disk when making changes. Or better to say: the fake mmap has functionality that is sufficient for git. In particular, it does *not* write back changes to disk (it supports only MAP_PRIVATE), and the mapped area does not change if the file is changed by a third party. -- Hannes -- 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: Porting git to HP NonStop
From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On Behalf Of Joachim Schmitz Sent: Friday, August 10, 2012 5:01 PM To: git@vger.kernel.org Cc: rsbec...@nexbridge.com Subject: RE: [PATCH v2] add tests for 'git rebase --keep-empty' Hi folks I'm a brand new subscriper of this mailing list, so please forgive if I violate some protocol or talk about things that had been discussed to death earlier. Ahrgl, 1st mistake: wrong subject, sorry I'm currently in the process of porting git (1.7.11.4 for now) to the HP NonStop platform and found several issues: - HP NonStop is lacking poll(), git is making quite some use of it. My Solution: I 'stole' the implementation from GNUlib, which implements poll() using select(). Git should either provide its own poll(), not use it at all or resort to using GNUlib, what do you think?. - HP NonStop is lacking getrlimit(), fsync(), setitimer() and memory mapped IO. For now I've commented out the part that used getrlimit() and use a home brewed implementation for fsync(), setitimer() and mmap(). - git makes use of some C99 features or at least feature that are not availabe in C89, like 'inline' C89 is the default compiler on HP NonStop, but we also habe a c99 compiler, so telling configure to search for c99 should help here. - libintl and libiconv sem to get linked in the wrong order, resulting in unresolved symbols. I've just moved the ifndef NO_GETTEXT section of Makefile to above the ifdef NEEDS_LIBICONF section. - HP NonStop doesn't have stat.st_blocks, this is used in builtin/count-objects.c around line 45, not sure yet how to fix that. - HP NonStop doesn't have stat.st_?time.nsec, there are several places what an #ifdef USE_NSEC is missing, I can provide a diff if needed (offending files: builtin/fetch-pack.c and read-cache.c). - HP NonStop doesn't know SA_RESTART I fixed that with a #define SA_RESTART 0 in the 3 files affected (builtin/log.c, fast-import.c and progress.c) - using C99 but not using #include strings.h results in compiler errors due to a missing prototype for strcasecmp() I fixed it by adding that to git-compat- util.h - HP NonStop doesn't have intptr_t and uintpr_t (in its stdint.h) I added them to git-compat-util.h - HP NonStop doesn't need the #define _XOPEN_SOURCE 600, just like __APPLE__, __FreeBSD__ etc, so I added a !defined(__TANDEM) in git- compat-util.h - there seems to be an issue with compat/fnmatch/fnmatch.c not including string.h, seems that HAVE_STRING_H is not #define'd anywhere. - Once compiled and installed, a simple jojo@\hpitug:/home/jojo/GitHub $ git clone git://github.com/git/git.git fails with: /home/jojo/GitHub/git/.git/branches/: No such file or directory After creating those manually it fails because the directory isn't empty, catch-22 After some trial'n'error I found that the culprit seems to be the subdirectories branches, hook and info in /usr/local/share/git-core/templates/, if I remove/rename those, the above command works fine. I have no idea why that is nor how to properly fix it, anyone out there? Bye, Jojo -- 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: Porting git to HP NonStop
On Fri, Aug 10, 2012 at 8:04 AM, Joachim Schmitz j...@schmitz-digital.de wrote: - HP NonStop is lacking poll(), git is making quite some use of it. My Solution: I 'stole' the implementation from GNUlib, which implements poll() using select(). Git should either provide its own poll(), not use it at all or resort to using GNUlib, what do you think?. poll() is usually better than select() because you don't need to worry about FD_SETSIZE. That said, the compat/ directory contains implementations of some functions. You could contribute a fake poll that uses select if it was under the GPLv2. - HP NonStop is lacking getrlimit(), fsync(), setitimer() and memory mapped IO. For now I've commented out the part that used getrlimit() and use a home brewed implementation for fsync(), setitimer() and mmap(). There is no need to define your own mmap(). Define NO_MMAP=1 in the Makefile. Git already has its own fake mmap and knows how to write it back to disk when making changes. - git makes use of some C99 features or at least feature that are not availabe in C89, like 'inline' C89 is the default compiler on HP NonStop, but we also habe a c99 compiler, so telling configure to search for c99 should help here. You could also disable inline by #define inline /**/, but this will probably result in a slower binary. - HP NonStop doesn't have stat.st_blocks, this is used in builtin/count-objects.c around line 45, not sure yet how to fix that. IIRC the block count is only used to give the user some notion of how much disk was wasted by the repository. You could hack a macro that redefines this as st_size. - HP NonStop doesn't have stat.st_?time.nsec, there are several places what an #ifdef USE_NSEC is missing, I can provide a diff if needed (offending files: builtin/fetch-pack.c and read-cache.c). I think this would be appreciated by anyone else that has a similar problem where the platform lacks nsec. - Once compiled and installed, a simple jojo@\hpitug:/home/jojo/GitHub $ git clone git://github.com/git/git.git fails with: /home/jojo/GitHub/git/.git/branches/: No such file or directory After creating those manually it fails because the directory isn't empty, catch-22 After some trial'n'error I found that the culprit seems to be the subdirectories branches, hook and info in /usr/local/share/git-core/templates/, if I remove/rename those, the above command works fine. I have no idea why that is nor how to properly fix it, anyone out there? This sounds like the templates directory was not created correctly during installation, or is being copied incorrectly during the git init process. I would start by comparing the structure and permissions of the templates directory on your HP NonStop system to one on a Linux system and see if there was a mistake made during the installation process. If the directory matches, I would then use `git init --bare` in a new directory to copy in the templates, and see if its the template copying code that is making an incorrect copy. -- 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: Porting git to HP NonStop
On Fri, Aug 10, 2012 at 10:32 AM, Joachim Schmitz j...@schmitz-digital.de wrote: then use `git init --bare` in a new directory to copy in the templates, and see if its the template copying code that is making an incorrect copy. git init --bare gives the same error. It isn't copying any of the subdirectories, only the file 'description' Time to start debugging copy_templates_1 in builtin/init-db.c. :-( -- 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: Porting git to HP NonStop
From: Joachim Schmitz [mailto:j...@schmitz-digital.de]
Sent: Friday, August 10, 2012 7:33 PM
To: 'Shawn Pearce'
Cc: 'git@vger.kernel.org'; 'rsbec...@nexbridge.com'
Subject: RE: Porting git to HP NonStop
From: Shawn Pearce [mailto:spea...@spearce.org]
Sent: Friday, August 10, 2012 6:28 PM
To: Joachim Schmitz
Cc: git@vger.kernel.org; rsbec...@nexbridge.com
Subject: Re: Porting git to HP NonStop
On Fri, Aug 10, 2012 at 8:04 AM, Joachim Schmitz
j...@schmitz-digital.de
wrote:
snip
- HP NonStop doesn't have stat.st_blocks, this is used in
builtin/count-objects.c
around line 45, not sure yet how to fix that.
IIRC the block count is only used to give the user some notion of how
much disk was wasted by the repository. You could hack a macro that
redefines this as st_size.
OK, thanks, will try that.
Setting NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease in Makefile helps, no need
for further hacking ;-).
- HP NonStop doesn't have stat.st_?time.nsec, there are several
places
what an
#ifdef USE_NSEC is missing, I can provide a diff if needed
(offending
files: builtin/fetch-pack.c and read-cache.c).
I think this would be appreciated by anyone else that has a similar
problem where the platform lacks nsec.
Will do.
OK, here we go:
/usr/local/bin/diff -EBbu ./builtin/fetch-pack.c.orig ./builtin/fetch-pack.c
--- ./builtin/fetch-pack.c.orig 2012-07-30 15:50:38 -0500
+++ ./builtin/fetch-pack.c 2012-08-10 01:50:28 -0500
@@ -1096,7 +1096,9 @@
int fd;
mtime.sec = st.st_mtime;
+#ifdef USE_NSEC
mtime.nsec = ST_MTIME_NSEC(st);
+#endif
if (stat(shallow, st)) {
if (mtime.sec)
die(shallow file was removed during
fetch);
/usr/local/bin/diff -EBbu ./read-cache.c.orig ./read-cache.c
--- ./read-cache.c.orig 2012-07-30 15:50:38 -0500
+++ ./read-cache.c 2012-08-09 10:57:57 -0500
@@ -72,8 +72,10 @@
{
ce-ce_ctime.sec = (unsigned int)st-st_ctime;
ce-ce_mtime.sec = (unsigned int)st-st_mtime;
+#ifdef USE_NSEC
ce-ce_ctime.nsec = ST_CTIME_NSEC(*st);
ce-ce_mtime.nsec = ST_MTIME_NSEC(*st);
+#endif
ce-ce_dev = st-st_dev;
ce-ce_ino = st-st_ino;
ce-ce_uid = st-st_uid;
@@ -1465,7 +1467,9 @@
}
strbuf_release(previous_name_buf);
istate-timestamp.sec = st.st_mtime;
+#ifdef USE_NSEC
istate-timestamp.nsec = ST_MTIME_NSEC(st);
+#endif
while (src_offset = mmap_size - 20 - 8) {
/* After an array of active_nr index entries,
@@ -1821,7 +1825,9 @@
if (ce_flush(c, newfd) || fstat(newfd, st))
return -1;
istate-timestamp.sec = (unsigned int)st.st_mtime;
+#ifdef USE_NSEC
istate-timestamp.nsec = ST_MTIME_NSEC(st);
+#endif
return 0;
}
Hope this helps?
Could you also consider adding the following:
/usr/local/bin/diff -EBbu ./git-compat-util.h.orig ./git-compat-util.h
--- ./git-compat-util.h.orig2012-07-30 15:50:38 -0500
+++ ./git-compat-util.h 2012-08-10 09:59:56 -0500
@@ -74,7 +74,8 @@
# define _XOPEN_SOURCE 500
# endif
#elif !defined(__APPLE__) !defined(__FreeBSD__) !defined(__USLC__)
\
- !defined(_M_UNIX) !defined(__sgi) !defined(__DragonFly__)
+ !defined(_M_UNIX) !defined(__sgi) !defined(__DragonFly__) \
+ !defined(__TANDEM)
#define _XOPEN_SOURCE 600 /* glibc2 and AIX 5.3L need 500, OpenBSD needs
600 for S_ISLNK() */
#define _XOPEN_SOURCE_EXTENDED 1 /* AIX 5.3L needs this */
#endif
@@ -98,6 +99,11 @@
#include stdlib.h
#include stdarg.h
#include string.h
+#ifdef __TANDEM
+# include strings.h /* for strcasecmp() */
+ typedef long int intptr_t;
+ typedef unsigned long int uintptr_t;
+#endif
#include errno.h
#include limits.h
#include sys/param.h
Bye, Jojo
--
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
