threads / discuss / 36316

AIX fixes

Subject: AIX fixes

## tl;dr

6 messages between Mar 29, 2014 and Mar 31, 2014.

replies: 5people: 3as markdown or json

Charles Bailey· Mar 29, 2014, 15:38 UTC · lore
[PATCH 1/2] Remove inline from git_fnmatch in dir.c

There are currently a few issues with building on AIX. These two patches address two of them. The first removes 'inline' from a function in dir.c. The function has grown such that I don't really see a benefit in explicitly encouraging the compiler to inline. (As it is in a .c file, it's only going to be inlined for sophisticated toolchains doing LTO or similar for other translation units and with this sophistication the inline hinting behaviour is probably not so important.)

The problem with having this function declared inline is a compliance issue.

6.7.4 of C99 says:
> An inline definition of a function with external linkage shall not
> contain a definition of a modifiable object with static storage
> duration, and shall not contain a reference to an identifier with
> internal linkage.

git_fnmatch contains calls to ps_strncmp and ps_strcmp which are all declared static so violate this and xlC complains about this.

[PATCH 2/2] Don't rely on strerror text when testing rmdir failure

The second issue is that AIX doesn't distinguish between EEXIST and WNOTEMPTY so two tests that rely on the exact text of strerror for rmdir's failure to remove a non-empty directory fail. My personal take was that the exact text of strerror was not too important but we can test the leading portion of the error message which is under the control of Git and verifies that the readdir function reported a failure.

I also have an issue where two low level tests (t0020-crlf and t0022-crlf-rename) fail (and perhaps later tests, too) unless I de-inline ce_path_match and dir_path_match from dir.h but as I cannot yet explain what the issue is or why this is a fix, I'm holding onto this one for now.

Charles.
Charles Bailey· Mar 29, 2014, 15:39 UTC · re: Charles Bailey · lore

[PATCH 1/2] Remove inline from git_fnmatch in dir.c

Now that it calls a static inline function, it cannot be an inline definition with external linkage. Remove inline and make it an external definition.

Signed-off-by: Charles Bailey <cbailey32@bloomberg.net>
---
 dir.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/dir.c b/dir.c
index 99f5303..eb6f581 100644
--- a/dir.c
+++ b/dir.c
@@ -54,9 +54,9 @@ int fnmatch_icase(const char *pattern, const char *string, int flags)
 			 NULL);
 }
 
-inline int git_fnmatch(const struct pathspec_item *item,
-		       const char *pattern, const char *string,
-		       int prefix)
+int git_fnmatch(const struct pathspec_item *item,
+		const char *pattern, const char *string,
+		int prefix)
 {
 	if (prefix > 0) {
 		if (ps_strncmp(item, pattern, string, prefix))

-- 
1.8.5.1.2.ge5d1dab
Charles Bailey· Mar 29, 2014, 15:39 UTC · re: Charles Bailey · lore

[PATCH 2/2] Don't rely on strerror text when testing rmdir failure

AIX doesn't make a distiction between EEXIST and ENOTEMPTY so relying on the strerror string for the rmdir failure is fragile. Just test that the start of the string matches the Git controlled "failed to rmdir..." error. The exact text of the OS generated error string isn't important to the test.

Signed-off-by: Charles Bailey <cbailey32@bloomberg.net>
---
 t/t3600-rm.sh | 5 ++---
 t/t7001-mv.sh | 3 +--
 2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 3d30581..23eed17 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -709,10 +709,9 @@ test_expect_success 'checking out a commit after submodule removal needs manual
 	git commit -m "submodule removal" submod &&
 	git checkout HEAD^ &&
 	git submodule update &&
-	git checkout -q HEAD^ 2>actual &&
+	git checkout -q HEAD^ 2>/dev/null &&
 	git checkout -q master 2>actual &&
-	echo "warning: unable to rmdir submod: Directory not empty" >expected &&
-	test_i18ncmp expected actual &&
+	test_i18ngrep "^warning: unable to rmdir submod:" actual &&
 	git status -s submod >actual &&
 	echo "?? submod/" >expected &&
 	test_cmp expected actual &&
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 215d43d..34fb1af 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -447,8 +447,7 @@ test_expect_success 'checking out a commit before submodule moved needs manual u
 	git mv sub sub2 &&
 	git commit -m "moved sub to sub2" &&
 	git checkout -q HEAD^ 2>actual &&
-	echo "warning: unable to rmdir sub2: Directory not empty" >expected &&
-	test_i18ncmp expected actual &&
+	test_i18ngrep "^warning: unable to rmdir sub2:" actual &&
 	git status -s sub2 >actual &&
 	echo "?? sub2/" >expected &&
 	test_cmp expected actual &&

-- 
1.8.5.1.2.ge5d1dab
Jens Lehmann· Mar 29, 2014, 15:48 UTC · re: Charles Bailey · lore

Re: [PATCH 2/2] Don't rely on strerror text when testing rmdir failure

Am 29.03.2014 16:39, schrieb Charles Bailey:
Show 5 quoted lines
> AIX doesn't make a distiction between EEXIST and ENOTEMPTY so relying on
> the strerror string for the rmdir failure is fragile. Just test that the
> start of the string matches the Git controlled "failed to rmdir..."
> error. The exact text of the OS generated error string isn't important
> to the test.
Makes sense.
Show 16 quoted lines
> Signed-off-by: Charles Bailey <cbailey32@bloomberg.net>
> ---
>  t/t3600-rm.sh | 5 ++---
>  t/t7001-mv.sh | 3 +--
>  2 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index 3d30581..23eed17 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -709,10 +709,9 @@ test_expect_success 'checking out a commit after submodule removal needs manual
>  	git commit -m "submodule removal" submod &&
>  	git checkout HEAD^ &&
>  	git submodule update &&
> -	git checkout -q HEAD^ 2>actual &&
> +	git checkout -q HEAD^ 2>/dev/null &&

Isn't this unrelated to the strerror issue you are fixing here? Why not just drop the redirection completely? But maybe I'm just being to pedantic here ;-)

Show 22 quoted lines
>  	git checkout -q master 2>actual &&
> -	echo "warning: unable to rmdir submod: Directory not empty" >expected &&
> -	test_i18ncmp expected actual &&
> +	test_i18ngrep "^warning: unable to rmdir submod:" actual &&
>  	git status -s submod >actual &&
>  	echo "?? submod/" >expected &&
>  	test_cmp expected actual &&
> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
> index 215d43d..34fb1af 100755
> --- a/t/t7001-mv.sh
> +++ b/t/t7001-mv.sh
> @@ -447,8 +447,7 @@ test_expect_success 'checking out a commit before submodule moved needs manual u
>  	git mv sub sub2 &&
>  	git commit -m "moved sub to sub2" &&
>  	git checkout -q HEAD^ 2>actual &&
> -	echo "warning: unable to rmdir sub2: Directory not empty" >expected &&
> -	test_i18ncmp expected actual &&
> +	test_i18ngrep "^warning: unable to rmdir sub2:" actual &&
>  	git status -s sub2 >actual &&
>  	echo "?? sub2/" >expected &&
>  	test_cmp expected actual &&
>
Charles Bailey· Mar 29, 2014, 15:58 UTC · re: Jens Lehmann · lore

Re: [PATCH 2/2] Don't rely on strerror text when testing rmdir failure

On Sat, Mar 29, 2014 at 04:48:44PM +0100, Jens Lehmann wrote:
Show 15 quoted lines
> Am 29.03.2014 16:39, schrieb Charles Bailey:
> > diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> > index 3d30581..23eed17 100755
> > --- a/t/t3600-rm.sh
> > +++ b/t/t3600-rm.sh
> > @@ -709,10 +709,9 @@ test_expect_success 'checking out a commit after submodule removal needs manual
> >  	git commit -m "submodule removal" submod &&
> >  	git checkout HEAD^ &&
> >  	git submodule update &&
> > -	git checkout -q HEAD^ 2>actual &&
> > +	git checkout -q HEAD^ 2>/dev/null &&
> 
> Isn't this unrelated to the strerror issue you are fixing here?
> Why not just drop the redirection completely? But maybe I'm just
> being to pedantic here ;-)

