[PULL] Pull request from msysGit

[PULL] Pull request from msysGit

[Pat Thoyts]

This follows up on the comments made for the previous pull request for
patches from msysGit. This batch is restricted to those that passed
review or have been modified as a result of the earlier review.
I've added some Acked-by's resulting from comments made.

The following changes since commit 1e633418479926bc85ed21a4f91c845a3dd3ad66:

  Merge branch 'maint' (2010-09-30 14:59:53 -0700)

are available in the git repository at:

  git://repo.or.cz/git/mingw/4msysgit.git work/pt/for-junio
or alternatively
  http://repo.or.cz/w/git/mingw/4msysgit.git/shortlog/refs/heads/work/pt/for-junio

5debf9a Add MinGW-specific execv() override.
77df1f1 Fix Windows-specific macro redefinition warning.
b248e95 Fix 'clone' failure at DOS root directory.
1a40420 mingw: do not crash on open(NULL, ...)
5e9677c git-am: fix detection of absolute paths for windows
36e035f Side-step MSYS-specific path "corruption" leading to t5560 failure.
ca02ad3 Side-step sed line-ending "corruption" leading to t6038 failure.
97f2c33 Skip 'git archive --remote' test on msysGit
a94114a Do not strip CR when grepping HTTP headers.
3ba9ba8 Skip t1300.70 and 71 on msysGit.
4e57baf merge-octopus: Work around environment issue on Windows
442dada MinGW: Report errors when failing to launch the html browser.
9b9784c MinGW: fix stat() and lstat() implementations for handling symlinks
4091bfc MinGW: Add missing file mode bit defines
e7cf4e9 MinGW: Use pid_t more consequently, introduce uid_t for greater compatibility

 abspath.c                        |    6 +++-
 compat/mingw.c                   |   56 ++++++++++++++++++++++++++++++++-----
 compat/mingw.h                   |   36 +++++++++++++++++++-----
 git-am.sh                        |   12 ++++----
 git-merge-octopus.sh             |    5 +++
 git-sh-setup.sh                  |   15 ++++++++++
 t/t1300-repo-config.sh           |    6 ++--
 t/t5000-tar-tree.sh              |    2 +-
 t/t5503-tagfollow.sh             |    9 +-----
 t/t5560-http-backend-noserver.sh |    5 ++-
 t/t6038-merge-text-auto.sh       |    4 ++-
 t/test-lib.sh                    |    2 +
 12 files changed, 122 insertions(+), 36 deletions(-)

Re: [PULL] Pull request from msysGit

[Junio C Hamano]

Pat Thoyts <patthoyts@users.sourceforge.net> writes:

> This follows up on the comments made for the previous pull request for
> patches from msysGit. This batch is restricted to those that passed
> review or have been modified as a result of the earlier review.
> I've added some Acked-by's resulting from comments made.
>
> The following changes since commit 1e633418479926bc85ed21a4f91c845a3dd3ad66:
>
>   Merge branch 'maint' (2010-09-30 14:59:53 -0700)
>
> are available in the git repository at:
>
>   git://repo.or.cz/git/mingw/4msysgit.git work/pt/for-junio
> or alternatively
>   http://repo.or.cz/w/git/mingw/4msysgit.git/shortlog/refs/heads/work/pt/for-junio

Thanks.

> 5debf9a Add MinGW-specific execv() override.
> 77df1f1 Fix Windows-specific macro redefinition warning.
> b248e95 Fix 'clone' failure at DOS root directory.
> 1a40420 mingw: do not crash on open(NULL, ...)
> 5e9677c git-am: fix detection of absolute paths for windows
> 36e035f Side-step MSYS-specific path "corruption" leading to t5560 failure.
> ca02ad3 Side-step sed line-ending "corruption" leading to t6038 failure.
> 97f2c33 Skip 'git archive --remote' test on msysGit
> a94114a Do not strip CR when grepping HTTP headers.
> 3ba9ba8 Skip t1300.70 and 71 on msysGit.
> 4e57baf merge-octopus: Work around environment issue on Windows
> 442dada MinGW: Report errors when failing to launch the html browser.
> 9b9784c MinGW: fix stat() and lstat() implementations for handling symlinks
> 4091bfc MinGW: Add missing file mode bit defines
> e7cf4e9 MinGW: Use pid_t more consequently, introduce uid_t for greater compatibility
>
>  abspath.c                        |    6 +++-
>  compat/mingw.c                   |   56 ++++++++++++++++++++++++++++++++-----
>  compat/mingw.h                   |   36 +++++++++++++++++++-----
>  git-am.sh                        |   12 ++++----
>  git-merge-octopus.sh             |    5 +++
>  git-sh-setup.sh                  |   15 ++++++++++
>  t/t1300-repo-config.sh           |    6 ++--
>  t/t5000-tar-tree.sh              |    2 +-
>  t/t5503-tagfollow.sh             |    9 +-----
>  t/t5560-http-backend-noserver.sh |    5 ++-
>  t/t6038-merge-text-auto.sh       |    4 ++-
>  t/test-lib.sh                    |    2 +
>  12 files changed, 122 insertions(+), 36 deletions(-)

