Using VC build git

pull from git://repo.or.cz/tgit.git

Hi,

On Mon, 10 Aug 2009, Frank Li wrote:

pull from git://repo.or.cz/tgit.git

Why the heck is this not a fork of git.git?

And did you not forget to mention which branch?

And is "git commit okay" really a sensible _full_ commit message?

Please, we have _high_ standards in git.git, and I really do not want to 
have to take anything to Junio that does not fulfill that standard.

Ciao,
Dscho

Hi,

On Mon, 10 Aug 2009, Johannes Schindelin wrote:

Please, we have _high_ standards in git.git, and I really do not want to 
have to take anything to Junio that does not fulfill that standard.

To elaborate:  if I see something like this in the --stat:

 contrib/vcbuild/include/zlib.h        | 1357 +++++++++++++++++++++++++++++++++
 contrib/vcbuild/lib/zlib.lib          |  Bin 0 -> 104148 bytes

... I know already that there is no way this can make it into git.git.  
There just is not.

Also, if the first commit says nothing else than "Rebase to v1.6.4", it 
is pretty obvious to me that I will not sign off on that (and I just guess 
that is the very reason you did not sign off on that, either).

Further, putting anything into contrib/ that really belongs into contrib/ 
is not cutting it, either.

And I am pretty astonished that mingw.[ch] is touched, as VC is definitely 
not MinGW32.

Changing 1000+ lines of libgit.vcproj in almost every commit is also 
something I really do not look upon favorably.

Finally, if _no single_ commit message says _anything_ about the reasons 
why you had to change code outside of vcbuild/, I am only puzzled.

Now, I want to give you a pretty clear idea what has to be done if this is 
going into 4msysgit.git, ever, because you obviously spent a lot of time 
on it, and other people want it, too:

- changing "open" to "_open" in mingw.c is a no-no-no.  If you need to use 
  "_open" in VC, then define "open" in the compile flags for mingw.c, but 
  leave code that is not written for VC alone.

- introducing trailing whitespace is usually a sign of not caring enough 
  about clean and neat code.  So just don't do it.

- making link() fail on MinGW32 just to be able to compile it with VC is 
  outright rude against all people who use a free and open compiler 
  instead of a closed one.

- changing an "_snprintf" to "_vsnprintf" in vcbuild/porting.c without 
  anything else is a clear and loud sign that the code before was broken, 
  and that you fix a faulty patch in a later patch.  This is not how we do 
  things in git.git.  We fix the proper patch before the patch series is 
  accepted into mainline.

- violating the coding style -- even if it is in your VC-specific part -- 
  is not an option.  You need to fix the coding style.

- violating the coding style in files that are not VC-specific is not an 
  option at all.  You really need to fix it.

- changing the default editor from "vi" to "notepad2" will break almost 
  every existing Git user's setup.  That is just inexcusable.

Note: these comments are _just for the last_ of your 5 patches.

Just a brief comment on the 4th patch, because I really do not want to 
spend more time on this round of patches: spelling the opendir() function 
as "open dir" function in the commit message is misleading, to say the 
least, and moving code that was added in a previous patch in the same 
patch series just shows that it was a mistake to begin with.  Besides, 
don't move anything into mingw.c if MinGW32 does not need it.

Hth,
Dscho

Thank you take care my patch.
I can fix all problems.

This patch is base on v1.6.4 release.  My working branch is vc_build
at git://repo.or.cz/tgit.git.

That is actually prototype to approve VC can build git.
The code style is not big problem. I will fix it.

VC build will reuse many msysgit works because msysgit really do many
work at windows porting.

I think the below is most important problem.

1.  Where are vcbuild directory put, is it okay under contrib ?
2.  How to handle external library, such as zlib? Can use submodule?


2009/8/10, Johannes Schindelin [off-list ref]:

Hi,

On Mon, 10 Aug 2009, Johannes Schindelin wrote:

quoted

Please, we have _high_ standards in git.git, and I really do not want to
have to take anything to Junio that does not fulfill that standard.

To elaborate:  if I see something like this in the --stat:

 contrib/vcbuild/include/zlib.h        | 1357
+++++++++++++++++++++++++++++++++
 contrib/vcbuild/lib/zlib.lib          |  Bin 0 -> 104148 bytes

