git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Frank Li <lznuaa@gmail.com>
Cc: git@vger.kernel.org, msysGit <msysgit@googlegroups.com>
Subject: Re: Using VC build git (new patch)
Date: Sat, 15 Aug 2009 19:29:36 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.1.00.0908151908300.8306@pacific.mpi-cbg.de> (raw)
In-Reply-To: <alpine.DEB.1.00.0908151851280.8306@pacific.mpi-cbg.de>

Hi,

On Sat, 15 Aug 2009, Johannes Schindelin wrote:

> A single monster patch?
> 
> Hmm.
> 
> Please understand that I will not review that unless it is split up into 
> nice little and reviewable chunks.

Well, I try to improve.

So I had a look at your patch.  The changes fall naturally into one of the 
following categories:

	- decl-after-statement fixes

	- missing #include "git-compat-util.h"

	- converting a K&R style function definition to modern C

	- #undef's only needed for Microsoft Visual C++

	- the addition of O_BINARY in the file modes

	- disabling link() (why?)

	- double-#define'ing stat (which puzzles me greatly)

	- dummy #define'ing of a few compiler attributes

	- adding _MSVC to a conditional to avoid  #define'ing
	  SNPRINTF_SIZE_CORR yourself

	- #define'ing several symbols without leading underscore to the
	  MS-specific version with a leading underscore

	- implementing strcasecmp() in a misnamed file

	- "fixing" the return value of winansi_vfprintf for Microsoft 
	  Visual C++ (I think this fix is wrong)

	- correctly adding a Visual C++-specific conditional to 
	  git-compat-util.h, pager.c, run-command.c, run-command.h, 
	  setup.c and help.c, although the latter five could use some 
	  refactoring into git-compat-util.h

	  Maybe there is also room to change the MinGW-conditional into a
	  NO_TRUSTABLE_FILEMODE one

	- adding several headers missing from Visual C++'s installation

	- adding _huge_ .vcproj files (can they not be smaller?)

As you can see, there is a pretty natural way to split that huge patch 
into chunks that most people on these lists can review easily, and that 
would actually be a pleasure to look at.

Even better, there are a few fixes (the first three, if you ask me), which 
are not even Windows-specific.

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.

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

Ciao,
Dscho

  reply	other threads:[~2009-08-15 17:29 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-08-15 16:21 Using VC build git (new patch) Frank Li
2009-08-15 16:57 ` Johannes Schindelin
2009-08-15 17:29   ` Johannes Schindelin [this message]
2009-08-16  4:07     ` Frank Li
2009-08-16  8:38       ` Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.1.00.0908151908300.8306@pacific.mpi-cbg.de \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=lznuaa@gmail.com \
    --cc=msysgit@googlegroups.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).