RE: Porting git to HP NonStop

RE: Porting git to HP NonStop

[Joachim Schmitz]

> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On
> Behalf Of Joachim Schmitz
> Sent: Friday, August 10, 2012 5:01 PM
> To: git@vger.kernel.org
> Cc: rsbecker@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

Re: Porting git to HP NonStop

[Shawn Pearce]

On Fri, Aug 10, 2012 at 8:04 AM, Joachim Schmitz
<jojo@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.

RE: Porting git to HP NonStop

[Joachim Schmitz]

> From: Shawn Pearce [mailto:spearce@spearce.org]
> Sent: Friday, August 10, 2012 6:28 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org; rsbecker@nexbridge.com
> Subject: Re: Porting git to HP NonStop
> 
> On Fri, Aug 10, 2012 at 8:04 AM, Joachim Schmitz <jojo@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.

This is what I did. Just to see now that compat/win32/poll.c has exacly the
same stuff...
 
> >> - 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.

Ah, excellent. Esp. as our home brewed implementation is pretty primitive.

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

Even our C99 compiler doesn't inline, it merly recognizes the keyword and
then warns about (unable to inline...)
But there are other C99 features used too.

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

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

jojo@\hpitug:/usr/local/share/git-core/templates $ ls -laR
total 41
drwxr-xr-x    1 SUPER.SUPER        SUPER       4096 Aug 10 12:10 .
drwxr-xr-x    1 SUPER.SUPER        SUPER       4096 Aug 10 08:19 ..
drwxr-xr-x    1 SUPER.SUPER        SUPER       4096 Aug 10 07:26 branches
drwxr-xr-x    1 SUPER.SUPER        SUPER       4096 Aug 10 07:26 hooks
drwxr-xr-x    1 SUPER.SUPER        SUPER       4096 Aug 10 07:26 info
-rw-r--r--    1 SUPER.SUPER        SUPER         73 Aug 10 07:26 description

./branches:
total 16
drwxr-xr-x    1 SUPER.SUPER        SUPER       4096 Aug 10 07:26 .
drwxr-xr-x    1 SUPER.SUPER        SUPER       4096 Aug 10 12:10 ..

./hooks:
total 43
drwxr-xr-x    1 SUPER.SUPER        SUPER       4096 Aug 10 07:26 .
drwxr-xr-x    1 SUPER.SUPER        SUPER       4096 Aug 10 12:10 ..
-rwxr-xr-x    1 SUPER.SUPER        SUPER        452 Aug 10 07:26
applypatch-msg.sample
-rwxr-xr-x    1 SUPER.SUPER        SUPER        896 Aug 10 07:26
commit-msg.sample
-rwxr-xr-x    1 SUPER.SUPER        SUPER        189 Aug 10 07:26
post-update.sample
-rwxr-xr-x    1 SUPER.SUPER        SUPER        398 Aug 10 07:26
pre-applypatch.sample
-rwxr-xr-x    1 SUPER.SUPER        SUPER       1704 Aug 10 07:26
pre-commit.sample
-rwxr-xr-x    1 SUPER.SUPER        SUPER       4957 Aug 10 07:26
pre-rebase.sample
-rwxr-xr-x    1 SUPER.SUPER        SUPER       1251 Aug 10 07:26
prepare-commit-msg.sample
-rwxr-xr-x    1 SUPER.SUPER        SUPER       3611 Aug 10 07:26
update.sample

./info:
total 17
drwxr-xr-x    1 SUPER.SUPER        SUPER       4096 Aug 10 07:26 .
drwxr-xr-x    1 SUPER.SUPER        SUPER       4096 Aug 10 12:10 ..
-rw-r--r--    1 SUPER.SUPER        SUPER        240 Aug 10 07:26 exclude
jojo@\hpitug:/usr/local/share/git-core/templates $

SUPER.SUPER on NonStop is equivalent to root in UNIX. Everything is readable
to everybody. Looks OK to me?

> 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'

Bye, Jojo