Well, it's a write to 'actual' and the next thing that tests the contents of 'actual' is the thing that I'm fixing so it's almost related. (See context kept below.)

I changed the redirection here while investigating the bug. The redirected output was being overwritten immediately and this was a more explicit way to write "I don't care about whatever goes to stderr from this command" which confused me less that merely overwriting the file on the next line, but perhaps simply not redirecting is better. I really didn't give it much thought.

Show 5 quoted lines
> 
> >  	git checkout -q master 2>actual &&
> > -	echo "warning: unable to rmdir submod: Directory not empty" >expected &&
> > -	test_i18ncmp expected actual &&
> > +	test_i18ngrep "^warning: unable to rmdir submod:" actual &&
Charles.
Junio C Hamano· Mar 31, 2014, 17:35 UTC · re: Jens Lehmann · lore

Re: [PATCH 2/2] Don't rely on strerror text when testing rmdir failure

Jens Lehmann <Jens.Lehmann@web.de> writes:
Show 29 quoted lines
> Am 29.03.2014 16:39, schrieb Charles Bailey:
>> AIX doesn't make a distiction between EEXIST and ENOTEMPTY so relying on
>> the strerror string for the rmdir failure is fragile. Just test that the
>> start of the string matches the Git controlled "failed to rmdir..."
>> error. The exact text of the OS generated error string isn't important
>> to the test.
>
> Makes sense.
>
>> Signed-off-by: Charles Bailey <cbailey32@bloomberg.net>
>> ---
>>  t/t3600-rm.sh | 5 ++---
>>  t/t7001-mv.sh | 3 +--
>>  2 files changed, 3 insertions(+), 5 deletions(-)
>> 
>> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
>> index 3d30581..23eed17 100755
>> --- a/t/t3600-rm.sh
>> +++ b/t/t3600-rm.sh
>> @@ -709,10 +709,9 @@ test_expect_success 'checking out a commit after submodule removal needs manual
>>  	git commit -m "submodule removal" submod &&
>>  	git checkout HEAD^ &&
>>  	git submodule update &&
>> -	git checkout -q HEAD^ 2>actual &&
>> +	git checkout -q HEAD^ 2>/dev/null &&
>
> Isn't this unrelated to the strerror issue you are fixing here?
> Why not just drop the redirection completely? But maybe I'm just
> being to pedantic here ;-)

No, that sounds like a very reasonable suggestion. Especially given that the redirection destination is overwritten immediately after.

In general tests should not have to squelch their standard error output with 2>/dev/null; that is a job for the test harness, and they will be shown in the output of "./t3600-rm -v" to serve as anchor point while finding where a test goes wrong, which is a good thing.

← back to recent threads