[PATCH 1/7] Redirect grep's stderr top null too.

[PATCH 1/7] Redirect grep's stderr top null too.

[Darren Tucker]

Prevents pollution of configure output on platforms that don't have
grep -a.

Signed-off-by: Darren Tucker <dtucker@dtucker.net>
---
 configure.ac | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 66aedb9288..4effc82b76 100644
--- a/configure.ac
+++ b/configure.ac
@@ -526,7 +526,7 @@ if test -n "$ASCIIDOC"; then
 	esac
 fi
 
-if grep -a ascii configure.ac >/dev/null; then
+if grep -a ascii configure.ac >/dev/null 2>&1; then
   AC_MSG_RESULT([Using 'grep -a' for sane_grep])
   SANE_TEXT_GREP=-a
 else
-- 
2.21.3

[PATCH 2/7] Define NO_PREAD for HPUX 11.11 and 11.00.

[Darren Tucker]

Tested on 11.11 but probably needed for 11.00 too.

Signed-off-by: Darren Tucker <dtucker@dtucker.net>
---
 config.mak.uname | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/config.mak.uname b/config.mak.uname
index 5ad43c80b1..e864e04346 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -368,9 +368,14 @@ ifeq ($(uname_S),HP-UX)
 	NO_SYS_SELECT_H = YesPlease
 	SNPRINTF_RETURNS_BOGUS = YesPlease
 	NO_NSEC = YesPlease
+	ifeq ($(uname_R),B.11.11)
+		NO_PREAD = YesPlease
+		CFLAGS = -D_REENTRANT
+	endif
 	ifeq ($(uname_R),B.11.00)
 		NO_INET_NTOP = YesPlease
 		NO_INET_PTON = YesPlease
+		NO_PREAD = YesPlease
 	endif
 	ifeq ($(uname_R),B.10.20)
 		# Override HP-UX 11.x setting:
-- 
2.21.3

[PATCH 3/7] Test for strtoull in configure.

[Darren Tucker]

It's already used in compat/strtoimax.c

Signed-off-by: Darren Tucker <dtucker@dtucker.net>
---
 configure.ac | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/configure.ac b/configure.ac
index 4effc82b76..14e09b04b6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1158,6 +1158,12 @@ AC_CHECK_TYPE(uintmax_t,
 ])
 GIT_CONF_SUBST([NO_UINTMAX_T])
 #