Re: Porting git to HP NonStop

[Shawn Pearce]

On Fri, Aug 10, 2012 at 10:32 AM, Joachim Schmitz
<jojo@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. :-(

RE: Porting git to HP NonStop

[Joachim Schmitz]

> From: Joachim Schmitz [mailto:jojo@schmitz-digital.de]
> Sent: Friday, August 10, 2012 7:33 PM
> To: 'Shawn Pearce'
> Cc: 'git@vger.kernel.org'; 'rsbecker@nexbridge.com'
> Subject: RE: Porting git to HP NonStop
> 
> > From: Shawn Pearce [mailto:spearce@spearce.org]
> > Sent: Friday, August 10, 2012 6:28 PM
> > To: Joachim Schmitz
> > Cc: git@vger.kernel.org; rsbecker@nexbridge.com
> > Subject: Re: Porting git to HP NonStop
> >
> > On Fri, Aug 10, 2012 at 8:04 AM, Joachim Schmitz
> > <jojo@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.orig    2012-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

Re: Porting git to HP NonStop

[Johannes Sixt]

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

RE: Porting git to HP NonStop

[Joachim Schmitz]

> From: Joachim Schmitz [mailto:jojo@schmitz-digital.de]
> Sent: Friday, August 10, 2012 10:09 PM
> To: 'Shawn Pearce'
> Cc: 'git@vger.kernel.org'; 'rsbecker@nexbridge.com'
> Subject: RE: Porting git to HP NonStop
> 
> > From: Joachim Schmitz [mailto:jojo@schmitz-digital.de]
> > Sent: Friday, August 10, 2012 7:33 PM
> > To: 'Shawn Pearce'
> > Cc: 'git@vger.kernel.org'; 'rsbecker@nexbridge.com'
> > Subject: RE: Porting git to HP NonStop
> >
> > > From: Shawn Pearce [mailto:spearce@spearce.org]
> > > Sent: Friday, August 10, 2012 6:28 PM
> > > To: Joachim Schmitz
> > > Cc: git@vger.kernel.org; rsbecker@nexbridge.com
> > > Subject: Re: Porting git to HP NonStop
> > >
> > > On Fri, Aug 10, 2012 at 8:04 AM, Joachim Schmitz
> > > <jojo@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.orig    2012-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

Re: Porting git to HP NonStop

[Junio C Hamano]

"Joachim Schmitz" <jojo@schmitz-digital.de> writes:

>> /usr/local/bin/diff -EBbu ./git-compat-util.h.orig ./git-compat-util.h
>> --- ./git-compat-util.h.orig    2012-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.

RE: Porting git to HP NonStop

[Joachim Schmitz]

> From: Shawn Pearce [mailto:spearce@spearce.org]
> Sent: Friday, August 10, 2012 7:38 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org; rsbecker@nexbridge.com
> Subject: Re: Porting git to HP NonStop
> 
> On Fri, Aug 10, 2012 at 10:32 AM, Joachim Schmitz
<jojo@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.orig    2012-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 

Re: Porting git to HP NonStop

[Junio C Hamano]

"Joachim Schmitz" <jojo@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.orig    2012-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 

RE: Porting git to HP NonStop

[Joachim Schmitz]

> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Sunday, August 19, 2012 7:23 PM
> To: Joachim Schmitz
> Cc: 'Shawn Pearce'; git@vger.kernel.org; rsbecker@nexbridge.com
> Subject: Re: Porting git to HP NonStop
> 
> "Joachim Schmitz" <jojo@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.orig    2012-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

Re: Porting git to HP NonStop

[Junio C Hamano]

"Joachim Schmitz" <jojo@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.

RE: Porting git to HP NonStop

[Joachim Schmitz]

> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Monday, August 20, 2012 4:42 PM
> To: Joachim Schmitz
> Cc: 'Shawn Pearce'; git@vger.kernel.org; rsbecker@nexbridge.com
> Subject: Re: Porting git to HP NonStop
> 
> "Joachim Schmitz" <jojo@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.

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

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

Bye, Jojo

Re: Porting git to HP NonStop

[Junio C Hamano]

"Joachim Schmitz" <jojo@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.

RE: Porting git to HP NonStop

[Joachim Schmitz]

> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Monday, August 20, 2012 6:54 PM
> To: Joachim Schmitz
> Cc: 'Shawn Pearce'; git@vger.kernel.org; rsbecker@nexbridge.com
> Subject: Re: Porting git to HP NonStop
> 
> "Joachim Schmitz" <jojo@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.c    2012-08-21 05:02:11 -0500
@@ -0,0 +1,24 @@
+#include "../git-compat-util.h"
+#undef mkdir
+
+/* for platforms that can't deal with a trailing '/' */
+int compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode)
+{
+       int retval;
+       char *tmp_dir = NULL;
+       size_t len = strlen(dir);
+
+       if (len && dir[len-1] == '/') {
+               if ((tmp_dir = strdup(dir)) == NULL)
+                       return -1;
+               tmp_dir[len-1] = '\0';
+       }
+       else
+               tmp_dir = (char *)dir;
+
+       retval = mkdir(tmp_dir, mode);
+       if (tmp_dir != dir)
+               free(tmp_dir);
+
+       return retval;
+}

