patch, 7 partsUse strtoumax instead of strtoull.

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

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

Darren Tucker <dtucker@dtucker.net> writes:

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>
---
 git-compat-util.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

On Mai 18 2020, Darren Tucker wrote:

size_t is an unsigned type.

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.

On Mon, 18 May 2020 at 20:24, Andreas Schwab <schwab@linux-m68k.org> wrote:

[...]

indeed; missed that in my cut and pasting.

[...]

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.

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

Darren Tucker <dtucker@dtucker.net> writes:

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.

On Tue, 19 May 2020 at 03:17, Junio C Hamano <gitster@pobox.com> wrote: [...]

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.

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@dtucker.net> writes:

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

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

It's already used in compat/strtoimax.c

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

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

Hi Darren,

On 2020-05-18 20:03:50+1000, Darren Tucker <dtucker@dtucker.net> wrote:

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"

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

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

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.

On 2020-05-19 00:29:47+1000, Darren Tucker <dtucker@dtucker.net> wrote:

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.

I'm thinking about: -----------8<------------

On Tue, 19 May 2020 at 01:30, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote: [...]

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

On 2020-05-19 09:22:01+1000, Darren Tucker <dtucker@dtucker.net> wrote:

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

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

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.

[...]

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.

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

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

This might be a good idea.

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

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

Thanks, Christian.

On 2020-05-19 09:13:19+0200, Christian Couder <christian.couder@gmail.com> wrote:

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.

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

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

Ok, sorry about the noise then.

On Mon, May 18, 2020 at 11:30 AM Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

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

Darren Tucker <dtucker@dtucker.net> writes:

Our patch title typically looks like

	area: title without initial Capitalization and full stop

Perhaps

	Subject: autoconf: redirect grep's stderr to null

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.