Re: Using VC build git (new patch)
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Aug 16, 2009, 08:38 UTC
- Message-ID
- <alpine.DEB.1.00.0908161018370.8306@pacific.mpi-cbg.de>
- In-Reply-To
- <1976ea660908152107s8b8f3e5l3c2f043fb3e4d62@mail.gmail.com>
Hi,
[please quote only those parts that you actually reply to.]
On Sun, 16 Aug 2009, Frank Li wrote:
Show 6 quoted lines
> 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?
Show 5 quoted lines
> > - 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.
Show 5 quoted lines
> 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.
Show 9 quoted lines
> > 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