[PATCH v2 0/6] Force pipes to flush immediately on NonStop platform

[PATCH v2 0/6] Force pipes to flush immediately on NonStop platform

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

* wrapper.c: called setbuf(stream,0) to force pipe flushes not enabled by
  default on the NonStop platform.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 wrapper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/wrapper.c b/wrapper.c
index d20356a77..671cbb4b4 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -403,6 +403,9 @@ FILE *xfdopen(int fd, const char *mode)
 	FILE *stream = fdopen(fd, mode);
 	if (stream == NULL)
 		die_errno("Out of memory? fdopen failed");
+#ifdef __TANDEM
+	setbuf(stream,0);
+#endif
 	return stream;
 }
 
-- 
2.16.0.31.gf1a482c

[PATCH v2 0/6] Cover Letter for NonStop Port Patches

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

This is the second attempt at submission of the NonStop port to
the git team. This package is split by file but should be applied
atomically.

I am not happy with the change in compat/regex/regcomp.c and figure
this might change with feedback.

Sincerely,
Randall

Randall S. Becker (7):
  Force pipes to flush immediately on NonStop platform
  Bypass GCC attributes on NonStop platform where used.
  Add tar extract install options override in installation processing.
  Define config options required for the HPE NonStop NSX and NSE
    platforms
  Force test suite traps to be cleared for NonStop ksh
  Bring NonStop platform definitions up to date in git-compat-util.h
  Add intptr_t and uintptr_t to regcomp.c for NonStop platform.

 Makefile               |  6 +++++-
 compat/regex/regcomp.c |  8 ++++++++
 config.mak.uname       | 29 +++++++++++++++++++++--------
 git-compat-util.h      | 15 +++++++++++++++
 remote.c               |  4 ++++
 t/lib-git-daemon.sh    |  3 +++
 wrapper.c              |  3 +++
 7 files changed, 59 insertions(+), 9 deletions(-)

-- 
2.16.0.31.gf1a482c

[PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

* remote.c: force ignoring of GCC __attribute construct not supported
  by c99 by defining it as an empty CPP macro.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 remote.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/remote.c b/remote.c
index 4e93753e1..c18f9de7f 100644
--- a/remote.c
+++ b/remote.c
@@ -11,6 +11,10 @@
 #include "mergesort.h"
 #include "argv-array.h"
 
+#if defined (__TANDEM)
+#define __attribute(a)
+#endif
+
 enum map_direction { FROM_SRC, FROM_DST };
 
 static struct refspec s_tag_refspec = {
-- 
2.16.0.31.gf1a482c

[PATCH v2 2/6] Add tar extract install options override in installation processing.

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

* Makefile: Add TAR_EXTRACT_OPTIONS to allow platform options to be
  specified if needed. The default is xof.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 Makefile | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 1a9b23b67..040e9eacd 100644
--- a/Makefile
+++ b/Makefile
@@ -429,6 +429,9 @@ all::
 # running the test scripts (e.g., bash has better support for "set -x"
 # tracing).
 #
+# Define TAR_EXTRACT_OPTIONS if you want to change the default behaviour
+# from xvf to something else during installation.
+#
 # When cross-compiling, define HOST_CPU as the canonical name of the CPU on
 # which the built Git will run (for instance "x86_64").
 
@@ -452,6 +455,7 @@ LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
 STRIP ?= strip
+TAR_EXTRACT_OPTIONS = xof
 
 # Create as necessary, replace existing, make ranlib unneeded.
 ARFLAGS = rcs
@@ -2569,7 +2573,7 @@ install: all
 ifndef NO_GETTEXT
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'
 	(cd po/build/locale && $(TAR) cf - .) | \
-	(cd '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) xof -)
+	(cd '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) $(TAR_EXTRACT_OPTIONS) -)
 endif
 ifndef NO_PERL
 	$(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
-- 
2.16.0.31.gf1a482c

[PATCH v2 3/6] Define config options required for the HPE NonStop NSX and NSE platforms

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

* config.mak.uname: upgrade old options to currently supported
  NonStop operating system versions (J06.21 and L17.xx).

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 config.mak.uname | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/config.mak.uname b/config.mak.uname
index 685a80d13..d9f8d57e3 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -428,27 +428,37 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
 	# INLINE='' would just replace one set of warnings with another and
 	# still not compile in c89 mode, due to non-const array initializations.
 	CC = cc -c99
+	# Build down-rev compatible objects that don't use our new getopt_long.
+	ifeq ($(uname_R).$(uname_V),J06.21)
+		CC += -WRVU=J06.20
+	endif
+	ifeq ($(uname_R).$(uname_V),L17.02)
+		CC += -WRVU=L16.05
+	endif
 	# Disable all optimization, seems to result in bad code, with -O or -O2
 	# or even -O1 (default), /usr/local/libexec/git-core/git-pack-objects
 	# abends on "git push". Needs more investigation.
-	CFLAGS = -g -O0
+	CFLAGS = -g -O0 -Winline
 	# We'd want it to be here.
 	prefix = /usr/local
 	# Our's are in ${prefix}/bin (perl might also be in /usr/bin/perl).
-	PERL_PATH = ${prefix}/bin/perl
-	PYTHON_PATH = ${prefix}/bin/python
-
+	PERL_PATH = /usr/bin/perl
+	PYTHON_PATH = /usr/bin/python
+	RM = /bin/rm -f
 	# As detected by './configure'.
 	# Missdetected, hence commented out, see below.
 	#NO_CURL = YesPlease
 	# Added manually, see above.
 	NEEDS_SSL_WITH_CURL = YesPlease
+	NEEDS_CRYPTO_WITH_SSL = YesPlease
+	HAVE_DEV_TTY = YesPlease
 	HAVE_LIBCHARSET_H = YesPlease
 	HAVE_STRINGS_H = YesPlease
 	NEEDS_LIBICONV = YesPlease
 	NEEDS_LIBINTL_BEFORE_LIBICONV = YesPlease
 	NO_SYS_SELECT_H = UnfortunatelyYes
 	NO_D_TYPE_IN_DIRENT = YesPlease
+	NO_GETTEXT = YesPlease
 	NO_HSTRERROR = YesPlease
 	NO_STRCASESTR = YesPlease
 	NO_MEMMEM = YesPlease
@@ -458,8 +468,13 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
 	NO_MKDTEMP = YesPlease
 	# Currently libiconv-1.9.1.
 	OLD_ICONV = UnfortunatelyYes
-	NO_REGEX = YesPlease
+	NO_REGEX=NeedsStartEnd
 	NO_PTHREADS = UnfortunatelyYes
+	ifdef NO_PTHREADS
+	else # WIP, use Posix User Threads
+		PTHREAD_CFLAGS = -D_PUT_MODEL_ -I/usr/include
+		PTHREAD_LIBS = -lput
+	endif
 
 	# Not detected (nor checked for) by './configure'.
 	# We don't have SA_RESTART on NonStop, unfortunalety.
@@ -477,9 +492,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
 	# RFE 10-120912-4693 submitted to HP NonStop development.
 	NO_SETITIMER = UnfortunatelyYes
 	SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
-	SHELL_PATH = /usr/local/bin/bash
-	# as of H06.25/J06.14, we might better use this
-	#SHELL_PATH = /usr/coreutils/bin/bash
+	SHELL_PATH = /usr/coreutils/bin/bash
 endif
 ifneq (,$(findstring MINGW,$(uname_S)))
 	pathsep = ;
-- 
2.16.0.31.gf1a482c

[PATCH v2 4/6] Force test suite traps to be cleared for NonStop ksh

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

* t/lib-git-daemon.sh: fix incompatibilities with ksh traps not being
  cleared automatically on platform. This caused tests to seem to fail
  while actually succeeding.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 t/lib-git-daemon.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 987d40680..955beecd9 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -68,6 +68,7 @@ start_git_daemon() {
 		test_skip_or_die $GIT_TEST_GIT_DAEMON \
 			"git daemon failed to start"
 	fi
+	trap '' EXIT
 }
 
 stop_git_daemon() {
@@ -89,4 +90,6 @@ stop_git_daemon() {
 	fi
 	GIT_DAEMON_PID=
 	rm -f git_daemon_output
+
+	trap '' EXIT
 }
-- 
2.16.0.31.gf1a482c

[PATCH v2 5/6] Bring NonStop platform definitions up to date in git-compat-util.h

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

* git-compat-util.h: add correct FLOSS definitions to allow correct
  emulation of non-platform behaviour. Add NSIG definition that is
  not explicitly supplied in signal.h on platform.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 git-compat-util.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 68b2ad531..1e13f050a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -378,6 +378,21 @@ static inline char *git_find_last_dir_sep(const char *path)
 #define find_last_dir_sep git_find_last_dir_sep
 #endif
 
+#ifdef __TANDEM
+#if !defined(_THREAD_SUPPORT_FUNCTIONS) && !defined(_PUT_MODEL_)
+/* #include <floss.h(floss_read,floss_write,floss_fsync,floss_fork)> */
+/* #include <floss.h(floss_fork)> */
+#endif
+#include <floss.h(floss_execl,floss_execlp,floss_execv,floss_execvp)>
+#include <floss.h(floss_getpwuid)>
+#if ! defined NSIG
+/* NonStop NSE and NSX do not provide NSIG. SIGGUARDIAN(99) is the highest
+   known, by detective work using kill -l as a list of all signals
+   instead of signal.h where it should be. */
+# define NSIG 100
+#endif
+#endif
+
 #if defined(__HP_cc) && (__HP_cc >= 61000)
 #define NORETURN __attribute__((noreturn))
 #define NORETURN_PTR
-- 
2.16.0.31.gf1a482c

[PATCH v2 6/6] Add intptr_t and uintptr_t to regcomp.c for NonStop platform.

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

* compat/regex/regcomp.c: fix missing intptr_t on NonStop. This is
  done because git-compat-util.h cannot be cleanly included into
  this file without additional compile errors.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 compat/regex/regcomp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
index 51cd60baa..8bb4c966d 100644
--- a/compat/regex/regcomp.c
+++ b/compat/regex/regcomp.c
@@ -17,6 +17,14 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#if defined __TANDEM
+/* This is currently duplicated from git-compat-utils.h */
+#ifdef NO_INTPTR_T
+typedef long intptr_t;
+typedef unsigned long uintptr_t;
+#endif
+#endif
+
 static reg_errcode_t re_compile_internal (regex_t *preg, const char * pattern,
 					  size_t length, reg_syntax_t syntax);
 static void re_compile_fastmap_iter (regex_t *bufp,
-- 
2.16.0.31.gf1a482c

Re: [PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[Stefan Beller]

On Fri, Jan 19, 2018 at 9:34 AM,  <randall.s.becker@rogers.com> wrote:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> * remote.c: force ignoring of GCC __attribute construct not supported
>   by c99 by defining it as an empty CPP macro.
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>

I do not know about the __TANDEM symbol, but some research[1]
indicates it is a macro specifically for case of cross platform support:

    You can use the __TANDEM symbol to increase the portability
    of your programs. Enclose system-dependent source text in an
    if section that uses #ifdef or #ifndef to test for the existence of the
    __TANDEM symbol.

It seems to not be used outside of the NonStop port[2], so the code seems
ok. (I would have used #ifdef, as I thought this is more prevalent in our
code base and for consistency we'd rather want to use one thing only, bug
"git grep '#if defined'" proves me wrong)

However the commit message is hard to understand (say for somebody who
will find this commit in 2 years using git-blame).

Maybe:

    In NonStop HPE (version X, all versions?) there is no support for the
    __attribute macro. By redefining the __attribute to an empty macro, the
    code compiles on NonStop HPE. Use the system specific __TANDEM
    macro to guard it for just this platform.

as that will help those people in the future not having to do the research?


[1] http://h20628.www2.hp.com/km-ext/kmcsdirect/emr_na-c02121175-1.pdf
[2] https://sourceforge.net/p/predef/wiki/OperatingSystems/?version=44

Thanks,
Stefan


> ---
>  remote.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/remote.c b/remote.c
> index 4e93753e1..c18f9de7f 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -11,6 +11,10 @@
>  #include "mergesort.h"
>  #include "argv-array.h"
>
> +#if defined (__TANDEM)
> +#define __attribute(a)
> +#endif
> +
>  enum map_direction { FROM_SRC, FROM_DST };
>
>  static struct refspec s_tag_refspec = {
> --
> 2.16.0.31.gf1a482c
>

Re: [PATCH v2 0/6] Force pipes to flush immediately on NonStop platform

[Stefan Beller]

On Fri, Jan 19, 2018 at 9:33 AM,  <randall.s.becker@rogers.com> wrote:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> * wrapper.c: called setbuf(stream,0) to force pipe flushes not enabled by
>   default on the NonStop platform.
>

Due to my review of a previous patch, I now know about the __TANDEM
directive and why we use this to guard this line. :)

The setbuf(0) is easier than sprinkling (guarded) flushes all over the code,
so that seems like a clean solution, which we currently don't use.
(it occurred twice in history, see eac14f8909 (Win32: Thread-safe
windows console output, 2012-01-14) for its introduction and fcd428f4a9
(Win32: fix broken pipe detection, 2012-03-01) for its deletion).

> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  wrapper.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/wrapper.c b/wrapper.c
> index d20356a77..671cbb4b4 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -403,6 +403,9 @@ FILE *xfdopen(int fd, const char *mode)
>         FILE *stream = fdopen(fd, mode);
>         if (stream == NULL)
>                 die_errno("Out of memory? fdopen failed");
> +#ifdef __TANDEM
> +       setbuf(stream,0);

My man page tells me the second arg is a pointer,
so we'd prefer NULL instead?

Thanks,
Stefan

> +#endif
>         return stream;
>  }
>
> --
> 2.16.0.31.gf1a482c
>

Re: [PATCH v2 4/6] Force test suite traps to be cleared for NonStop ksh

[Stefan Beller]

On Fri, Jan 19, 2018 at 9:34 AM,  <randall.s.becker@rogers.com> wrote:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> * t/lib-git-daemon.sh: fix incompatibilities with ksh traps not being
>   cleared automatically on platform.

Which platform? Do we need to guard it for other platforms?
(I guess we don't we have traps in some other places and it is
POSIX)

>  This caused tests to seem to fail
>   while actually succeeding.
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  t/lib-git-daemon.sh | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
> index 987d40680..955beecd9 100644
> --- a/t/lib-git-daemon.sh
> +++ b/t/lib-git-daemon.sh
> @@ -68,6 +68,7 @@ start_git_daemon() {
>                 test_skip_or_die $GIT_TEST_GIT_DAEMON \
>                         "git daemon failed to start"
>         fi
> +       trap '' EXIT
>  }
>
>  stop_git_daemon() {
> @@ -89,4 +90,6 @@ stop_git_daemon() {
>         fi
>         GIT_DAEMON_PID=
>         rm -f git_daemon_output
> +
> +       trap '' EXIT
>  }
> --
> 2.16.0.31.gf1a482c
>

Re: [PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[Ramsay Jones]

On 19/01/18 17:34, randall.s.becker@rogers.com wrote:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
> 
> * remote.c: force ignoring of GCC __attribute construct not supported
>   by c99 by defining it as an empty CPP macro.
> 
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  remote.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/remote.c b/remote.c
> index 4e93753e1..c18f9de7f 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -11,6 +11,10 @@
>  #include "mergesort.h"
>  #include "argv-array.h"
>  
> +#if defined (__TANDEM)
> +#define __attribute(a)
> +#endif
> +

Hmm, the only use of __attribute() I can find is in compat/regex/.
In particular, there is no use of __attribute() in regex.c.
[__attribute__() is used in regex.c]

Is this an old patch which is no longer required?

puzzled.

ATB,
Ramsay Jones

>  enum map_direction { FROM_SRC, FROM_DST };
>  
>  static struct refspec s_tag_refspec = {
> 

Re: [PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[Jeff King]

On Fri, Jan 19, 2018 at 08:28:48PM +0000, Ramsay Jones wrote:

> > diff --git a/remote.c b/remote.c
> > index 4e93753e1..c18f9de7f 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -11,6 +11,10 @@
> >  #include "mergesort.h"
> >  #include "argv-array.h"
> >  
> > +#if defined (__TANDEM)
> > +#define __attribute(a)
> > +#endif
> > +
> 
> Hmm, the only use of __attribute() I can find is in compat/regex/.
> In particular, there is no use of __attribute() in regex.c.
> [__attribute__() is used in regex.c]
> 
> Is this an old patch which is no longer required?
> 
> puzzled.

I'm puzzled, too. The actual gcc thing is __attribute__(), and we
already turn that into a noop via macro expansion if __GNUC__ is not
defined (in git-compat-util.h, but see below).

__attribute(), without the trailing underscores, is used internally by
the regex compat code (but it also converts that into a noop on non-GNUC
platforms)>

However the logic in git-compat-util is weird:

  #if defined(__HP_cc) && (__HP_cc >= 61000)
  #define NORETURN __attribute__((noreturn))
  #define NORETURN_PTR
  #elif defined(__GNUC__) && !defined(NO_NORETURN)
  #define NORETURN __attribute__((__noreturn__))
  #define NORETURN_PTR __attribute__((__noreturn__))
  #elif defined(_MSC_VER)
  #define NORETURN __declspec(noreturn)
  #define NORETURN_PTR
  #else
  #define NORETURN
  #define NORETURN_PTR
  #ifndef __GNUC__
  #ifndef __attribute__
  #define __attribute__(x)
  #endif
  #endif
  #endif

Most of the conditional is dealing with NORETURN, but then we stick the
__attribute()__ handling in the "else" block. Is it possible that this
platform triggers __HP_cc, but doesn't actually understand
__attribute__?

-Peff

Re: [PATCH v2 4/6] Force test suite traps to be cleared for NonStop ksh

[Jeff King]

On Fri, Jan 19, 2018 at 12:34:04PM -0500, randall.s.becker@rogers.com wrote:

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
> 
> * t/lib-git-daemon.sh: fix incompatibilities with ksh traps not being
>   cleared automatically on platform. This caused tests to seem to fail
>   while actually succeeding.
> 
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  t/lib-git-daemon.sh | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
> index 987d40680..955beecd9 100644
> --- a/t/lib-git-daemon.sh
> +++ b/t/lib-git-daemon.sh
> @@ -68,6 +68,7 @@ start_git_daemon() {
>  		test_skip_or_die $GIT_TEST_GIT_DAEMON \
>  			"git daemon failed to start"
>  	fi
> +	trap '' EXIT
>  }

I don't think this can be right. The way these traps are supposed to
work is:

  - as soon as test-lib.sh is loaded, we "trap die EXIT" to catch an
    accidental death/exit from one of the scripts

  - when test_done is called, we clear the trap (i.e., it is OK to exit
    now because the script has given us a positive indication that all
    tests have been run)

  - while the child git-daemon is running, we'd want to clean up after
    ourselves. So during start_git_daemon() we add our cleanup (followed
    by the original "die", because shell traps don't push onto a stack).
    And then at stop_git_daemon(), we restore the original die trap.

But this patch means that while git-daemon is running, we have no trap
at all! That means that a failed test which causes us to exit would
leave a child daemon running.

Furthermore, both of these functions now drop the 'die' trap entirely,
meaning the script would fail to notice premature exit from one of the
test snippets.

So while this may be papering over a problem on NonStop, I think it's
breaking the intent of the traps.

I'm not sure what the root of the problem is, but it sounds like ksh may
be triggering EXIT traps when we don't expect (during function returns?
Subshell exits? Other?)

-Peff

RE: [PATCH v2 4/6] Force test suite traps to be cleared for NonStop ksh

[Randall S. Becker]

On January 19, 2018 4:27 PM, Jeff King wrote:
> On Fri, Jan 19, 2018 at 12:34:04PM -0500, randall.s.becker@rogers.com wrote:
> 
> > From: "Randall S. Becker" <rsbecker@nexbridge.com>
> >
> > * t/lib-git-daemon.sh: fix incompatibilities with ksh traps not being
> >   cleared automatically on platform. This caused tests to seem to fail
> >   while actually succeeding.
> >
> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > ---
> >  t/lib-git-daemon.sh | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh index
> > 987d40680..955beecd9 100644
> > --- a/t/lib-git-daemon.sh
> > +++ b/t/lib-git-daemon.sh
> > @@ -68,6 +68,7 @@ start_git_daemon() {
> >  		test_skip_or_die $GIT_TEST_GIT_DAEMON \
> >  			"git daemon failed to start"
> >  	fi
> > +	trap '' EXIT
> >  }
> 
> I don't think this can be right. The way these traps are supposed to work is:
> 
>   - as soon as test-lib.sh is loaded, we "trap die EXIT" to catch an
>     accidental death/exit from one of the scripts
> 
>   - when test_done is called, we clear the trap (i.e., it is OK to exit
>     now because the script has given us a positive indication that all
>     tests have been run)
> 
>   - while the child git-daemon is running, we'd want to clean up after
>     ourselves. So during start_git_daemon() we add our cleanup (followed
>     by the original "die", because shell traps don't push onto a stack).
>     And then at stop_git_daemon(), we restore the original die trap.
> 
> But this patch means that while git-daemon is running, we have no trap at all!
> That means that a failed test which causes us to exit would leave a child
> daemon running.
> 
> Furthermore, both of these functions now drop the 'die' trap entirely,
> meaning the script would fail to notice premature exit from one of the test
> snippets.
> 
> So while this may be papering over a problem on NonStop, I think it's
> breaking the intent of the traps.
> 
> I'm not sure what the root of the problem is, but it sounds like ksh may be
> triggering EXIT traps when we don't expect (during function returns?
> Subshell exits? Other?)

The "unexpected" EXIT traps are exactly the issue we found when working with the platform support team. There was some discussion about what the right expectation was, and I was not able to have a change made to ksh on the platform. The problem may have a similar (identical?) root cause to the Perl exit issues we are also experiencing that is in their hands. I'm currently retesting without this change (results in 36 hours ☹ ). Is there a preferred way you would like me to bypass this except on NonStop? I can add a check based on uname.

Cheers,
Randall

Re: [PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[Ramsay Jones]

On 19/01/18 21:20, Jeff King wrote:
> On Fri, Jan 19, 2018 at 08:28:48PM +0000, Ramsay Jones wrote:
> 
>>> diff --git a/remote.c b/remote.c
>>> index 4e93753e1..c18f9de7f 100644
>>> --- a/remote.c
>>> +++ b/remote.c
>>> @@ -11,6 +11,10 @@
>>>  #include "mergesort.h"
>>>  #include "argv-array.h"
>>>  
>>> +#if defined (__TANDEM)
>>> +#define __attribute(a)
>>> +#endif
>>> +
>>
>> Hmm, the only use of __attribute() I can find is in compat/regex/.
>> In particular, there is no use of __attribute() in regex.c.
>> [__attribute__() is used in regex.c]
>>
>> Is this an old patch which is no longer required?
>>
>> puzzled.

heh, I only just noticed that I (twice) wrote regex.c when I meant
remote.c instead. Hopefully, that didn't cause too much confusion!

> I'm puzzled, too. The actual gcc thing is __attribute__(), and we
> already turn that into a noop via macro expansion if __GNUC__ is not
> defined (in git-compat-util.h, but see below).
> 
> __attribute(), without the trailing underscores, is used internally by
> the regex compat code (but it also converts that into a noop on non-GNUC
> platforms)>

Indeed.

> However the logic in git-compat-util is weird:
> 
>   #if defined(__HP_cc) && (__HP_cc >= 61000)
>   #define NORETURN __attribute__((noreturn))
>   #define NORETURN_PTR
>   #elif defined(__GNUC__) && !defined(NO_NORETURN)
>   #define NORETURN __attribute__((__noreturn__))
>   #define NORETURN_PTR __attribute__((__noreturn__))
>   #elif defined(_MSC_VER)
>   #define NORETURN __declspec(noreturn)
>   #define NORETURN_PTR
>   #else
>   #define NORETURN
>   #define NORETURN_PTR
>   #ifndef __GNUC__
>   #ifndef __attribute__
>   #define __attribute__(x)
>   #endif
>   #endif
>   #endif
> 
> Most of the conditional is dealing with NORETURN, but then we stick the
> __attribute()__ handling in the "else" block. Is it possible that this
> platform triggers __HP_cc, but doesn't actually understand
> __attribute__?

That seems unlikely. However, that conditional looks a mess ... ;-)

ATB,
Ramsay Jones

RE: [PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[Randall S. Becker]

On January 19, 2018 5:43 PM, Ramsay Jones wrote:
> On 19/01/18 21:20, Jeff King wrote:
> > On Fri, Jan 19, 2018 at 08:28:48PM +0000, Ramsay Jones wrote:
> >
> >>> diff --git a/remote.c b/remote.c
> >>> index 4e93753e1..c18f9de7f 100644
> >>> --- a/remote.c
> >>> +++ b/remote.c
> >>> @@ -11,6 +11,10 @@
> >>>  #include "mergesort.h"
> >>>  #include "argv-array.h"
> >>>
> >>> +#if defined (__TANDEM)
> >>> +#define __attribute(a)
> >>> +#endif
> >>> +
> >>
> >> Hmm, the only use of __attribute() I can find is in compat/regex/.
> >> In particular, there is no use of __attribute() in regex.c.
> >> [__attribute__() is used in regex.c]
> >>
> >> Is this an old patch which is no longer required?
> >>
> >> puzzled.
> 
> heh, I only just noticed that I (twice) wrote regex.c when I meant remote.c
> instead. Hopefully, that didn't cause too much confusion!
> 
> > I'm puzzled, too. The actual gcc thing is __attribute__(), and we
> > already turn that into a noop via macro expansion if __GNUC__ is not
> > defined (in git-compat-util.h, but see below).
> >
> > __attribute(), without the trailing underscores, is used internally by
> > the regex compat code (but it also converts that into a noop on
> > non-GNUC platforms)>
> 
> Indeed.
> 
> > However the logic in git-compat-util is weird:
> >
> >   #if defined(__HP_cc) && (__HP_cc >= 61000)
> >   #define NORETURN __attribute__((noreturn))
> >   #define NORETURN_PTR
> >   #elif defined(__GNUC__) && !defined(NO_NORETURN)
> >   #define NORETURN __attribute__((__noreturn__))
> >   #define NORETURN_PTR __attribute__((__noreturn__))
> >   #elif defined(_MSC_VER)
> >   #define NORETURN __declspec(noreturn)
> >   #define NORETURN_PTR
> >   #else
> >   #define NORETURN
> >   #define NORETURN_PTR
> >   #ifndef __GNUC__
> >   #ifndef __attribute__
> >   #define __attribute__(x)
> >   #endif
> >   #endif
> >   #endif
> >
> > Most of the conditional is dealing with NORETURN, but then we stick
> > the __attribute()__ handling in the "else" block. Is it possible that
> > this platform triggers __HP_cc, but doesn't actually understand
> > __attribute__?
> 
> That seems unlikely. However, that conditional looks a mess ... ;-)

Very messy and confusing and it is working properly now, so... consider this patch gone ;-)

Cheers,
Randall

RE: [PATCH v2 4/6] Force test suite traps to be cleared for NonStop ksh

[Randall S. Becker]

On January 19, 2018 5:29 PM, I wrote:
> On January 19, 2018 4:27 PM, Jeff King wrote:
> > On Fri, Jan 19, 2018 at 12:34:04PM -0500, randall.s.becker@rogers.com
> wrote:
> >
> > > From: "Randall S. Becker" <rsbecker@nexbridge.com>
> > >
> > > * t/lib-git-daemon.sh: fix incompatibilities with ksh traps not being
> > >   cleared automatically on platform. This caused tests to seem to fail
> > >   while actually succeeding.
> > >
> > > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > > ---
> > >  t/lib-git-daemon.sh | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh index
> > > 987d40680..955beecd9 100644
> > > --- a/t/lib-git-daemon.sh
> > > +++ b/t/lib-git-daemon.sh
> > > @@ -68,6 +68,7 @@ start_git_daemon() {
> > >  		test_skip_or_die $GIT_TEST_GIT_DAEMON \
> > >  			"git daemon failed to start"
> > >  	fi
> > > +	trap '' EXIT
> > >  }
> >
> > I don't think this can be right. The way these traps are supposed to work is:
> >
> >   - as soon as test-lib.sh is loaded, we "trap die EXIT" to catch an
> >     accidental death/exit from one of the scripts
> >
> >   - when test_done is called, we clear the trap (i.e., it is OK to exit
> >     now because the script has given us a positive indication that all
> >     tests have been run)
> >
> >   - while the child git-daemon is running, we'd want to clean up after
> >     ourselves. So during start_git_daemon() we add our cleanup (followed
> >     by the original "die", because shell traps don't push onto a stack).
> >     And then at stop_git_daemon(), we restore the original die trap.
> >
> > But this patch means that while git-daemon is running, we have no trap at
> all!
> > That means that a failed test which causes us to exit would leave a
> > child daemon running.
> >
> > Furthermore, both of these functions now drop the 'die' trap entirely,
> > meaning the script would fail to notice premature exit from one of the
> > test snippets.
> >
> > So while this may be papering over a problem on NonStop, I think it's
> > breaking the intent of the traps.
> >
> > I'm not sure what the root of the problem is, but it sounds like ksh
> > may be triggering EXIT traps when we don't expect (during function
> returns?
> > Subshell exits? Other?)
> 
> The "unexpected" EXIT traps are exactly the issue we found when working
> with the platform support team. There was some discussion about what the
> right expectation was, and I was not able to have a change made to ksh on
> the platform. The problem may have a similar (identical?) root cause to the
> Perl exit issues we are also experiencing that is in their hands. I'm currently
> retesting without this change (results in 36 hours ☹ ). Is there a preferred
> way you would like me to bypass this except on NonStop? I can add a check
> based on uname.

The option that may work, if the tests that are currenting running until Sunday (sadly) fail miserably, is to use:

if [ `uname` = "NONSTOP_KERNEL" ]; then trap '' EXIT; fi

or perhaps to add a descriptive function along those lines. We have had two major operating system upgrades since the original case relating to ksh traps, so perhaps things might improve. Our baseline is that there are currently 6 breaks (t1308#23, t1405#52, t9001#31/106/107/134), most of which have been traced back to perl completion codes.

Cheers,
Randall
P.S. I am happy to explain why the tests perform the at the rate they do on the development machines I have, if anyone is interested, although dissertations might be involved 😉

-- Brief whoami:
 NonStop developer since approximately 211288444200000000
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [PATCH v2 0/6] Force pipes to flush immediately on NonStop platform

[Torsten Bögershausen]

On Fri, Jan 19, 2018 at 12:33:59PM -0500, randall.s.becker@rogers.com wrote:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
> 
> * wrapper.c: called setbuf(stream,0) to force pipe flushes not enabled by
>   default on the NonStop platform.
> 
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  wrapper.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/wrapper.c b/wrapper.c
> index d20356a77..671cbb4b4 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -403,6 +403,9 @@ FILE *xfdopen(int fd, const char *mode)
>  	FILE *stream = fdopen(fd, mode);
>  	if (stream == NULL)
>  		die_errno("Out of memory? fdopen failed");
> +#ifdef __TANDEM
> +	setbuf(stream,0);
> +#endif

Reading the commit message, I would have expected someting similar to

#ifdef FORCE_PIPE_FLUSHES
	setbuf(stream,0);
#endif

(Because other systems may need the tweak as well, some day)
Of course you need to change that in the Makefile and config.mak.uname

>  	return stream;
>  }
>  
> -- 
> 2.16.0.31.gf1a482c
> 

Re: [PATCH v2 2/6] Add tar extract install options override in installation processing.

[René Scharfe]

Am 19.01.2018 um 18:34 schrieb randall.s.becker@rogers.com:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
> 
> * Makefile: Add TAR_EXTRACT_OPTIONS to allow platform options to be
>    specified if needed. The default is xof.
> 
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>   Makefile | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 1a9b23b67..040e9eacd 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -429,6 +429,9 @@ all::
>   # running the test scripts (e.g., bash has better support for "set -x"
>   # tracing).
>   #
> +# Define TAR_EXTRACT_OPTIONS if you want to change the default behaviour
> +# from xvf to something else during installation.

"xof" instead of "xvf"?

> +#
>   # When cross-compiling, define HOST_CPU as the canonical name of the CPU on
>   # which the built Git will run (for instance "x86_64").
>   
> @@ -452,6 +455,7 @@ LDFLAGS =
>   ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
>   ALL_LDFLAGS = $(LDFLAGS)
>   STRIP ?= strip
> +TAR_EXTRACT_OPTIONS = xof
>   
>   # Create as necessary, replace existing, make ranlib unneeded.
>   ARFLAGS = rcs
> @@ -2569,7 +2573,7 @@ install: all
>   ifndef NO_GETTEXT
>   	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'
>   	(cd po/build/locale && $(TAR) cf - .) | \
> -	(cd '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) xof -)
> +	(cd '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) $(TAR_EXTRACT_OPTIONS) -)

Hmm.  TAR_EXTRACT_OPTIONS always needs to have f (or -f, or --file) at
the end to go together with the following dash, meaning to extract from
stdin.  And x (or -x, or --extract) is probably needed in all cases as
well.  So wouldn't it make more sense to only put the o (or -o, or
--no-same-owner) into TAR_EXTRACT_OPTIONS and enforce x and f?

>   endif
>   ifndef NO_PERL
>   	$(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
> 

RE: [PATCH v2 0/6] Force pipes to flush immediately on NonStop platform

[Randall S. Becker]

On January 20, 2018 6:10 AM,  Torsten Bögershausen wrote:
> On Fri, Jan 19, 2018 at 12:33:59PM -0500, randall.s.becker@rogers.com
> wrote:
> > From: "Randall S. Becker" <rsbecker@nexbridge.com>
> >
> > * wrapper.c: called setbuf(stream,0) to force pipe flushes not enabled by
> >   default on the NonStop platform.
> >
> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > ---
> >  wrapper.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/wrapper.c b/wrapper.c
> > index d20356a77..671cbb4b4 100644
> > --- a/wrapper.c
> > +++ b/wrapper.c
> > @@ -403,6 +403,9 @@ FILE *xfdopen(int fd, const char *mode)
> >  	FILE *stream = fdopen(fd, mode);
> >  	if (stream == NULL)
> >  		die_errno("Out of memory? fdopen failed");
> > +#ifdef __TANDEM
> > +	setbuf(stream,0);
> > +#endif
> 
> Reading the commit message, I would have expected someting similar to
> 
> #ifdef FORCE_PIPE_FLUSHES
> 	setbuf(stream,0);
> #endif
> 
> (Because other systems may need the tweak as well, some day) Of course
> you need to change that in the Makefile and config.mak.uname
> 
> >  	return stream;
> >  }

I can definitely see the point. Would you be agreeable to expanding the scope of this as a separate patch after this one is applied? I would want to bring at least one more Not NonStop machine into the mix for testing, which is more than I can do this weekend 😊.

Cheers,
Randall

RE: [PATCH v2 2/6] Add tar extract install options override in installation processing.

[Randall S. Becker]

On January 20, 2018 7:31 AM, René Scharfe wrote:
> Am 19.01.2018 um 18:34 schrieb randall.s.becker@rogers.com:
> > From: "Randall S. Becker" <rsbecker@nexbridge.com>
> >
> > * Makefile: Add TAR_EXTRACT_OPTIONS to allow platform options to be
> >    specified if needed. The default is xof.
> >
> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > ---
> >   Makefile | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 1a9b23b67..040e9eacd 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -429,6 +429,9 @@ all::
> >   # running the test scripts (e.g., bash has better support for "set -x"
> >   # tracing).
> >   #
> > +# Define TAR_EXTRACT_OPTIONS if you want to change the default
> > +behaviour # from xvf to something else during installation.
> 
> "xof" instead of "xvf"?

When I look at the parent commit, it says xof, so I wanted to preserve existing behaviour by default. Our install process wants to see the actual set of files, so we wanted to use xvof but that hardly seemed of general interest. I was hoping an option to control it would be agreeable.

> > +#
> >   # When cross-compiling, define HOST_CPU as the canonical name of the
> CPU on
> >   # which the built Git will run (for instance "x86_64").
> >
> > @@ -452,6 +455,7 @@ LDFLAGS =
> >   ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
> >   ALL_LDFLAGS = $(LDFLAGS)
> >   STRIP ?= strip
> > +TAR_EXTRACT_OPTIONS = xof
> >
> >   # Create as necessary, replace existing, make ranlib unneeded.
> >   ARFLAGS = rcs
> > @@ -2569,7 +2573,7 @@ install: all
> >   ifndef NO_GETTEXT
> >   	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'
> >   	(cd po/build/locale && $(TAR) cf - .) | \
> > -	(cd '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) xof -)
> > +	(cd '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR)
> > +$(TAR_EXTRACT_OPTIONS) -)
> 
> Hmm.  TAR_EXTRACT_OPTIONS always needs to have f (or -f, or --file) at the
> end to go together with the following dash, meaning to extract from stdin.
> And x (or -x, or --extract) is probably needed in all cases as well.  So wouldn't
> it make more sense to only put the o (or -o, or
> --no-same-owner) into TAR_EXTRACT_OPTIONS and enforce x and f?

This is a good suggestion, and I'd love to do that, if I could guarantee a modern tar, which I can't. The platform comes with a really old-school tar from some old (seemingly BSD4.3) epoch that only takes one option set. There is a more modern tar that can be optionally installed if the sysadmin decides to that takes a slightly more modern set, which could support your request, and my team also has a gnu port that is very modern. I can't control what customers are choosing to have installed, unfortunately. Your point is well made and I am completely on board with it, but it introduces a configuration requirement.

As with the broadening setbuf (patch 2/6) change, I would like to consider this for the future, having a slightly different more complex idea. I could introduce something like this:

1. HAS_ANCIENT_TAR=UnfortunatelyYes in config.mak.uname that disables this capability all together
2. HAS_ANCIENT_TAR=AreYouKiddingMe (joke) then set up TAR_EXTRACT_ADDITIONAL_OPTIONS above and beyond the default, so --file, --no-same-owner would always be in effect for that operation.

The micro-project would also, logically, need to apply to other tar occurrences throughout the code and potentially need a test case written for it (not entirely sure what that would test, yet).

Is that a reasonable approach?

> 
> >   endif
> >   ifndef NO_PERL
> >   	$(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)'
> > install
> >

Re: [PATCH v2 2/6] Add tar extract install options override in installation processing.

[René Scharfe]

Am 20.01.2018 um 14:44 schrieb Randall S. Becker:
> On January 20, 2018 7:31 AM, René Scharfe wrote:
>> Am 19.01.2018 um 18:34 schrieb randall.s.becker@rogers.com:
>>> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>>> 
>>> * Makefile: Add TAR_EXTRACT_OPTIONS to allow platform options to
>>> be specified if needed. The default is xof.
>>> 
>>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com> --- 
>>> Makefile | 6 +++++- 1 file changed, 5 insertions(+), 1
>>> deletion(-)
>>> 
>>> diff --git a/Makefile b/Makefile index 1a9b23b67..040e9eacd
>>> 100644 --- a/Makefile +++ b/Makefile @@ -429,6 +429,9 @@ all:: #
>>> running the test scripts (e.g., bash has better support for "set
>>> -x" # tracing). # +# Define TAR_EXTRACT_OPTIONS if you want to
>>> change the default +behaviour # from xvf to something else during
>>> installation.
>> 
>> "xof" instead of "xvf"?
> 
> When I look at the parent commit, it says xof, so I wanted to
> preserve existing behaviour by default. Our install process wants to
> see the actual set of files, so we wanted to use xvof but that hardly
> seemed of general interest. I was hoping an option to control it
> would be agreeable.

Preserving the default is good. I meant that the default is "xof",
but the added line implies it was "xvf" instead.

Seeing your actual use case confirms that my suggestion below would
work for you.

> 
>>> +# # When cross-compiling, define HOST_CPU as the canonical name
>>> of the
>> CPU on
>>> # which the built Git will run (for instance "x86_64").
>>> 
>>> @@ -452,6 +455,7 @@ LDFLAGS = ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) 
>>> ALL_LDFLAGS = $(LDFLAGS) STRIP ?= strip +TAR_EXTRACT_OPTIONS =
>>> xof
>>> 
>>> # Create as necessary, replace existing, make ranlib unneeded. 
>>> ARFLAGS = rcs @@ -2569,7 +2573,7 @@ install: all ifndef
>>> NO_GETTEXT $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)' 
>>> (cd po/build/locale && $(TAR) cf - .) | \ -	(cd
>>> '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) xof -) +
>>> (cd '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) 
>>> +$(TAR_EXTRACT_OPTIONS) -)
>> 
>> Hmm.  TAR_EXTRACT_OPTIONS always needs to have f (or -f, or --file)
>> at the end to go together with the following dash, meaning to
>> extract from stdin. And x (or -x, or --extract) is probably needed
>> in all cases as well.  So wouldn't it make more sense to only put
>> the o (or -o, or --no-same-owner) into TAR_EXTRACT_OPTIONS and
>> enforce x and f?
> 
> This is a good suggestion, and I'd love to do that, if I could
> guarantee a modern tar, which I can't. The platform comes with a
> really old-school tar from some old (seemingly BSD4.3) epoch that
> only takes one option set. There is a more modern tar that can be
> optionally installed if the sysadmin decides to that takes a slightly
> more modern set, which could support your request, and my team also
> has a gnu port that is very modern. I can't control what customers
> are choosing to have installed, unfortunately. Your point is well
> made and I am completely on board with it, but it introduces a
> configuration requirement.

Long options would be nice to nice to have, but are not my main
point; I included them mainly to spare readers from firing up
"man tar" to look up the meaning of the short ones.

I just meant to say that something like this here would make more
sense because you always need x and f- anyway:

	TAR_EXTRACT_OPTIONS = o

	... ${TAR} x${TAR_EXTRACT_OPTIONS}f -

> As with the broadening setbuf (patch 2/6) change, I would like to
> consider this for the future, having a slightly different more
> complex idea. I could introduce something like this:
> 
> 1. HAS_ANCIENT_TAR=UnfortunatelyYes in config.mak.uname that disables
> this capability all together 2. HAS_ANCIENT_TAR=AreYouKiddingMe
> (joke) then set up TAR_EXTRACT_ADDITIONAL_OPTIONS above and beyond
> the default, so --file, --no-same-owner would always be in effect for
> that operation.
> 
> The micro-project would also, logically, need to apply to other tar
> occurrences throughout the code and potentially need a test case
> written for it (not entirely sure what that would test, yet).
> Is that a reasonable approach?

As long as old-school dash-less flags suffice for our purposes
(including yours) we can just keep using that style everywhere and
avoid adding more settings.  It would be a different matter if we
needed features that have no short flag, or are only offered by
certain tar implementations.

René

RE: [PATCH v2 2/6] Add tar extract install options override in installation processing.

[Randall S. Becker]

On January 20, 2018 9:25 AM, René Scharfe wrote:
> To: Randall S. Becker <rsbecker@nexbridge.com>; git@vger.kernel.org
> Subject: Re: [PATCH v2 2/6] Add tar extract install options override in
> installation processing.
> 
> Am 20.01.2018 um 14:44 schrieb Randall S. Becker:
> > On January 20, 2018 7:31 AM, René Scharfe wrote:
> >> Am 19.01.2018 um 18:34 schrieb randall.s.becker@rogers.com:
> >>> From: "Randall S. Becker" <rsbecker@nexbridge.com>
> >>>
> >>> * Makefile: Add TAR_EXTRACT_OPTIONS to allow platform options to be
> >>> specified if needed. The default is xof.
> >>>
> >>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com> ---
> >>> Makefile | 6 +++++- 1 file changed, 5 insertions(+), 1
> >>> deletion(-)
> >>>
> >>> diff --git a/Makefile b/Makefile index 1a9b23b67..040e9eacd
> >>> 100644 --- a/Makefile +++ b/Makefile @@ -429,6 +429,9 @@ all:: #
> >>> running the test scripts (e.g., bash has better support for "set -x"
> >>> # tracing). # +# Define TAR_EXTRACT_OPTIONS if you want to change
> >>> the default +behaviour # from xvf to something else during
> >>> installation.
> >>
> >> "xof" instead of "xvf"?
> >
> > When I look at the parent commit, it says xof, so I wanted to preserve
> > existing behaviour by default. Our install process wants to see the
> > actual set of files, so we wanted to use xvof but that hardly seemed
> > of general interest. I was hoping an option to control it would be
> > agreeable.
> 
> Preserving the default is good. I meant that the default is "xof", but the
> added line implies it was "xvf" instead.
> 
> Seeing your actual use case confirms that my suggestion below would work
> for you.
> 
> >
> >>> +# # When cross-compiling, define HOST_CPU as the canonical name
> >>> of the
> >> CPU on
> >>> # which the built Git will run (for instance "x86_64").
> >>>
> >>> @@ -452,6 +455,7 @@ LDFLAGS = ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
> >>> ALL_LDFLAGS = $(LDFLAGS) STRIP ?= strip +TAR_EXTRACT_OPTIONS = xof
> >>>
> >>> # Create as necessary, replace existing, make ranlib unneeded.
> >>> ARFLAGS = rcs @@ -2569,7 +2573,7 @@ install: all ifndef NO_GETTEXT
> >>> $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'
> >>> (cd po/build/locale && $(TAR) cf - .) | \ -	(cd
> >>> '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) xof -) + (cd
> >>> '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR)
> >>> +$(TAR_EXTRACT_OPTIONS) -)
> >>
> >> Hmm.  TAR_EXTRACT_OPTIONS always needs to have f (or -f, or --file)
> >> at the end to go together with the following dash, meaning to extract
> >> from stdin. And x (or -x, or --extract) is probably needed in all
> >> cases as well.  So wouldn't it make more sense to only put the o (or
> >> -o, or --no-same-owner) into TAR_EXTRACT_OPTIONS and enforce x and
> f?
> >
> > This is a good suggestion, and I'd love to do that, if I could
> > guarantee a modern tar, which I can't. The platform comes with a
> > really old-school tar from some old (seemingly BSD4.3) epoch that only
> > takes one option set. There is a more modern tar that can be
> > optionally installed if the sysadmin decides to that takes a slightly
> > more modern set, which could support your request, and my team also
> > has a gnu port that is very modern. I can't control what customers are
> > choosing to have installed, unfortunately. Your point is well made and
> > I am completely on board with it, but it introduces a configuration
> > requirement.
> 
> Long options would be nice to nice to have, but are not my main point; I
> included them mainly to spare readers from firing up "man tar" to look up
> the meaning of the short ones.
> 
> I just meant to say that something like this here would make more sense
> because you always need x and f- anyway:
> 
> 	TAR_EXTRACT_OPTIONS = o
> 
> 	... ${TAR} x${TAR_EXTRACT_OPTIONS}f -
> 
> > As with the broadening setbuf (patch 2/6) change, I would like to
> > consider this for the future, having a slightly different more complex
> > idea. I could introduce something like this:
> >
> > 1. HAS_ANCIENT_TAR=UnfortunatelyYes in config.mak.uname that disables
> > this capability all together 2. HAS_ANCIENT_TAR=AreYouKiddingMe
> > (joke) then set up TAR_EXTRACT_ADDITIONAL_OPTIONS above and beyond
> the
> > default, so --file, --no-same-owner would always be in effect for that
> > operation.
> >
> > The micro-project would also, logically, need to apply to other tar
> > occurrences throughout the code and potentially need a test case
> > written for it (not entirely sure what that would test, yet).
> > Is that a reasonable approach?
> 
> As long as old-school dash-less flags suffice for our purposes (including
> yours) we can just keep using that style everywhere and avoid adding more
> settings.  It would be a different matter if we needed features that have no
> short flag, or are only offered by certain tar implementations.

Points taken. I will reissue this particular patch shortly.

Re: [PATCH v2 2/6] Add tar extract install options override in installation processing.

[Ævar Arnfjörð Bjarmason]

On Sat, Jan 20 2018, Randall S. Becker jotted:

> I will reissue this particular patch shortly.

It makes sense to base that submission on the next branch instead of
master. I have a patch queued up there which adds two new tar
invocations.

Also re your commit message see the formatting guide in
Documentation/SubmittingPatches, in particular: instead of:

 - Add a brief subject line

 - Just make the body be a normal paragraph instead of a bullet-point
   list with one item.

RE: [PATCH v2 2/6] Add tar extract install options override in installation processing.

[Randall S. Becker]

On January 20, 2018 3:34 PM, Ævar Arnfjörð Bjarmason
> On Sat, Jan 20 2018, Randall S. Becker jotted:
> 
> > I will reissue this particular patch shortly.
> 
> It makes sense to base that submission on the next branch instead of master.
> I have a patch queued up there which adds two new tar invocations.
> 
> Also re your commit message see the formatting guide in
> Documentation/SubmittingPatches, in particular: instead of:
> 
>  - Add a brief subject line
> 
>  - Just make the body be a normal paragraph instead of a bullet-point
>    list with one item.

Got it. I've been swapping between contribution styles so got a little confused. I'm going to reissue all of the patches required for the NonStop port later tonight or tomorrow, once the test suite run is finished. I'll take everyone's comments into account for that. Thanks for your (and everyone else's) guidance.

Cheers,
Randall

-- Brief whoami:
  NonStop developer since approximately NonStop(211288444200000000)
  UNIX developer since approximately 421664400
-- In my real life, I talk too much.

RE: [PATCH v2 4/6] Force test suite traps to be cleared for NonStop ksh

[Randall S. Becker]

On January 19, 2018 5:29 PM, I wrote:
> On January 19, 2018 4:27 PM, Jeff King wrote:
> > On Fri, Jan 19, 2018 at 12:34:04PM -0500, randall.s.becker@rogers.com
> wrote:
> >
> > > From: "Randall S. Becker" <rsbecker@nexbridge.com>
> > >
> > > * t/lib-git-daemon.sh: fix incompatibilities with ksh traps not being
> > >   cleared automatically on platform. This caused tests to seem to fail
> > >   while actually succeeding.
> > >
> > > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > > ---
> > >  t/lib-git-daemon.sh | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh index
> > > 987d40680..955beecd9 100644
> > > --- a/t/lib-git-daemon.sh
> > > +++ b/t/lib-git-daemon.sh
> > > @@ -68,6 +68,7 @@ start_git_daemon() {
> > >  		test_skip_or_die $GIT_TEST_GIT_DAEMON \
> > >  			"git daemon failed to start"
> > >  	fi
> > > +	trap '' EXIT
> > >  }
> >
> > I don't think this can be right. The way these traps are supposed to work is:
> >
> >   - as soon as test-lib.sh is loaded, we "trap die EXIT" to catch an
> >     accidental death/exit from one of the scripts
> >
> >   - when test_done is called, we clear the trap (i.e., it is OK to exit
> >     now because the script has given us a positive indication that all
> >     tests have been run)
> >
> >   - while the child git-daemon is running, we'd want to clean up after
> >     ourselves. So during start_git_daemon() we add our cleanup (followed
> >     by the original "die", because shell traps don't push onto a stack).
> >     And then at stop_git_daemon(), we restore the original die trap.
> >
> > But this patch means that while git-daemon is running, we have no trap at
> all!
> > That means that a failed test which causes us to exit would leave a
> > child daemon running.
> >
> > Furthermore, both of these functions now drop the 'die' trap entirely,
> > meaning the script would fail to notice premature exit from one of the
> > test snippets.
> >
> > So while this may be papering over a problem on NonStop, I think it's
> > breaking the intent of the traps.
> >
> > I'm not sure what the root of the problem is, but it sounds like ksh
> > may be triggering EXIT traps when we don't expect (during function
> returns?
> > Subshell exits? Other?)
> 
> The "unexpected" EXIT traps are exactly the issue we found when working
> with the platform support team. There was some discussion about what the
> right expectation was, and I was not able to have a change made to ksh on
> the platform. The problem may have a similar (identical?) root cause to the
> Perl exit issues we are also experiencing that is in their hands. I'm currently
> retesting without this change (results in 36 hours ☹ ). Is there a preferred
> way you would like me to bypass this except on NonStop? I can add a check
> based on uname.

After running through the git test suite, it turns out that this particular need has gone away as of the latest OS releases. The test results without the trap '' EXIT are identical to that with the trap (6 breaks that look completion code handling-related on the platform). I'm going to drop the need for this and repackage the entire set of patches applying everyone's comments and removing this (4/6) and the GCC attribute (1/6) patch. This will be followed-up with generalizing the setbuf and tar patches for a broader audience, but I need a bit more time for that generalization.

Please accept my thanks and expect the updated set tomorrow, which will be sufficient to bring the NonStop NSE, NSX, and NSV platforms into the common code-base for git at long last.

Cheers,
Randall

Re: [PATCH v2 0/6] Force pipes to flush immediately on NonStop platform

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> On Fri, Jan 19, 2018 at 12:33:59PM -0500, randall.s.becker@rogers.com wrote:
>> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>> 
>> * wrapper.c: called setbuf(stream,0) to force pipe flushes not enabled by
>>   default on the NonStop platform.
>> 
>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> ---
>>  wrapper.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/wrapper.c b/wrapper.c
>> index d20356a77..671cbb4b4 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -403,6 +403,9 @@ FILE *xfdopen(int fd, const char *mode)
>>  	FILE *stream = fdopen(fd, mode);
>>  	if (stream == NULL)
>>  		die_errno("Out of memory? fdopen failed");
>> +#ifdef __TANDEM
>> +	setbuf(stream,0);
>> +#endif
>
> Reading the commit message, I would have expected someting similar to
>
> #ifdef FORCE_PIPE_FLUSHES
> 	setbuf(stream,0);
> #endif
>
> (Because other systems may need the tweak as well, some day)
> Of course you need to change that in the Makefile and config.mak.uname

I actually wouldn't have expected anything like that after reading
the commit message.  

First I thought it was describing only what it does (i.e. "let's use
setbuf() to set the stream unbuffered on TANDEM"), which is a
useless description that only says what it does which we can read
from the diff, but "NonStop by default creates pipe that does not
flush" is a potentially useful information the log message adds.
But it is just "potentially"---we cannot read what exact problem the
change is trying to address.  Making a pipe totally unbuffered is a
heavy hammer that may not be an appropriate solution---it could be
that we are missing calls to fflush() where we need and have been
lucky because most of the systems we deal with do line-buffered by
default, or something silly/implausible like that, and if that is
the case, a more proper fix may be to add these missing fflush() to
right places.

IOW, I do not see it explained clearly why this change is needed on
any single platform---so "that issue may be shared by others, too"
is a bit premature thing for me to listen to and understand, as
"that issue" is quite unclear to me.

RE: [PATCH v2 0/6] Force pipes to flush immediately on NonStop platform

[Randall S. Becker]

On January 22, 2018 5:36 PM, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
> 
> > On Fri, Jan 19, 2018 at 12:33:59PM -0500, randall.s.becker@rogers.com
> wrote:
> >> From: "Randall S. Becker" <rsbecker@nexbridge.com>
> >>
> >> * wrapper.c: called setbuf(stream,0) to force pipe flushes not enabled by
> >>   default on the NonStop platform.
> >>
> >> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> >> ---
> >>  wrapper.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/wrapper.c b/wrapper.c
> >> index d20356a77..671cbb4b4 100644
> >> --- a/wrapper.c
> >> +++ b/wrapper.c
> >> @@ -403,6 +403,9 @@ FILE *xfdopen(int fd, const char *mode)
> >>  	FILE *stream = fdopen(fd, mode);
> >>  	if (stream == NULL)
> >>  		die_errno("Out of memory? fdopen failed");
> >> +#ifdef __TANDEM
> >> +	setbuf(stream,0);
> >> +#endif
> >
> > Reading the commit message, I would have expected someting similar to
> >
> > #ifdef FORCE_PIPE_FLUSHES
> > 	setbuf(stream,0);
> > #endif
> >
> > (Because other systems may need the tweak as well, some day) Of course
> > you need to change that in the Makefile and config.mak.uname
> 
> I actually wouldn't have expected anything like that after reading the commit
> message.
> 
> First I thought it was describing only what it does (i.e. "let's use
> setbuf() to set the stream unbuffered on TANDEM"), which is a useless
> description that only says what it does which we can read from the diff, but
> "NonStop by default creates pipe that does not flush" is a potentially useful
> information the log message adds.
> But it is just "potentially"---we cannot read what exact problem the change is
> trying to address.  Making a pipe totally unbuffered is a heavy hammer that
> may not be an appropriate solution---it could be that we are missing calls to
> fflush() where we need and have been lucky because most of the systems
> we deal with do line-buffered by default, or something silly/implausible like
> that, and if that is the case, a more proper fix may be to add these missing
> fflush() to right places.
> 
> IOW, I do not see it explained clearly why this change is needed on any single
> platform---so "that issue may be shared by others, too"
> is a bit premature thing for me to listen to and understand, as "that issue" is
> quite unclear to me.

v4 might be a little better. The issue seems to be specific to NonStop that it's PIPE mechanism needs to have setbuf(pipe,NULL) called for git to be happy. The default behaviour appears to be different on NonStop from other platforms from our testing. We get hung up waiting on pipes unless this is done. At the moment, this is platform-specific. Other parts of the discussion led to the conclusion that we should make this available to any platform using a new configuration option, but my objective is to get the NonStop port integrated with the main git code base and when my $DAYJOB permits it, spend the time adding the option. Note: __TANDEM is #define automatically emitted by the NonStop compilers. 

Does that help?

Sincerely,
Randall

Re: [PATCH v2 0/6] Force pipes to flush immediately on NonStop platform

[Junio C Hamano]

"Randall S. Becker" <rsbecker@nexbridge.com> writes:

>> IOW, I do not see it explained clearly why this change is needed on any single
>> platform---so "that issue may be shared by others, too"
>> is a bit premature thing for me to listen to and understand, as "that issue" is
>> quite unclear to me.
>
> v4 might be a little better. The issue seems to be specific to
> NonStop that it's PIPE mechanism needs to have setbuf(pipe,NULL)
> called for git to be happy.  The default behaviour appears to be
> different on NonStop from other platforms from our testing. We get
> hung up waiting on pipes unless this is done.

I am afraid that that is not a "diagnosis" enough to allow us moving
forward.  We get hung up because...?  When the process that has the
other end of pipe open exits, NonStop does not close the pipe
properly?  Or NonStop does not flush the data buffered in the pipe?
Would it help if a compat wrapper that does setbuf(fd, NULL)
immediately before closing the fd, or some other more targetted
mechanism, is used only on NonStop, for example?  Potentially
megabytes of data can pass thru a pipe, and if the platform bug
affects only at the tail end of the transfer, marking the pipe not
to buffer at all at the beginning is too big a hammer to work it
around.  With the explanation given so far, this still smells more
like "we have futzed around without understanding why, and this
happens to work."  It may be good enough for your purpose of making
progress (after all, I'd imagine that you'd need to work this around
one way or another to hunt for and fix more issues on the platform),
but it does not sound like "we know what the problem is, and this is
the best workaround for that", which is what we want if it wants to
become part of the official codebase.

RE: [PATCH v2 0/6] Force pipes to flush immediately on NonStop platform

[Randall S. Becker]

On January 23, 2018 1:13 PM, Junio C Hamano wrote:
> "Randall S. Becker" <rsbecker@nexbridge.com> writes:
> 
> >> IOW, I do not see it explained clearly why this change is needed on
> >> any single platform---so "that issue may be shared by others, too"
> >> is a bit premature thing for me to listen to and understand, as "that
> >> issue" is quite unclear to me.
> >
> > v4 might be a little better. The issue seems to be specific to NonStop
> > that it's PIPE mechanism needs to have setbuf(pipe,NULL) called for
> > git to be happy.  The default behaviour appears to be different on
> > NonStop from other platforms from our testing. We get hung up waiting
> > on pipes unless this is done.
> 
> I am afraid that that is not a "diagnosis" enough to allow us moving forward.
> We get hung up because...?  When the process that has the other end of
> pipe open exits, NonStop does not close the pipe properly?  Or NonStop
> does not flush the data buffered in the pipe?
> Would it help if a compat wrapper that does setbuf(fd, NULL) immediately
> before closing the fd, or some other more targetted mechanism, is used only
> on NonStop, for example?  Potentially megabytes of data can pass thru a
> pipe, and if the platform bug affects only at the tail end of the transfer,
> marking the pipe not to buffer at all at the beginning is too big a hammer to
> work it around.  With the explanation given so far, this still smells more like
> "we have futzed around without understanding why, and this happens to
> work."  It may be good enough for your purpose of making progress (after
> all, I'd imagine that you'd need to work this around one way or another to
> hunt for and fix more issues on the platform), but it does not sound like "we
> know what the problem is, and this is the best workaround for that", which is
> what we want if it wants to become part of the official codebase.

I am retesting without setbuf(NULL) but this is unlikely to be enlightening. The test cases do not adequately simulate the configuration in which my team originally encountered the problem. This requires a guarantee of the source and destination coming through different logical CPUs. We never encountered the issue in the test suite, only when end users got hold of it. We had two distinct problems, one which was the revent=0 related hang (already solved) and other was a stream flush problem. The two are related but distinct. The platform support group insisted that we should have the setbuf(NULL) in place for interprocess communications in the form used here - I'm worried about losing this, but completely aware that this is far too heavy for other platforms (hence the __TANDEM guard in wrapper.c). If the form of the wrapper should be different, I would be happy to comply.

I have a much longer explanation about the platform message stack structure, but that doesn't belong here. Happy to respond privately if requested.

Cheers,
Randall

-- Brief whoami:
 NonStop developer since approximately 211288444200000000
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

RE: [PATCH v2 0/6] Force pipes to flush immediately on NonStop platform

[Randall S. Becker]

On January 23, 2018 1:13 PM, Junio C Hamano wrote:
> 
> "Randall S. Becker" <rsbecker@nexbridge.com> writes:
> 
> >> IOW, I do not see it explained clearly why this change is needed on
> >> any single platform---so "that issue may be shared by others, too"
> >> is a bit premature thing for me to listen to and understand, as "that
> >> issue" is quite unclear to me.
> >
> > v4 might be a little better. The issue seems to be specific to NonStop
> > that it's PIPE mechanism needs to have setbuf(pipe,NULL) called for
> > git to be happy.  The default behaviour appears to be different on
> > NonStop from other platforms from our testing. We get hung up waiting
> > on pipes unless this is done.
> 
> I am afraid that that is not a "diagnosis" enough to allow us moving forward.
> We get hung up because...?  When the process that has the other end of
> pipe open exits, NonStop does not close the pipe properly?  Or NonStop
> does not flush the data buffered in the pipe?
> Would it help if a compat wrapper that does setbuf(fd, NULL) immediately
> before closing the fd, or some other more targetted mechanism, is used only
> on NonStop, for example?  Potentially megabytes of data can pass thru a
> pipe, and if the platform bug affects only at the tail end of the transfer,
> marking the pipe not to buffer at all at the beginning is too big a hammer to
> work it around.  With the explanation given so far, this still smells more like
> "we have futzed around without understanding why, and this happens to
> work."  It may be good enough for your purpose of making progress (after
> all, I'd imagine that you'd need to work this around one way or another to
> hunt for and fix more issues on the platform), but it does not sound like "we
> know what the problem is, and this is the best workaround for that", which is
> what we want if it wants to become part of the official codebase.

As I feared, the test suite was unable to reproduce the issue without setbuf(NULL) - primary because the test structure ends up with both ends of the git dialogs on clone and fetch in the same CPU (even if different IPUs), which does not experience the issue and we can't loop-back through the platform's proprietary SSH. I am not comfortable releasing without it at this stage, but if you don't want to go forward with this fix, my team can run it for a few months internally in the hope that this works out for the better. The situation is timing related and is fine 99.98-ish% of the time. I really do want the setbuf present in any compiled versions that our community might get, primarily because I don't like sleepless nights chasing this down (again).

Cheers,
Randall