BTW: I've just today reported that mkdir bug to HP NonStop development.
Let's see how fast they fix it and whether at all, but I'd be pretty
surprised if that happened earlier than 6 months from now.

Bye, Jojo

Re: Porting git to HP NonStop

[Brandon Casey]

On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz
<jojo@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.c    2012-08-21 05:02:11 -0500
> @@ -0,0 +1,24 @@
> +#include "../git-compat-util.h"
> +#undef mkdir
> +
> +/* for platforms that can't deal with a trailing '/' */
> +int compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode)
> +{
> +       int retval;
> +       char *tmp_dir = NULL;
> +       size_t len = strlen(dir);
> +
> +       if (len && dir[len-1] == '/') {
> +               if ((tmp_dir = strdup(dir)) == NULL)
> +                       return -1;
> +               tmp_dir[len-1] = '\0';
> +       }
> +       else
> +               tmp_dir = (char *)dir;
> +
> +       retval = mkdir(tmp_dir, mode);
> +       if (tmp_dir != dir)
> +               free(tmp_dir);
> +
> +       return retval;
> +}

Why not rearrange this so that you assign to dir the value of tmp_dir
and then just pass dir to mkdir.  Then you can avoid the recast of dir
to (char*) in the else branch.  Later, just call free(tmp_dir).  Also,
we have xstrndup.  So I think the body of your function can become
something like:

   if (len && dir[len-1] == '/')
       dir = tmp_dir = xstrndup(dir, len-1);

   retval = mkdir(dir, mode);
   free(tmp_dir);

-Brandon

Re: Porting git to HP NonStop

[Brandon Casey]

On Wed, Aug 22, 2012 at 10:00 AM, Brandon Casey <drafnel@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

RE: Porting git to HP NonStop

[Joachim Schmitz]

> From: Brandon Casey [mailto:drafnel@gmail.com]
> Sent: Wednesday, August 22, 2012 7:01 PM
> To: Joachim Schmitz
> Cc: Junio C Hamano; Shawn Pearce; git@vger.kernel.org;
> rsbecker@nexbridge.com
> Subject: Re: Porting git to HP NonStop
> 
> On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz <jojo@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.c    2012-08-21 05:02:11 -0500
> > @@ -0,0 +1,24 @@
> > +#include "../git-compat-util.h"
> > +#undef mkdir
> > +
> > +/* for platforms that can't deal with a trailing '/' */ int
> > +compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode) {
> > +       int retval;
> > +       char *tmp_dir = NULL;
> > +       size_t len = strlen(dir);
> > +
> > +       if (len && dir[len-1] == '/') {
> > +               if ((tmp_dir = strdup(dir)) == NULL)
> > +                       return -1;
> > +               tmp_dir[len-1] = '\0';
> > +       }
> > +       else
> > +               tmp_dir = (char *)dir;
> > +
> > +       retval = mkdir(tmp_dir, mode);
> > +       if (tmp_dir != dir)
> > +               free(tmp_dir);
> > +
> > +       return retval;
> > +}
> 
> Why not rearrange this so that you assign to dir the value of tmp_dir and then
> just pass dir to mkdir.  Then you can avoid the recast of dir to (char*) in the
> else branch.  Later, just call free(tmp_dir).  Also, we have xstrndup.  So I think
> the body of your function can become something like:
> 
>    if (len && dir[len-1] == '/')
>        dir = tmp_dir = xstrndup(dir, len-1);

