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
next prev parent 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).