git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Johannes Sixt <johannes.sixt@telecom.at>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 0/11] Miscellaneous MinGW port fallout
Date: Wed, 14 Nov 2007 03:02:24 -0800	[thread overview]
Message-ID: <7vir45hyyn.fsf@gitster.siamese.dyndns.org> (raw)
In-Reply-To: <1194984306-3181-1-git-send-email-johannes.sixt@telecom.at> (Johannes Sixt's message of "Tue, 13 Nov 2007 21:04:55 +0100")

Johannes Sixt <johannes.sixt@telecom.at> writes:

> This is a series of smallish, unrelated changes that were necessary
> for the MinGW port.

I was _VERY_ afraid of reviewing this series.

> [PATCH 05/11] Use is_absolute_path() in sha1_file.c.
> [PATCH 06/11] Move #include <sys/select.h> and <sys/ioctl.h> to
> 	git-compat-util.h.
>
> These two are certainly undisputed.

Except on esoteric/broken systems there might be some dependency
on the order the system include files are included, so 06/11
needs some testing.  But it is a change in the right direction.

> [PATCH 07/11] builtin run_command: do not exit with -1.
>
> Replaces exit(-1) by exit(255). I don't know if this has any bad
> consequences on *nix.

Linux manual page says "the value of status & 0377 is returned
to the parent", which agrees with POSIX's "only the least
significant 8 bits(that is, status & 0377) shall be available to
a waiting parent process".  So I think we are safe on conforming
platforms.

> [PATCH 08/11] Close files opened by lock_file() before unlinking.
>
> This one was authored by Dscho. It is a definite MUST on Windows.

This was something we've talked about doing a few times on the
list but did not.  It is good that this saw some testing in the
field, as it is easy to get wrong while moving the call site of
close(2) around.

> [PATCH 09/11] Allow a relative builtin template directory.
> [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access
> 	of ETC_GITCONFIG.
> [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.
>
> These need probably some discussion. They avoid that $(prefix) is
> hardcoded and so allows that an arbitrary installation directory.

I had to worry a bit about bootstrapping issues in 11/11.  We
need to ensure that anybody who wants to read the configuration
data first does setup_path() because git_exec_path() reads from
argv_exec_path and setup_path() is what assigns to that
variable.

But other than that and 08/11, I found everything is trivially
correct and it was a pleasant read.

Thanks.

  parent reply	other threads:[~2007-11-14 11:02 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-11-13 20:04 [PATCH 0/11] Miscellaneous MinGW port fallout Johannes Sixt
2007-11-13 20:04 ` [PATCH 01/11] t5300-pack-object.sh: Split the big verify-pack test into smaller parts Johannes Sixt
2007-11-13 20:04   ` [PATCH 02/11] t7501-commit.sh: Not all seds understand option -i Johannes Sixt
2007-11-13 20:04     ` [PATCH 03/11] t5302-pack-index: Skip tests of 64-bit offsets if necessary Johannes Sixt
2007-11-13 20:04       ` [PATCH 04/11] Skip t3902-quoted.sh if the file system does not support funny names Johannes Sixt
2007-11-13 20:05         ` [PATCH 05/11] Use is_absolute_path() in sha1_file.c Johannes Sixt
2007-11-13 20:05           ` [PATCH 06/11] Move #include <sys/select.h> and <sys/ioctl.h> to git-compat-util.h Johannes Sixt
2007-11-13 20:05             ` [PATCH 07/11] builtin run_command: do not exit with -1 Johannes Sixt
2007-11-13 20:05               ` [PATCH 08/11] Close files opened by lock_file() before unlinking Johannes Sixt
2007-11-13 20:05                 ` [PATCH 09/11] Allow a relative builtin template directory Johannes Sixt
2007-11-13 20:05                   ` [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access of ETC_GITCONFIG Johannes Sixt
2007-11-13 20:05                     ` [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path Johannes Sixt
2007-11-15  6:53                       ` Steffen Prohaska
2007-11-15 18:31                         ` Johannes Sixt
2007-11-15 20:10                           ` Steffen Prohaska
2007-11-15 21:43                             ` Johannes Sixt
2007-11-15 22:24                               ` Steffen Prohaska
2007-11-13 21:22                     ` [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access of ETC_GITCONFIG Johannes Schindelin
2007-11-13 21:35                       ` Johannes Sixt
2007-11-13 21:43                         ` Johannes Schindelin
2007-11-13 21:05                 ` [PATCH 08/11] Close files opened by lock_file() before unlinking Johannes Schindelin
2007-11-13 21:39           ` [PATCH 05/11] Use is_absolute_path() in sha1_file.c Johannes Schindelin
2007-11-13 21:43             ` Johannes Sixt
2007-11-13 21:48               ` Johannes Schindelin
2007-11-14 21:57               ` Robin Rosenberg
2007-11-13 20:10 ` [PATCH 0/11] Miscellaneous MinGW port fallout Johannes Sixt
2007-11-13 21:10   ` Junio C Hamano
2007-11-13 21:32     ` Johannes Sixt
2007-11-13 21:46       ` Johannes Schindelin
2007-11-13 21:54         ` Johannes Sixt
2007-11-13 22:22         ` Junio C Hamano
2007-11-14 11:02 ` Junio C Hamano [this message]
2007-11-14 14:50   ` Johannes Schindelin
2007-11-14 20:13     ` Junio C Hamano
2007-11-14 20:45       ` 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

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

  git send-email \
    --in-reply-to=7vir45hyyn.fsf@gitster.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=johannes.sixt@telecom.at \
    /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.
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 NNTP newsgroup(s).