xstndup() can't fail?
 
>    retval = mkdir(dir, mode);
>    free(tmp_dir);
> 
> -Brandon

Bye, Jojo

Re: Porting git to HP NonStop

[Brandon Casey]

On Wed, Aug 22, 2012 at 10:18 AM, Joachim Schmitz
<jojo@schmitz-digital.de> wrote:
>> From: Brandon Casey [mailto:drafnel@gmail.com]
>> Sent: Wednesday, August 22, 2012 7:01 PM
>> To: Joachim Schmitz
>> Cc: Junio C Hamano; Shawn Pearce; git@vger.kernel.org;
>> rsbecker@nexbridge.com
>> Subject: Re: Porting git to HP NonStop
>>
>> On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz <jojo@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.c    2012-08-21 05:02:11 -0500
>> > @@ -0,0 +1,24 @@
>> > +#include "../git-compat-util.h"
>> > +#undef mkdir
>> > +
>> > +/* for platforms that can't deal with a trailing '/' */ int
>> > +compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode) {
>> > +       int retval;
>> > +       char *tmp_dir = NULL;
>> > +       size_t len = strlen(dir);
>> > +
>> > +       if (len && dir[len-1] == '/') {
>> > +               if ((tmp_dir = strdup(dir)) == NULL)
>> > +                       return -1;
>> > +               tmp_dir[len-1] = '\0';
>> > +       }
>> > +       else
>> > +               tmp_dir = (char *)dir;
>> > +
>> > +       retval = mkdir(tmp_dir, mode);
>> > +       if (tmp_dir != dir)
>> > +               free(tmp_dir);
>> > +
>> > +       return retval;
>> > +}
>>
>> Why not rearrange this so that you assign to dir the value of tmp_dir and then
>> just pass dir to mkdir.  Then you can avoid the recast of dir to (char*) in the
>> else branch.  Later, just call free(tmp_dir).  Also, we have xstrndup.  So I think
>> the body of your function can become something like:
>>
>>    if (len && dir[len-1] == '/')
>>        dir = tmp_dir = xstrndup(dir, len-1);
>
> xstndup() can't fail?

Correct.  It will either succeed or die.  It will also try to free up
some memory used by git if possible.

-Brandon