+# Define NO_STRTOULL if you don't have strtoull in the C library.
+GIT_CHECK_FUNC(strtoull,
+[NO_STRTOULL=],
+[NO_STRTOULL=YesPlease])
+GIT_CONF_SUBST([NO_STRTOULL])
+#
 # Define NO_STRTOUMAX if you don't have strtoumax in the C library.
 GIT_CHECK_FUNC(strtoumax,
 [NO_STRTOUMAX=],
-- 
2.21.3

[PATCH 4/7] Use strtoumax instead of strtoull.

[Darren Tucker]

strtoumax is in the compat library so this works on platforms that don't
have a native strtoull.

Signed-off-by: Darren Tucker <dtucker@dtucker.net>
---
 t/helper/test-progress.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
index 5d05cbe789..3e9eb2abe3 100644
--- a/t/helper/test-progress.c
+++ b/t/helper/test-progress.c
@@ -47,7 +47,7 @@ int cmd__progress(int argc, const char **argv)
 		char *end;
 
 		if (skip_prefix(line.buf, "progress ", (const char **) &end)) {
-			uint64_t item_count = strtoull(end, &end, 10);
+			uint64_t item_count = strtoumax(end, &end, 10);
 			if (*end != '\0')
 				die("invalid input: '%s'\n", line.buf);
 			display_progress(progress, item_count);
@@ -55,10 +55,10 @@ int cmd__progress(int argc, const char **argv)
 				       (const char **) &end)) {
 			uint64_t byte_count, test_ms;
 
-			byte_count = strtoull(end, &end, 10);
+			byte_count = strtoumax(end, &end, 10);
 			if (*end != ' ')
 				die("invalid input: '%s'\n", line.buf);
-			test_ms = strtoull(end + 1, &end, 10);
+			test_ms = strtoumax(end + 1, &end, 10);
 			if (*end != '\0')
 				die("invalid input: '%s'\n", line.buf);
 			progress_test_ns = test_ms * 1000 * 1000;
-- 
2.21.3

Re: [PATCH 4/7] Use strtoumax instead of strtoull.

[Junio C Hamano]

Darren Tucker <dtucker@dtucker.net> writes:

> strtoumax is in the compat library so this works on platforms that don't
> have a native strtoull.

The above description (even though is a good one) alone would leave
the reader wondering if this patch now has removed the last use of
strtoull(), removing the need to have NO_STRTOULL in the Makefile
and configure.ac, and also wonder why the patch does not touch
Makefile etc.  Something like

	We still use strtoull() as a part of the compatibility
	implementation of strtoumax(), so we cannot remove the
	support to detect/configure the use of the function in the
	Makefile and autoconf, though.

may deserve to be made into the second paragraph that follows the
above description.

Shouldn't the type of some variables in the codepath also be changed
to make sure we receive the return value from the function in a
variable that is large enough?

Thanks.

> Signed-off-by: Darren Tucker <dtucker@dtucker.net>
> ---
>  t/helper/test-progress.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
> index 5d05cbe789..3e9eb2abe3 100644
> --- a/t/helper/test-progress.c
> +++ b/t/helper/test-progress.c
> @@ -47,7 +47,7 @@ int cmd__progress(int argc, const char **argv)
>  		char *end;
>  
>  		if (skip_prefix(line.buf, "progress ", (const char **) &end)) {
> -			uint64_t item_count = strtoull(end, &end, 10);
> +			uint64_t item_count = strtoumax(end, &end, 10);
>  			if (*end != '\0')
>  				die("invalid input: '%s'\n", line.buf);
>  			display_progress(progress, item_count);
> @@ -55,10 +55,10 @@ int cmd__progress(int argc, const char **argv)
>  				       (const char **) &end)) {
>  			uint64_t byte_count, test_ms;
>  
> -			byte_count = strtoull(end, &end, 10);
> +			byte_count = strtoumax(end, &end, 10);
>  			if (*end != ' ')
>  				die("invalid input: '%s'\n", line.buf);
> -			test_ms = strtoull(end + 1, &end, 10);
> +			test_ms = strtoumax(end + 1, &end, 10);
>  			if (*end != '\0')
>  				die("invalid input: '%s'\n", line.buf);
>  			progress_test_ns = test_ms * 1000 * 1000;

[PATCH 5/7] Define LLONG_MAX/MIN etc if not already defined.

[Darren Tucker]

Signed-off-by: Darren Tucker <dtucker@dtucker.net>
---
 git-compat-util.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 8ba576e81e..aa221eba1b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -127,6 +127,22 @@
 /* Approximation of the length of the decimal representation of this type. */
 #define decimal_length(x)	((int)(sizeof(x) * 2.56 + 0.5) + 1)
 
+#ifndef SIZE_MAX
+#define SIZE_MAX ((size_t)maximum_signed_value_of_type(size_t))
+#endif
+
+#ifndef LLONG_MIN
+#define LLONG_MIN ((long long)minimum_signed_value_of_type(long long))
+#endif
+
+#ifndef LLONG_MAX
+#define LLONG_MAX ((long long)maximum_signed_value_of_type(long long))
+#endif
+
+#ifndef ULLONG_MAX
+#define ULLONG_MAX ((unsigned long long)maximum_unsigned_value_of_type(unsigned long long))
+#endif
+
 #if defined(__sun__)
  /*
   * On Solaris, when _XOPEN_EXTENDED is set, its header file
-- 
2.21.3

Re: [PATCH 5/7] Define LLONG_MAX/MIN etc if not already defined.

[Andreas Schwab]

On Mai 18 2020, Darren Tucker wrote:

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 8ba576e81e..aa221eba1b 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -127,6 +127,22 @@
>  /* Approximation of the length of the decimal representation of this type. */
>  #define decimal_length(x)	((int)(sizeof(x) * 2.56 + 0.5) + 1)
>  
> +#ifndef SIZE_MAX
> +#define SIZE_MAX ((size_t)maximum_signed_value_of_type(size_t))

size_t is an unsigned type.

> +#endif
> +
> +#ifndef LLONG_MIN
> +#define LLONG_MIN ((long long)minimum_signed_value_of_type(long long))
> +#endif
> +
> +#ifndef LLONG_MAX
> +#define LLONG_MAX ((long long)maximum_signed_value_of_type(long long))
> +#endif
> +
> +#ifndef ULLONG_MAX
> +#define ULLONG_MAX ((unsigned long long)maximum_unsigned_value_of_type(unsigned long long))
> +#endif

These definitions won't work in the preprocessor.  The are no current
uses of these macros in preprocessor directives, but that needs to be
kept in mind.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 5/7] Define LLONG_MAX/MIN etc if not already defined.

