Re: [patch 00/16] Portability Patches for git-1.7.1 (v4)

From: Junio C Hamano <gitster@pobox.com>
Date: May 1, 2010, 23:25 UTC
Message-ID: <7vk4rn2oiv.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4BDC744E.4050006@drmicha.warpmail.net>

Michael J Gruber <git@drmicha.warpmail.net> writes:

Show 36 quoted lines

> Michael J Gruber venit, vidit, dixit 27.04.2010 17:30:
>> Gary V. Vaughan venit, vidit, dixit 27.04.2010 15:57:
>>> Here are the portability patches we needed at TWW to enable git-1.7.1
>>> to compile and run on all of the wide range of Unix machines we
>>> support.  These patches apply to the git-1.7.1 release,  and address
>>> all of the feedback from the previous three times I posted them to
>>> this list, including fixing the massive testsuite failures I was
>>> experiencing and taking into account that the ss_family fixes and
>>> partial GMT_CMP_TEST fixes that have been pushed since my last post of
>>> this patch queue.
>> 
>> General remark: None of your patches have a s-o-b line. If you want to
>> have your patches in git you are required to sign-off on them (commit
>> -s) in order to certify that you can submit them under the license terms
>> of the project.
>> 
>> Your diff -> test_cmp are certainly something we want to have in any
>> case. The code changes look ugly, honestly, making the code much less
>> readable, but it seems to be the only way to make those older platforms
>> and compilers happy. (Erik pointed out some good ways to reduce the
>> uglyness somewhat.)
>> 
>> I can't test your target platform, but I would test the impact of the
>> code and test changes on mine. Do you have your series somewhere to pull
>> from?
>
> OK, unsurprsingly, tests still pass on Linux (Fedora 12 x86_64).
> If anyone wants to try, the series can be found at
>
> git://repo.or.cz/git/mjg.git
>
> in branch
>
> gvv/platform-compatibility
>
> applied cleanly on current master.

Thanks.

Like everybody else, I like the s/diff/test_cmp/ one in general. On platforms without "diff -u", test_cmp may want to use "diff -c" instead for readability, but that can be fixed/enhanced independently.

I agree with your general remark and also noticed that the titles are way suboptimal.

* user-cppflags.patch

  Makefile: pass CPPFLAGS through to fllow customization

* const-expr.patch

  Rewrite dynamic structure initializations to runtime assignment

* pthread.patch

  Makefile: -lpthread may still be necessary when libc has only pthread stubs

* Without this patch at least IBM VisualAge C 5.0 (I have 5.0.2) on AIX 5.1 fails to compile git.

  enums: omit trailing comma for portability

* diff-export.patch

  Do not use "diff" found on PATH while building and installing

I think the change to git-merge-one-file.sh in this patch is wrong, by the way.

* diff-test_cmp.patch

  tests: use "test_cmp", not "diff", when verifying the result

The patch to t9400 has indent-with-spaces, by the way.

* diff-defaults.patch

  test_cmp: do not use "diff -u" on platforms that lack one

It may be better to use "diff -c" on most of them, though.

* host-SunOS56.patch

  Makefile: SunOS 5.6 portability fix

* host-IRIX.patch

  git-compat-util.h: Irix 6.5 defines 'sgi' but not '__sgi'.

* host-HPUX10.patch

This would be better as two patches

  Makefile: HP-UX 10.20 lacks pread()
  git-compat-util.h: some platforms with mmap() lack MAP_FAILED definition

* host-HPUX11.patch

  Makefile: HPUX does not have nanosecond timestamp in struct stat

* host-OSF1.patch

  Makefile: Tru64 portability fix

* no-hstrerror.patch

I think this should come before "Makefile: SunOS 5.6 portability fix" (split the change to Makefile from this one and move it to the other patch). Then this patch does not talk about SunOS specific issues.

  Makefile: some platforms do not have hstrerror anywhere

* no-inet_ntop.patch

It might make sense to squash this patch into the previous one (and again do this before HPUX patches to Makefile) that deals with three functions that are traditionally related to libresolv (hstrerror, inet_ntop/pton).

  Make NO_{HSTRERROR,INET_NTOP,INET_PTON} configured independently

* no-socklen_t.patch

Do this before the platform dependent bits, i.e. move the hunks that changes "ifeq ($(uname_S,XXX)" block from this patch to host-XXX patch, and do the remainder of this patch before any of the platform ones.

  Some platforms lack socklen_t type

* no-inline.patch

Do this before ... (ditto) ...

  Allow disabling "inline"