Re: Porting git to HP NonStop

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz
> <jojo@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.c    2012-08-21 05:02:11 -0500
>> @@ -0,0 +1,24 @@
>> +#include "../git-compat-util.h"
>> +#undef mkdir
>> +
>> +/* for platforms that can't deal with a trailing '/' */
>> +int compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode)
>> +{
>> +       int retval;
>> +       char *tmp_dir = NULL;
>> +       size_t len = strlen(dir);
>> + ...
> 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.

RE: Porting git to HP NonStop

[Joachim Schmitz]

> From: Brandon Casey [mailto:drafnel@gmail.com]
> Sent: Wednesday, August 22, 2012 7:23 PM
> To: Joachim Schmitz
> Cc: Junio C Hamano; Shawn Pearce; git@vger.kernel.org;
> rsbecker@nexbridge.com
> Subject: Re: Porting git to HP NonStop
> 
> On Wed, Aug 22, 2012 at 10:18 AM, Joachim Schmitz <jojo@schmitz-digital.de>
> wrote:
> >> From: Brandon Casey [mailto:drafnel@gmail.com]
> >> Sent: Wednesday, August 22, 2012 7:01 PM
> >> To: Joachim Schmitz
> >> Cc: Junio C Hamano; Shawn Pearce; git@vger.kernel.org;
> >> rsbecker@nexbridge.com
> >> Subject: Re: Porting git to HP NonStop
> >>
> >> On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz
> >> <jojo@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.c    2012-08-21 05:02:11 -0500
> >> > @@ -0,0 +1,24 @@
> >> > +#include "../git-compat-util.h"
> >> > +#undef mkdir
> >> > +
> >> > +/* for platforms that can't deal with a trailing '/' */ int
> >> > +compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode) {
> >> > +       int retval;
> >> > +       char *tmp_dir = NULL;
> >> > +       size_t len = strlen(dir);
> >> > +
> >> > +       if (len && dir[len-1] == '/') {
> >> > +               if ((tmp_dir = strdup(dir)) == NULL)
> >> > +                       return -1;
> >> > +               tmp_dir[len-1] = '\0';
> >> > +       }
> >> > +       else
> >> > +               tmp_dir = (char *)dir;
> >> > +
> >> > +       retval = mkdir(tmp_dir, mode);
> >> > +       if (tmp_dir != dir)
> >> > +               free(tmp_dir);
> >> > +
> >> > +       return retval;
> >> > +}
> >>
> >> Why not rearrange this so that you assign to dir the value of tmp_dir
> >> and then just pass dir to mkdir.  Then you can avoid the recast of
> >> dir to (char*) in the else branch.  Later, just call free(tmp_dir).
> >> Also, we have xstrndup.  So I think the body of your function can become
> something like:
> >>
> >>    if (len && dir[len-1] == '/')
> >>        dir = tmp_dir = xstrndup(dir, len-1);
> >
> > xstndup() can't fail?
> 
> 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

Re: Porting git to HP NonStop

[Johannes Sixt]

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

RE: Porting git to HP NonStop

[Joachim Schmitz]

> -----Original Message-----
> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Wednesday, August 22, 2012 7:30 PM
> To: Brandon Casey
> Cc: Joachim Schmitz; Shawn Pearce; git@vger.kernel.org;
> rsbecker@nexbridge.com
> Subject: Re: Porting git to HP NonStop
> 
> Brandon Casey <drafnel@gmail.com> writes:
> 
> > On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz
> > <jojo@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.c    2012-08-21 05:02:11 -0500
> >> @@ -0,0 +1,24 @@
> >> +#include "../git-compat-util.h"
> >> +#undef mkdir
> >> +
> >> +/* for platforms that can't deal with a trailing '/' */ int
> >> +compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode) {
> >> +       int retval;
> >> +       char *tmp_dir = NULL;
> >> +       size_t len = strlen(dir);
> >> + ...
> > 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.

How's that one used?

Bye, Jojo

RE: Porting git to HP NonStop

[Joachim Schmitz]

> -----Original Message-----
> From: Johannes Sixt [mailto:j6t@kdbg.org]
> Sent: Wednesday, August 22, 2012 7:41 PM
> To: Brandon Casey
> Cc: Joachim Schmitz; Junio C Hamano; Shawn Pearce; git@vger.kernel.org;
> rsbecker@nexbridge.com
> Subject: Re: Porting git to HP NonStop
> 
> Am 22.08.2012 19:00, schrieb Brandon Casey:
> >  So I think the body of [compat_mkdir] can become something like:
> >
> >    if (len && dir[len-1] == '/')
> >        dir = tmp_dir = xstrndup(dir, len-1);
> 
> Don't use x* wrappers in the compat layer, at least not those that allocate
> memory: They behave unpredictably due to try_to_free_routine and may lead
> to recursive invocations.

I was just following orders ;-)
What about the other proposal, xmemdupz? Same story I guess?

Bye, Jojo

Re: Porting git to HP NonStop

[Johannes Sixt]