Re: [PULL] Pull request from msysGit

[Ramsay Jones]

Pat Thoyts wrote:
> The following changes since commit 1e633418479926bc85ed21a4f91c845a3dd3ad66:
> 
>   Merge branch 'maint' (2010-09-30 14:59:53 -0700)
> 
> are available in the git repository at:
> 
>   git://repo.or.cz/git/mingw/4msysgit.git work/pt/for-junio
> or alternatively
>   http://repo.or.cz/w/git/mingw/4msysgit.git/shortlog/refs/heads/work/pt/for-junio
> 
> 5debf9a Add MinGW-specific execv() override.
> 77df1f1 Fix Windows-specific macro redefinition warning.
> b248e95 Fix 'clone' failure at DOS root directory.
> 1a40420 mingw: do not crash on open(NULL, ...)
> 5e9677c git-am: fix detection of absolute paths for windows
> 36e035f Side-step MSYS-specific path "corruption" leading to t5560 failure.
> ca02ad3 Side-step sed line-ending "corruption" leading to t6038 failure.
> 97f2c33 Skip 'git archive --remote' test on msysGit
> a94114a Do not strip CR when grepping HTTP headers.
> 3ba9ba8 Skip t1300.70 and 71 on msysGit.
> 4e57baf merge-octopus: Work around environment issue on Windows
> 442dada MinGW: Report errors when failing to launch the html browser.
> 9b9784c MinGW: fix stat() and lstat() implementations for handling symlinks
> 4091bfc MinGW: Add missing file mode bit defines

This commit (4091bfc) may well introduce logic errors into the msvc
build; I haven't checked (it depends on what the msvc compiler does
when a macro is redefined - does the new definition replace the old?).
However, no matter what else may be wrong, this commit introduces a
shed load of new warnings, thus:

    $ make clean
    $ make MSVC=1 >out 2>&1
    $ grep warning out | grep S_I | wc -l
    1000
    $ 

so 1000 additional warnings which, looking at the start of the out
file, look like this:

GIT_VERSION = 1.7.3.dirty
    * new build flags or prefix
    CC fast-import.o
fast-import.c
c:\cygwin\home\ramsay\git\compat/mingw.h(16) : warning C4005: 'S_IRUSR' : macro redefinition
        compat/vcbuild/include\unistd.h(85) : see previous definition of 'S_IRUSR'
c:\cygwin\home\ramsay\git\compat/mingw.h(17) : warning C4005: 'S_IWUSR' : macro redefinition
        compat/vcbuild/include\unistd.h(84) : see previous definition of 'S_IWUSR'
c:\cygwin\home\ramsay\git\compat/mingw.h(18) : warning C4005: 'S_IXUSR' : macro redefinition
        compat/vcbuild/include\unistd.h(83) : see previous definition of 'S_IXUSR'
c:\cygwin\home\ramsay\git\compat/mingw.h(19) : warning C4005: 'S_IRWXU' : macro redefinition
        compat/vcbuild/include\unistd.h(82) : see previous definition of 'S_IRWXU'

Now, Peter Harris has already submitted a fix for this, which is
currently on the work/msvc-fixes branch, which contains:

    358f1be Modify MSVC wrapper script
    38bd27d Fix MSVC build

The suggested fix is given in commit 38bd27d. However, I prefer a
different solution, which is given below:

--- >8 ---
diff --git a/compat/mingw.h b/compat/mingw.h
index afedf3a..445d1a1 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -12,12 +12,6 @@ typedef int pid_t;
 #define S_ISLNK(x) (((x) & S_IFMT) == S_IFLNK)
 #define S_ISSOCK(x) 0
 
-#ifndef _STAT_H_
-#define S_IRUSR 0
-#define S_IWUSR 0
-#define S_IXUSR 0
-#define S_IRWXU (S_IRUSR | S_IWUSR | S_IXUSR)
-#endif
 #define S_IRGRP 0
 #define S_IWGRP 0
 #define S_IXGRP 0
--- 8< ---

Note that, for *both* MinGW and MSVC, the deleted #defines
are not wanted, pointless and just plain wrong! :-D

If you squash the above into 4091bfc then we find:

    $ make clean
    $ make MSVC=1 >out1 2>&1
    $ grep warning out1 | grep S_I | wc -l
    0
    $ 

and there is also no chance of introducing a logic error.

Although I'm not recommending you use one of the commits on
the work/msvc-fixes branch, can I request, once again, that
you include:

    358f1be Modify MSVC wrapper script

If it makes any difference, you can add:

    Acked-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>

ATB,
Ramsay Jones

Re: [PULL] Pull request from msysGit

[Peter Harris]

