Re: Using VC build git (new patch)

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4438 bytes --]

Hi,

[please quote only those parts that you actually reply to.]

On Sun, 16 Aug 2009, Frank Li wrote:

> 2009/8/16 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>
> >        - the addition of O_BINARY in the file modes
>
> Yes, default is text mode if no O_BINARY, _read _write will do union 
> code and cr\cf convert.

As Git _never_ wants to open files in text mode, this is a change that can 
come _separately_ from your VC patches.  It could even be considered a bug 
fix (MinGW's current GCC implies O_BINARY, but previous ones may not, and 
it is wrong of us to rely on this magic).

> >        - disabling link() (why?)
>
> VC report stack error. I also don't know why. I have not deep debug this 
> problem.

You should.  What with Visual Studio's superior debugging capabilities, it 
should not be a problem.  Right?

> >        - double-#define'ing stat (which puzzles me greatly)
>
> old define is
>           #define stat    stati64
>           #define stati64 msys_stati64.

Not exactly.

	#define stat _stati64
	[...]
	#define _stati64(x,y) mingw_lstat(x,y)

And even VC should handle that just fine.

> stat is used for both struct and function name.
> In C code:
>       struct stat a;
> stat -> stati64 -> msys_stati64,  so compiler report struct
> msys_stati64 have not defined.

It is a real pity that you did not inline your patch, and that I could not 
quote it as a consequence, because now we could discuss code properly:

The original code was done on purpose as it is, to use the _stati64 struct 
(which allows long pointers, as Microsoft decided off_t was not good 
enough to represent 64 bit offsets).  I could imagine that your patch 
breaks that for Visual C++.

> >        - adding _huge_ .vcproj files (can they not be smaller?)
>
> It is created by VS2008.  I think it is difficult to make smaller.

Is there not some magic to make a compact export version?

But maybe it is not that bad?  I have 72 (out of 1730) files in my 
repository that are larger, gitk being the king.

FWIW I would have loved it if _you_ defended the size of the file, instead 
of having to do it myself.

> > Further, I would like to suggest adding a header file compat/msvc.h 
> > which contains all the #undef's and #define's necessary only for 
> > Visual C++, and which can be #include'd from git-compat-util.h, to 
> > better separate your work from the other platforms (who do not want 
> > those changes).  That should avoid those unwanted changes to mingw.c 
> > and mingw.h.  You just have to make sure that msvc.h is #include'd 
> > before mingw.h.
> 
> VC build will reuse msysgit work.

That is no question.  We made it easy for you.

> I will reduce change to mingw.c and mingw.h
> But there are some problem at mingw.c and mingw.h
> 
> 1. stat defination.  Because both struct and function use the same name stat.

That needs to be resolved somehow, and you would get _much_ more help if 
you had split your patch series properly and this was _one_ patch.

> 2. open need binary mode.

As I said, this can be a separate change.  In my original reply I 
_implied_ that it is okay, and in this reply I elaborated on that (namely 
that it might be considered a bug fix).

> 3. mingw.c use C99 style to break build at VC.

The same applies here as to O_BINARY.

> 4. mingw.c use special DIR structure, So I need add open_dir in mingw.c.

No, you do not need to add that to mingw.c.  MinGW does not need it.  So 
it has no place in mingw.c.

> If I don't change mingw.c, I need copy it to new file totally.

NOOOO!!!

You #include it after making sure that you have #undef'ed and #define'd 
whatever needs to, and in a separate file -- which is only referenced in 
the .vcproj -- you can implement whatever Visual C++ misses.

> > With these comments, I look forward to your next iteration.
> >
> > Ciao,
> > Dscho

Please, please, PLEASE, do not quote things that you are not addressing.  
And please, please, PLEASE adher to Documentation/SubmittingPatches.

Actually, the only thing SubmittingPatches does is spell out how to make 
things easier for reviewers, should you not know how to do so.

Remember: the more time it takes me to cull the quoted parts of your mail, 
the more time it takes me to have a look at your patches, the more time it 
takes me to refer to your patch and make suggestions, the more am I 
inclined to spend my time elsewhere, doing something that is much more 
enjoyable.

Ciao,
Dscho