Am 22.08.2012 20:02, schrieb Joachim Schmitz:
>> From: Johannes Sixt [mailto:j6t@kdbg.org]
>> Don't use x* wrappers in the compat layer, at least not those that allocate
>> memory: They behave unpredictably due to try_to_free_routine and may lead
>> to recursive invocations.
> 
> I was just following orders ;-)
> What about the other proposal, xmemdupz? Same story I guess?

xmemdupz calls xmalloc, so, yes, same story.

-- Hannes

Re: Porting git to HP NonStop

[Brandon Casey]

On Wed, Aug 22, 2012 at 10:41 AM, Johannes Sixt <j6t@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

RE: Porting git to HP NonStop

[Joachim Schmitz]

> -----Original Message-----
> From: Johannes Sixt [mailto:j6t@kdbg.org]
> Sent: Wednesday, August 22, 2012 8:09 PM
> To: Joachim Schmitz
> Cc: 'Brandon Casey'; 'Junio C Hamano'; 'Shawn Pearce'; git@vger.kernel.org;
> rsbecker@nexbridge.com
> Subject: Re: Porting git to HP NonStop
> 
> Am 22.08.2012 20:02, schrieb Joachim Schmitz:
> >> From: Johannes Sixt [mailto:j6t@kdbg.org] Don't use x* wrappers in
> >> the compat layer, at least not those that allocate
> >> memory: They behave unpredictably due to try_to_free_routine and may
> >> lead to recursive invocations.
> >
> > I was just following orders ;-)
> > What about the other proposal, xmemdupz? Same story I guess?
> 
> xmemdupz calls xmalloc, so, yes, same story.

So back to my original patch, using strdup, check the return value, etc.

Bye, Jojo

Re: Porting git to HP NonStop

[Brandon Casey]

On Wed, Aug 22, 2012 at 11:09 AM, Brandon Casey <drafnel@gmail.com> wrote:
> On Wed, Aug 22, 2012 at 10:41 AM, Johannes Sixt <j6t@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

Re: Porting git to HP NonStop

[Junio C Hamano]

"Joachim Schmitz" <jojo@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 x<allocate>() 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.

Re: Porting git to HP NonStop

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> On Wed, Aug 22, 2012 at 10:41 AM, Johannes Sixt <j6t@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...

Re: Porting git to HP NonStop

[Junio C Hamano]

Brandon Casey <drafnel@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.

Re: Porting git to HP NonStop

[Brandon Casey]

On Wed, Aug 22, 2012 at 11:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Casey <drafnel@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

RE: Porting git to HP NonStop

[Joachim Schmitz]

> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Wednesday, August 22, 2012 8:25 PM
> To: Joachim Schmitz
> Cc: 'Brandon Casey'; 'Shawn Pearce'; git@vger.kernel.org;
> rsbecker@nexbridge.com
> Subject: Re: Porting git to HP NonStop
> 
> "Joachim Schmitz" <jojo@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 x<allocate>() 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

RE: Porting git to HP NonStop

[Joachim Schmitz]

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 > 1000000
                || 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

Re: Porting git to HP NonStop

[Junio C Hamano]

"Joachim Schmitz" <jojo@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 > 1000000
>                 || 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?

RE: Porting git to HP NonStop

[Joachim Schmitz]

> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Wednesday, August 22, 2012 10:50 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org; rsbecker@nexbridge.com
> Subject: Re: Porting git to HP NonStop
> 
> "Joachim Schmitz" <jojo@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

Re: Porting git to HP NonStop

[Junio C Hamano]

"Joachim Schmitz" <jojo@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.

RE: Porting git to HP NonStop

[Joachim Schmitz]

> -----Original Message-----
> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Wednesday, August 22, 2012 11:12 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org; rsbecker@nexbridge.com
> Subject: Re: Porting git to HP NonStop
> 
> "Joachim Schmitz" <jojo@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

Re: Porting git to HP NonStop

[Junio C Hamano]

"Joachim Schmitz" <jojo@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).