On 2010-10-07 13:17, Ramsay Jones wrote:
> Now, Peter Harris has already submitted a fix for this, which is
> currently on the work/msvc-fixes branch, which contains:
> 
>     358f1be Modify MSVC wrapper script
>     38bd27d Fix MSVC build
> 
> The suggested fix is given in commit 38bd27d. However, I prefer a
> different solution, which is given below:
> 
> --- >8 ---
> diff --git a/compat/mingw.h b/compat/mingw.h
> index afedf3a..445d1a1 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -12,12 +12,6 @@ typedef int pid_t;
>  #define S_ISLNK(x) (((x) & S_IFMT) == S_IFLNK)
>  #define S_ISSOCK(x) 0
>  
> -#ifndef _STAT_H_
> -#define S_IRUSR 0
> -#define S_IWUSR 0
> -#define S_IXUSR 0
> -#define S_IRWXU (S_IRUSR | S_IWUSR | S_IXUSR)
> -#endif
>  #define S_IRGRP 0
>  #define S_IWGRP 0
>  #define S_IXGRP 0
> --- 8< ---
> 
> Note that, for *both* MinGW and MSVC, the deleted #defines
> are not wanted, pointless and just plain wrong! :-D

I didn't realize that the defines were not wanted for MinGW either.

I heartily approve of removing code rather than just ifdefing around it.
Please use this version of the patch instead of mine.

Peter Harris
-- 
               Open Text Connectivity Solutions Group
Peter Harris                    http://connectivity.opentext.com/
Research and Development        Phone: +1 905 762 6001
pharris@opentext.com            Toll Free: 1 877 359 4866

Re: [msysGit] Re: [PULL] Pull request from msysGit

[Pat Thoyts]

On 7 October 2010 20:30, Peter Harris <pharris@opentext.com> wrote:
> On 2010-10-07 13:17, Ramsay Jones wrote:
>> Now, Peter Harris has already submitted a fix for this, which is
>> currently on the work/msvc-fixes branch, which contains:
>>
>>     358f1be Modify MSVC wrapper script
>>     38bd27d Fix MSVC build
>>
>> The suggested fix is given in commit 38bd27d. However, I prefer a
>> different solution, which is given below:
>>
>> --- >8 ---
>> diff --git a/compat/mingw.h b/compat/mingw.h
>> index afedf3a..445d1a1 100644
>> --- a/compat/mingw.h
>> +++ b/compat/mingw.h
>> @@ -12,12 +12,6 @@ typedef int pid_t;
>>  #define S_ISLNK(x) (((x) & S_IFMT) == S_IFLNK)
>>  #define S_ISSOCK(x) 0
>>
>> -#ifndef _STAT_H_
>> -#define S_IRUSR 0
>> -#define S_IWUSR 0
>> -#define S_IXUSR 0
>> -#define S_IRWXU (S_IRUSR | S_IWUSR | S_IXUSR)
>> -#endif
>>  #define S_IRGRP 0
>>  #define S_IWGRP 0
>>  #define S_IXGRP 0
>> --- 8< ---
>>
>> Note that, for *both* MinGW and MSVC, the deleted #defines
>> are not wanted, pointless and just plain wrong! :-D
>
> I didn't realize that the defines were not wanted for MinGW either.
>
> I heartily approve of removing code rather than just ifdefing around it.
> Please use this version of the patch instead of mine.
>
> Peter Harris

The patch in question has been on the msysGit tree for about 10 months
now. Its somewhat disappointing not to have had it spotted before we
pushed it upstream. Are the msvc builders only working against junio's
repository?

Reverting it seems to make no difference to the msysGit build at all -
presumably because S_IRUSR and friends are all defined in the mingw
<sys/stat.h> anyway. Sebastian - can you recall why this got added?
The commit comment is not all that enlightening.

I also wonder why changes to a compat/mingw.h file should affect the
msvc build. As it has it's own compat/vcbuild and headers in there,
surely it should be independent of mingw-gcc compatability headers?

Pat Thoyts

Re: [msysGit] Re: [PULL] Pull request from msysGit

[Erik Faye-Lund]

On Thu, Oct 7, 2010 at 10:18 PM, Pat Thoyts <patthoyts@gmail.com> wrote:
>
> I also wonder why changes to a compat/mingw.h file should affect the
> msvc build. As it has it's own compat/vcbuild and headers in there,
> surely it should be independent of mingw-gcc compatability headers?
>

compat/msvc.h includes compat/mingw.h. We really should move more
stuff to compat/win32.h, and have cygwin include it's own header or
something instead.

Re: [msysGit] Re: [PULL] Pull request from msysGit

[Ramsay Jones]

Pat Thoyts wrote:
> The patch in question has been on the msysGit tree for about 10 months
> now. Its somewhat disappointing not to have had it spotted before we
> pushed it upstream. Are the msvc builders only working against junio's
> repository?

Well, I can't speak for anyone else, but for me the answer is yes. ;-)
I build git (from the git.kernel.org repository) on Linux, cygwin,
cygwin+msvc and msysGit. (For a short while I also built it on FreeBSD
hosted in a VM - but that was so slow, I soon stopped doing that!).

So, the git I *use* on MinGW/msysGit is built from junio's repository.
I only installed msysGit to enable me to check that my patches worked
on MinGW (I was tired of always saying "could somebody test this on
MinGW ..."). As you may have noticed, the mingw and msvc builds share
quite a bit of compatibility code...

[I don't follow 4msysgit or subscribe to the msysgit mailing list, so
I would not normally see any 4msysgit patches, unless they were also
discussed on the git mailing list.]

ATB,
Ramsay Jones