[Darren Tucker]

On Mon, 18 May 2020 at 20:24, Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Mai 18 2020, Darren Tucker wrote:
[...]
> > +#define SIZE_MAX ((size_t)maximum_signed_value_of_type(size_t))
>
> size_t is an unsigned type.

indeed; missed that in my cut and pasting.

[...]
> These definitions won't work in the preprocessor.  The are no current
> uses of these macros in preprocessor directives, but that needs to be
> kept in mind.

I originally had these defined to LONG_LONG_MAX and ULONG_LONG_MAX
which it (HP-UX 11.11) has.  It's also got SSIZE_MAX but not SIZE_MAX,
so I originally had

#if !defined(SIZE_MAX) && defined(SSIZE_MAX) && (SSIZE_MAX == LONG_MAX)
# define SIZE_MAX ULONG_MAX
#endif

which would work in macros, but I changed it at the last minute to use
those other macros since it seemed more likely to work on any other
systems that had this problem.

-- 
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860  37F4 9357 ECEF 11EA A6FA (new)
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.

[PATCH 6/7] Check if strtoumax is a macro (eg HP-UX 11.11).

[Darren Tucker]

Signed-off-by: Darren Tucker <dtucker@dtucker.net>
---
 configure.ac | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 14e09b04b6..87a39c5ae0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1164,10 +1164,16 @@ GIT_CHECK_FUNC(strtoull,
 [NO_STRTOULL=YesPlease])
 GIT_CONF_SUBST([NO_STRTOULL])
 #