... I know already that there is no way this can make it into git.git.
There just is not.

Also, if the first commit says nothing else than "Rebase to v1.6.4", it
is pretty obvious to me that I will not sign off on that (and I just guess
that is the very reason you did not sign off on that, either).

Further, putting anything into contrib/ that really belongs into contrib/
is not cutting it, either.

And I am pretty astonished that mingw.[ch] is touched, as VC is definitely
not MinGW32.

Changing 1000+ lines of libgit.vcproj in almost every commit is also
something I really do not look upon favorably.

Finally, if _no single_ commit message says _anything_ about the reasons
why you had to change code outside of vcbuild/, I am only puzzled.

Now, I want to give you a pretty clear idea what has to be done if this is
going into 4msysgit.git, ever, because you obviously spent a lot of time
on it, and other people want it, too:

- changing "open" to "_open" in mingw.c is a no-no-no.  If you need to use
  "_open" in VC, then define "open" in the compile flags for mingw.c, but
  leave code that is not written for VC alone.

- introducing trailing whitespace is usually a sign of not caring enough
  about clean and neat code.  So just don't do it.

- making link() fail on MinGW32 just to be able to compile it with VC is
  outright rude against all people who use a free and open compiler
  instead of a closed one.

- changing an "_snprintf" to "_vsnprintf" in vcbuild/porting.c without
  anything else is a clear and loud sign that the code before was broken,
  and that you fix a faulty patch in a later patch.  This is not how we do
  things in git.git.  We fix the proper patch before the patch series is
  accepted into mainline.

- violating the coding style -- even if it is in your VC-specific part --
  is not an option.  You need to fix the coding style.

- violating the coding style in files that are not VC-specific is not an
  option at all.  You really need to fix it.

- changing the default editor from "vi" to "notepad2" will break almost
  every existing Git user's setup.  That is just inexcusable.

Note: these comments are _just for the last_ of your 5 patches.

Just a brief comment on the 4th patch, because I really do not want to
spend more time on this round of patches: spelling the opendir() function
as "open dir" function in the commit message is misleading, to say the
least, and moving code that was added in a previous patch in the same
patch series just shows that it was a mistake to begin with.  Besides,
don't move anything into mingw.c if MinGW32 does not need it.

Hth,
Dscho

[Please do *not* do top-posting! Both git and msysgit mailing lists 
use bottom-posting]

Frank Li said the following on 11.08.2009 03:26:

Thank you take care my patch.
I can fix all problems.

Good! Many people want to see git build with MSVC, if only to use a 
compiler better at optimizing code on Windows. (And the debugger, of 
course)

This patch is base on v1.6.4 release.  My working branch is vc_build
at git://repo.or.cz/tgit.git.

Ok. Dscho wondered why it wasn't a proper fork of the main git.git 
repo, so it *should* really have been
     git://repo.or.cz/git/tgit.git
                      ^^^^ <-- Notice the fork relationship?

It would save some valuable space on the server, show relations, make 
it easier to find, etc.

That is actually prototype to approve VC can build git.
The code style is not big problem. I will fix it.

You will experience that for the git community coding style is very 
important (for good reason), so expect many rounds of rewriting your 
patches, until they all shine like diamonds.

VC build will reuse many msysgit works because msysgit really do many
work at windows porting.

Sure, but I think it rarely will involve your patches changing the 
code in MinGW at all. Try your outmost to keep your patches separated 
from anything else. If in doubt, please ask us on the msysgit mailing 
list, and we will guide you.

I think the below is most important problem.

1.  Where are vcbuild directory put, is it okay under contrib ?
2.  How to handle external library, such as zlib? Can use submodule?

For point 2: If we cannot compile we don't accept it, is the basic 
rule. This means that if you think the people building git with MSVC 
cannot install some dependencies themselves, you need to provide the 
full sources yourself, and make sure that the code compiles without 
changes. So, in this case if would mean to include the sources for 
zlib, and setup the vcproj to compile this code into the executable, 
instead of linking with a precompiled lib.

In this case, however, I think you should rely on the developer 
providing this library themselves, and not add it to the git project. 
The zlib project is ~750KB of code itself..

--
.marius