Johannes Sixt [off-list ref] 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.
Hi,
On Wed, 14 Nov 2007, Junio C Hamano wrote:
Johannes Sixt [off-list ref] writes:
quoted
This is a series of smallish, unrelated changes that were necessary
for the MinGW port.
I was _VERY_ afraid of reviewing this series.
Why? Because we get closer to MinGW integration into git.git for real?
;-)
quoted
[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.
The safe thing, of course, would be to move them at the end of the include
list in git-compat-util.h, since they are now included after cache.h,
(which includes git-compat-util.h and only strbuf.h, the sha1 header and
zlib.h).
This way it should be really certainly undisputed.
quoted
[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.
Note that we are not strictly _moving_ it around. In fact, we are _adding_
more close() calls... And even ignoring the errors when close() was
already called, so it feels a tad hacky. But it does the job.
quoted
[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.
Just to be safe in the future, we could check for that condition (by
introducing a static variable setup_path_called) and die() should anybody
introduce a code path where the order of calls is not maintained.
But other than that and 08/11, I found everything is trivially correct
and it was a pleasant read.
Me, too.
Ciao,
Dscho