-# Define NO_STRTOUMAX if you don't have strtoumax in the C library.
+# Define NO_STRTOUMAX if you don't have strtoumax in the C library
+# or as a macro in inttypes.h.
 GIT_CHECK_FUNC(strtoumax,
 [NO_STRTOUMAX=],
-[NO_STRTOUMAX=YesPlease])
+[
+	AC_CHECK_DECL(strtoumax,
+	[NO_STRTOUMAX=],
+	[NO_STRTOUMAX=YesPlease],
+	[#include <inttypes.h>])
+])
 GIT_CONF_SUBST([NO_STRTOUMAX])
 #
 # Define NO_SETENV if you don't have setenv in the C library.
-- 
2.21.3

Re: [PATCH 6/7] Check if strtoumax is a macro (eg HP-UX 11.11).

[Junio C Hamano]

Darren Tucker <dtucker@dtucker.net> writes:

> Signed-off-by: Darren Tucker <dtucker@dtucker.net>
> ---
>  configure.ac | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 14e09b04b6..87a39c5ae0 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1164,10 +1164,16 @@ GIT_CHECK_FUNC(strtoull,
>  [NO_STRTOULL=YesPlease])
>  GIT_CONF_SUBST([NO_STRTOULL])
>  #
> -# Define NO_STRTOUMAX if you don't have strtoumax in the C library.
> +# Define NO_STRTOUMAX if you don't have strtoumax in the C library
> +# or as a macro in inttypes.h.
>  GIT_CHECK_FUNC(strtoumax,
>  [NO_STRTOUMAX=],
> -[NO_STRTOUMAX=YesPlease])
> +[
> +	AC_CHECK_DECL(strtoumax,
> +	[NO_STRTOUMAX=],
> +	[NO_STRTOUMAX=YesPlease],
> +	[#include <inttypes.h>])
> +])

It is kind of surprising that we got away with GIT_CHECK_FUNC() that
misses an implementation by macro without having problems with other
symbols.  

I don't mind taking this patch as-is, but it makes me wonder if we
need to devise a more systematic approach to the issue than "oh, I
found GIT_CHECK_FUNC() does not work for X on system Y, so let's add
an AC_CHECK_DECL() for it, too" approach, which this patch is its
first step.

Thanks.

Re: [PATCH 6/7] Check if strtoumax is a macro (eg HP-UX 11.11).

[Darren Tucker]

On Tue, 19 May 2020 at 03:17, Junio C Hamano <gitster@pobox.com> wrote:
[...]
> It is kind of surprising that we got away with GIT_CHECK_FUNC() that
> misses an implementation by macro without having problems with other
> symbols.

I suspect that in most cases you'll go to the fallback and be fine.
In this case, it falls back to strtoull which it doesn't have. (Adding
the strtoull test in patch 3/7 makes it fall back to strtoul which it
does have).

The other annoyance is that it produces a macro redefinition warning
on each file.  An alternative solution to that would be to undef
strtoumax in git-compat-util.h before redefining it.

> I don't mind taking this patch as-is, but it makes me wonder if we
> need to devise a more systematic approach to the issue than "oh, I
> found GIT_CHECK_FUNC() does not work for X on system Y, so let's add
> an AC_CHECK_DECL() for it, too" approach, which this patch is its
> first step.

The caveat with putting it in GIT_CHECK_FUNC() is that you'll need to
include the union of all of the header files that might have all of
the things you're looking for instead of the hopefully small set of
them that might have the one specific thing you're looking for.  I
suspect that would be more hassle than it's worth.

-- 
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860  37F4 9357 ECEF 11EA A6FA (new)
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.

Re: [PATCH 6/7] Check if strtoumax is a macro (eg HP-UX 11.11).

[Junio C Hamano]

Darren Tucker <dtucker@dtucker.net> writes:

> The caveat with putting it in GIT_CHECK_FUNC() is that you'll need to
> include the union of all of the header files that might have all of
> the things you're looking for instead of the hopefully small set of
> them that might have the one specific thing you're looking for.  I
> suspect that would be more hassle than it's worth.

Oh, what I had in mind was to replace GIT_CHECK_FUNC() with a macro
that takes not just function name but also include file.  Not "the
union of all the include files".

[PATCH 7/7] Define SCNuMAX if not already defined.

[Darren Tucker]

Signed-off-by: Darren Tucker <dtucker@dtucker.net>
---
 git-compat-util.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index aa221eba1b..2bc468fbd0 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -342,6 +342,10 @@ typedef uintmax_t timestamp_t;
 #define TIME_MAX UINTMAX_MAX
 #define TIME_MIN 0
 
+#if !defined(SCNuMAX) && defined(SCNu64)
+#define SCNuMAX SCNu64
+#endif
+
 #ifndef PATH_SEP
 #define PATH_SEP ':'
 #endif
-- 
2.21.3

Re: [PATCH 1/7] Redirect grep's stderr top null too.

[Đoàn Trần Công Danh]

Hi Darren,

On 2020-05-18 20:03:50+1000, Darren Tucker <dtucker@dtucker.net> wrote:
> Prevents pollution of configure output on platforms that don't have
> grep -a.

From your other's patch, I think you're in HP-UX,
would you mind also run the test.

Since t5703 also uses "grep -a"

> 
> Signed-off-by: Darren Tucker <dtucker@dtucker.net>
> ---
>  configure.ac | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 66aedb9288..4effc82b76 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -526,7 +526,7 @@ if test -n "$ASCIIDOC"; then
>  	esac
>  fi
>  
> -if grep -a ascii configure.ac >/dev/null; then
> +if grep -a ascii configure.ac >/dev/null 2>&1; then
>    AC_MSG_RESULT([Using 'grep -a' for sane_grep])
>    SANE_TEXT_GREP=-a
>  else
> -- 
> 2.21.3
> 

-- 
Danh

Re: [PATCH 1/7] Redirect grep's stderr top null too.

[Darren Tucker]

On Tue, 19 May 2020 at 00:13, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
>
> Hi Darren,
>
> On 2020-05-18 20:03:50+1000, Darren Tucker <dtucker@dtucker.net> wrote:
> > Prevents pollution of configure output on platforms that don't have
> > grep -a.
>
> From your other's patch, I think you're in HP-UX,

Yes (it's not my usual platform but I had occasion to test something
on it so revived an old system).

> would you mind also run the test.
>
> Since t5703 also uses "grep -a"

It fails with:
$ ./t5703-upload-pack-ref-in-want.sh
sed: There are too many commands for the s/\n// function.
ok 1 - setup repository
sed: There are too many commands for the s/\n// function.
not ok 2 - config controls ref-in-want advertisement
#
#               test-tool serve-v2 --advertise-capabilities >out &&
#               ! grep -a ref-in-want out &&
#
#               git config uploadpack.allowRefInWant false &&
#               test-tool serve-v2 --advertise-capabilities >out &&
#               ! grep -a ref-in-want out &&
#
#               git config uploadpack.allowRefInWant true &&
#               test-tool serve-v2 --advertise-capabilities >out &&
#               grep -a ref-in-want out
#
sed: There are too many commands for the s/\n// function.
ok 3 - invalid want-ref line
sed: There are too many commands for the s/\n// function.
ok 4 - basic want-ref
sed: There are too many commands for the s/\n// function.
ok 5 - multiple want-ref lines
sed: There are too many commands for the s/\n// function.
ok 6 - mix want and want-ref
sed: There are too many commands for the s/\n// function.
ok 7 - want-ref with ref we already have commit for
sed: There are too many commands for the s/\n// function.
FATAL: Unexpected exit with code 0

If I use gnu sed and native grep, only #2 fails as above.  If I use
gnu grep and gnu sed it passes with a warning from (gnu) printf:
$ ./t5703-upload-pack-ref-in-want.sh
printf: \3: invalid escape
ok 1 - setup repository
ok 2 - config controls ref-in-want advertisement
ok 3 - invalid want-ref line
ok 4 - basic want-ref
ok 5 - multiple want-ref lines
ok 6 - mix want and want-ref
ok 7 - want-ref with ref we already have commit for
ok 8 - setup repos for fetching with ref-in-want tests
ok 9 - fetching with exact OID
ok 10 - fetching multiple refs
ok 11 - fetching ref and exact OID
ok 12 - fetching with wildcard that does not match any refs
ok 13 - fetching with wildcard that matches multiple refs
# passed all 13 test(s)
# SKIP skipping test, git built without http support
1..13

Maybe you want AC_PROG_GREP?  That picks the gnu grep in /usr/local,
but plumbing it in is more involved.

-- 
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860  37F4 9357 ECEF 11EA A6FA (new)
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.

Re: [PATCH 1/7] Redirect grep's stderr top null too.

[Đoàn Trần Công Danh]

On 2020-05-19 00:29:47+1000, Darren Tucker <dtucker@dtucker.net> wrote:
> On Tue, 19 May 2020 at 00:13, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> >
> > Hi Darren,
> >
> > On 2020-05-18 20:03:50+1000, Darren Tucker <dtucker@dtucker.net> wrote:
> > > Prevents pollution of configure output on platforms that don't have
> > > grep -a.
> >
> > From your other's patch, I think you're in HP-UX,
> 
> Yes (it's not my usual platform but I had occasion to test something
> on it so revived an old system).
> 
> > would you mind also run the test.
> >
> > Since t5703 also uses "grep -a"
> 
> It fails with:
> $ ./t5703-upload-pack-ref-in-want.sh
> sed: There are too many commands for the s/\n// function.

I think this was introduced at 878f988350 (t/test-lib: teach
--chain-lint to detect broken &&-chains in subshells, 2018-07-11)

The chainlint.sed is too complicated for a mortal like me to
understand, I added Eric to Cc.

> ok 1 - setup repository
> sed: There are too many commands for the s/\n// function.
> not ok 2 - config controls ref-in-want advertisement
> #
> #               test-tool serve-v2 --advertise-capabilities >out &&
> #               ! grep -a ref-in-want out &&
> #
> #               git config uploadpack.allowRefInWant false &&
> #               test-tool serve-v2 --advertise-capabilities >out &&
> #               ! grep -a ref-in-want out &&
> #
> #               git config uploadpack.allowRefInWant true &&
> #               test-tool serve-v2 --advertise-capabilities >out &&
> #               grep -a ref-in-want out

I'm thinking about:
-----------8<------------
diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index a34460f7d8..92ad5eeec0 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -49,15 +49,18 @@ test_expect_success 'setup repository' '
 
 test_expect_success 'config controls ref-in-want advertisement' '
 	test-tool serve-v2 --advertise-capabilities >out &&
-	! grep -a ref-in-want out &&
+	perl -ne "/ref-in-want/ and print" out >out.filter &&
+	test_must_be_empty out.filter &&
 
 	git config uploadpack.allowRefInWant false &&
 	test-tool serve-v2 --advertise-capabilities >out &&
-	! grep -a ref-in-want out &&
+	perl -ne "/ref-in-want/ and print" out >out.filter &&
+	test_must_be_empty out.filter &&
 
 	git config uploadpack.allowRefInWant true &&
 	test-tool serve-v2 --advertise-capabilities >out &&
-	grep -a ref-in-want out
+	perl -ne "/ref-in-want/ and print" out >out.filter &&
+	test_file_not_empty out.filter
 '
 
 test_expect_success 'invalid want-ref line' '
------------------->8-----------

> #
> sed: There are too many commands for the s/\n// function.
> ok 3 - invalid want-ref line
> sed: There are too many commands for the s/\n// function.
> ok 4 - basic want-ref
> sed: There are too many commands for the s/\n// function.
> ok 5 - multiple want-ref lines
> sed: There are too many commands for the s/\n// function.
> ok 6 - mix want and want-ref
> sed: There are too many commands for the s/\n// function.
> ok 7 - want-ref with ref we already have commit for
> sed: There are too many commands for the s/\n// function.
> FATAL: Unexpected exit with code 0
> 
> If I use gnu sed and native grep, only #2 fails as above.  If I use
> gnu grep and gnu sed it passes with a warning from (gnu) printf:
> $ ./t5703-upload-pack-ref-in-want.sh
> printf: \3: invalid escape

Look like HP-UX's printf doesn't understand octal escape.
This may be a problem, since we've a patch to fix git-bisect's replace
for Windows, which is using "printf \015"
Cc-ing Junio for checking.


-- 
Danh

Re: [PATCH 1/7] Redirect grep's stderr top null too.

[Darren Tucker]

On Tue, 19 May 2020 at 01:30, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
[...]
> > printf: \3: invalid escape
>
> Look like HP-UX's printf doesn't understand octal escape.

The HP-UX one is actually OK with that.  The error is from an old gnu
coreutils (2.0), and it's complaining because there no leading zero,
which POSIX says octal escapes have:
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/printf.html

"""
The following <backslash>-escape sequences shall be supported:
[...]
"\0ddd", where ddd is a zero, one, two, or three-digit octal number
that shall be converted to a byte with the numeric value specified by
the octal number
"""

$ /usr/local/bin/printf '\03' | od -x
0000000 0300
0000001
$ /usr/local/bin/printf '\3' | od -x
/usr/local/bin/printf: \3: invalid escape
0000000

-- 
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860  37F4 9357 ECEF 11EA A6FA (new)
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.

[PATCH] t5703: replace "grep -a" usage by perl

[Đoàn Trần Công Danh]

On 2020-05-19 09:22:01+1000, Darren Tucker <dtucker@dtucker.net> wrote:
> On Tue, 19 May 2020 at 01:30, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> [...]
> > > printf: \3: invalid escape
> >
> > Look like HP-UX's printf doesn't understand octal escape.
> 
> The HP-UX one is actually OK with that.  The error is from an old gnu
> coreutils (2.0), and it's complaining because there no leading zero,
> which POSIX says octal escapes have:
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/printf.html

I think it's better to use HP-UX native tools for the test.
Can you check with this patch applied on top of your tree.
-------------8<------------
From: =?UTF-8?q?=C4=90o=C3=A0n=20Tr=E1=BA=A7n=20C=C3=B4ng=20Danh?=
 <congdanhqx@gmail.com>
Date: Tue, 19 May 2020 07:50:46 +0700
Subject: [PATCH] t5703: replace "grep -a" usage by perl
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On some platforms likes HP-UX, grep(1) doesn't understand "-a".

Let's switch to perl.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---

Notes:
    We can also copy-and-paste code from t4019,
    to avoid introduce perl to this test.

 t/t5703-upload-pack-ref-in-want.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index a34460f7d8..92ad5eeec0 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -49,15 +49,18 @@ test_expect_success 'setup repository' '
 
 test_expect_success 'config controls ref-in-want advertisement' '
 	test-tool serve-v2 --advertise-capabilities >out &&
-	! grep -a ref-in-want out &&
+	perl -ne "/ref-in-want/ and print" out >out.filter &&
+	test_must_be_empty out.filter &&
 
 	git config uploadpack.allowRefInWant false &&
 	test-tool serve-v2 --advertise-capabilities >out &&
-	! grep -a ref-in-want out &&
+	perl -ne "/ref-in-want/ and print" out >out.filter &&
+	test_must_be_empty out.filter &&
 
 	git config uploadpack.allowRefInWant true &&
 	test-tool serve-v2 --advertise-capabilities >out &&
-	grep -a ref-in-want out
+	perl -ne "/ref-in-want/ and print" out >out.filter &&
+	test_file_not_empty out.filter
 '
 
 test_expect_success 'invalid want-ref line' '
-- 
2.27.0.rc0.4.gf5e7b24084


-- 
Danh

Re: [PATCH] t5703: replace "grep -a" usage by perl

[Darren Tucker]

On Tue, 19 May 2020 at 10:56, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> On 2020-05-19 09:22:01+1000, Darren Tucker <dtucker@dtucker.net> wrote:
> > On Tue, 19 May 2020 at 01:30, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> > [...]
> > > > printf: \3: invalid escape
> > >
> > > Look like HP-UX's printf doesn't understand octal escape.
> >
> > The HP-UX one is actually OK with that.  The error is from an old gnu
> > coreutils (2.0), and it's complaining because there no leading zero,
> > which POSIX says octal escapes have:
> > https://pubs.opengroup.org/onlinepubs/9699919799/utilities/printf.html

I've replaced the printf in question.  You could add leading zeroes in
case anything else strictly conforms to POSIX but it looks like
there's quite a lot of them.

> I think it's better to use HP-UX native tools for the test.
> Can you check with this patch applied on top of your tree.
[...]
> On some platforms likes HP-UX, grep(1) doesn't understand "-a".

nit: "like HP-UX"

Yep that works:

$ PATH=/usr/local/bin:$PATH ./t5703-upload-pack-ref-in-want.sh
ok 1 - setup repository
ok 2 - config controls ref-in-want advertisement
ok 3 - invalid want-ref line
ok 4 - basic want-ref
ok 5 - multiple want-ref lines
ok 6 - mix want and want-ref
ok 7 - want-ref with ref we already have commit for
ok 8 - setup repos for fetching with ref-in-want tests
ok 9 - fetching with exact OID
ok 10 - fetching multiple refs
ok 11 - fetching ref and exact OID
ok 12 - fetching with wildcard that does not match any refs
ok 13 - fetching with wildcard that matches multiple refs
# passed all 13 test(s)
# SKIP skipping test, git built without http support
1..13

Thanks.

-- 
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860  37F4 9357 ECEF 11EA A6FA (new)
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.

Re: [PATCH] t5703: replace "grep -a" usage by perl

[Christian Couder]

On Tue, May 19, 2020 at 3:00 AM Đoàn Trần Công Danh
<congdanhqx@gmail.com> wrote:

> On some platforms likes HP-UX, grep(1) doesn't understand "-a".
>
> Let's switch to perl.

There are some other platforms where Perl is difficult to install.
That's why we have the 'NO_PERL' Makefile knob, and the 'PERL' test
prereq which is used in some test scripts like t0021-conversion.sh,
t2016-checkout-patch.sh, t2071-restore-patch.sh, ...

>     We can also copy-and-paste code from t4019,
>     to avoid introduce perl to this test.

This might be a good idea.

>  t/t5703-upload-pack-ref-in-want.sh | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
> index a34460f7d8..92ad5eeec0 100755
> --- a/t/t5703-upload-pack-ref-in-want.sh
> +++ b/t/t5703-upload-pack-ref-in-want.sh
> @@ -49,15 +49,18 @@ test_expect_success 'setup repository' '
>
>  test_expect_success 'config controls ref-in-want advertisement' '

At least you should add the PERL prereq to the test with something like:

test_expect_success PERL 'config controls ref-in-want advertisement' '

>         test-tool serve-v2 --advertise-capabilities >out &&
> -       ! grep -a ref-in-want out &&
> +       perl -ne "/ref-in-want/ and print" out >out.filter &&
> +       test_must_be_empty out.filter &&

Thanks,
Christian.

Re: [PATCH] t5703: replace "grep -a" usage by perl

[Đoàn Trần Công Danh]

On 2020-05-19 09:13:19+0200, Christian Couder <christian.couder@gmail.com> wrote:
> On Tue, May 19, 2020 at 3:00 AM Đoàn Trần Công Danh
> <congdanhqx@gmail.com> wrote:

> 
> >     We can also copy-and-paste code from t4019,
> >     to avoid introduce perl to this test.
> 
> This might be a good idea.

Probably we need 2 patches, one to libify the current usage in t4019,
one to replace "grep -a" usage in t5703.

Consider this is floating for a long time, I think we can hold this
for the time being, until after 2.27.0 released.

> >  t/t5703-upload-pack-ref-in-want.sh | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
> > index a34460f7d8..92ad5eeec0 100755
> > --- a/t/t5703-upload-pack-ref-in-want.sh
> > +++ b/t/t5703-upload-pack-ref-in-want.sh
> > @@ -49,15 +49,18 @@ test_expect_success 'setup repository' '
> >
> >  test_expect_success 'config controls ref-in-want advertisement' '
> 
> At least you should add the PERL prereq to the test with something like:
> 
> test_expect_success PERL 'config controls ref-in-want advertisement' '

Well, that's is not t/README says:

 - PERL

   Git wasn't compiled with NO_PERL=YesPlease.

   Even without the PERL prerequisite, tests can assume there is a
   usable perl interpreter at $PERL_PATH, though it need not be
   particularly modern.

And, there're a lot of tests using perl without PERL prereq:

	$ git grep -l '[        ]perl ' 't/t????-*.sh' | wc -l
	55
	$ git grep -l PERL 't/t????-*.sh' | wc -l
	35


-- 
Danh

Re: [PATCH] t5703: replace "grep -a" usage by perl

[Christian Couder]

On Tue, May 19, 2020 at 1:39 PM Đoàn Trần Công Danh
<congdanhqx@gmail.com> wrote:
>
> On 2020-05-19 09:13:19+0200, Christian Couder <christian.couder@gmail.com> wrote:
> > On Tue, May 19, 2020 at 3:00 AM Đoàn Trần Công Danh
> > <congdanhqx@gmail.com> wrote:

> > >  test_expect_success 'config controls ref-in-want advertisement' '
> >
> > At least you should add the PERL prereq to the test with something like:
> >
> > test_expect_success PERL 'config controls ref-in-want advertisement' '
>
> Well, that's is not t/README says:
>
>  - PERL
>
>    Git wasn't compiled with NO_PERL=YesPlease.
>
>    Even without the PERL prerequisite, tests can assume there is a
>    usable perl interpreter at $PERL_PATH, though it need not be
>    particularly modern.
>
> And, there're a lot of tests using perl without PERL prereq:
>
>         $ git grep -l '[        ]perl ' 't/t????-*.sh' | wc -l
>         55
>         $ git grep -l PERL 't/t????-*.sh' | wc -l
>         35

Ok, sorry about the noise then.

Re: [PATCH 1/7] Redirect grep's stderr top null too.

[Eric Sunshine]

On Mon, May 18, 2020 at 11:30 AM Đoàn Trần Công Danh
<congdanhqx@gmail.com> wrote:
> On 2020-05-19 00:29:47+1000, Darren Tucker <dtucker@dtucker.net> wrote:
> > $ ./t5703-upload-pack-ref-in-want.sh
> > sed: There are too many commands for the s/\n// function.
>
> I think this was introduced at 878f988350 (t/test-lib: teach
> --chain-lint to detect broken &&-chains in subshells, 2018-07-11)
>
> The chainlint.sed is too complicated for a mortal like me to
> understand, I added Eric to Cc.

That's a rather weird error message; seems like that 'sed' is somewhat broken.

Back when Ævar was trying to get chain-lint to work on some really old
and broken platforms, it was ultimately decided (if I recall
correctly) that it wasn't worth the effort, and that chain-lint should
simply be disabled via GIT_TEST_CHAIN_LINT=0 or --no-chain-lint on
those platforms.

After all, chain-lint exists only to ferret out a specific problem in
_newly-written_ tests (it's testing the tests), not to ferret out
problems in Git functionality (that's what the test suite itself is
for). So it's not a great loss to disable chain-lint on an old or
broken platform on which it is unlikely someone will be developing
_new_ tests. (And, even if someone does write a test on such a
platform, &&-chain breakage will be discovered soon enough once the
patch is posted to the mailing list and someone runs it on a
non-broken platform.)

Re: [PATCH 1/7] Redirect grep's stderr top null too.

[Junio C Hamano]

Darren Tucker <dtucker@dtucker.net> writes:

> Subject: Re: [PATCH 1/7] Redirect grep's stderr top null too.

Our patch title typically looks like

	area: title without initial Capitalization and full stop

Perhaps

	Subject: autoconf: redirect grep's stderr to null

> Prevents pollution of configure output on platforms that don't have
> grep -a.

Our log message typically describes the state without this patch in
the current tense to highlight the problem being solved, and then
orders the person who is updating the system to "make the codebase
like so", e.g.

	When the tested platform's 'grep' does not support the '-a'
	option, an error message would be given to its standard
	error stream, polluting the output.

	Redirect the error message to /dev/null, in addition to the
	standard output, to squelch it.

or something along that style.

Thanks.

>
> Signed-off-by: Darren Tucker <dtucker@dtucker.net>
> ---
>  configure.ac | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/configure.ac b/configure.ac
> index 66aedb9288..4effc82b76 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -526,7 +526,7 @@ if test -n "$ASCIIDOC"; then
>  	esac
>  fi
>  
> -if grep -a ascii configure.ac >/dev/null; then
> +if grep -a ascii configure.ac >/dev/null 2>&1; then
>    AC_MSG_RESULT([Using 'grep -a' for sane_grep])
>    SANE_TEXT_GREP=-a
>  else