[PATCH 1/2] Introduces xmkstemp()

[PATCH 1/2] Introduces xmkstemp()

[Luiz Fernando N. Capitulino]

This is a wrapper for mkstemp() that performs error checking and
calls die() when an error occur.

Signed-off-by: Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>
---
 git-compat-util.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 362e040..ca0a597 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -303,6 +303,16 @@ static inline FILE *xfdopen(int fd, const char *mode)
 	return stream;
 }
 
+static inline int xmkstemp(char *template)
+{
+	int fd;
+
+	fd = mkstemp(template);
+	if (fd < 0)
+		die("Unable to create temporary file: %s", strerror(errno));
+	return fd;
+}
+
 static inline size_t xsize_t(off_t len)
 {
 	return (size_t)len;
-- 
1.5.3.GIT



-- 
Luiz Fernando N. Capitulino

Re: [PATCH 1/2] Introduces xmkstemp()

[Carlos Rica]

Hi Luiz:

2007/8/14, Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>:
>
> This is a wrapper for mkstemp() that performs error checking and
> calls die() when an error occur.

I think that it is related with the git_mkstemp() function in path.c.
Perhaps we should use xmkstemp inside that function too, since
it is only used in diff.c and after the call it also dies when fd<0.

--Carlos

Re: [PATCH 1/2] Introduces xmkstemp()

[Luiz Fernando N. Capitulino]

 Hi Carlos,

Em Wed, 15 Aug 2007 17:11:02 +0200
"Carlos Rica" <jasampler@gmail.com> escreveu:

| 2007/8/14, Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>:
| >
| > This is a wrapper for mkstemp() that performs error checking and
| > calls die() when an error occur.
| 
| I think that it is related with the git_mkstemp() function in path.c.

 Yeah, you right.

| Perhaps we should use xmkstemp inside that function too, since
| it is only used in diff.c and after the call it also dies when fd<0.

 Actually, git_mkstemp() is also called by your builtin-verify-tag.c
program and the function which calls it (run_gpg_verify()) doesn't
die() on error.

 This series is not supposed to change programs/design behaivor,
hence I've chosen to not port the calls which doesn't explicit die
on error.

 But feel free to go ahead and port the others. ;)

-- 
Luiz Fernando N. Capitulino

Re: [PATCH 1/2] Introduces xmkstemp()

[Carlos Rica]

2007/8/15, Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>:
>
>  Hi Carlos,
>
> Em Wed, 15 Aug 2007 17:11:02 +0200
> "Carlos Rica" <jasampler@gmail.com> escreveu:
>
> | 2007/8/14, Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>:
> | >
> | > This is a wrapper for mkstemp() that performs error checking and
> | > calls die() when an error occur.
> |
> | I think that it is related with the git_mkstemp() function in path.c.
>
>  Yeah, you right.
>
> | Perhaps we should use xmkstemp inside that function too, since
> | it is only used in diff.c and after the call it also dies when fd<0.
>
>  Actually, git_mkstemp() is also called by your builtin-verify-tag.c
> program and the function which calls it (run_gpg_verify()) doesn't
> die() on error.

Oh, yeah, I had an old version here. I even don't know if there is need
for verifying more than one tag in one command. When more than
one tag is verified, the return value says nothing about what tag or
tags failed, it is just the same as it would be if the program exited
on the first not verifiable tag, so perhaps it could do that.

Re: [PATCH 1/2] Introduces xmkstemp()

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]

Luiz Fernando N. Capitulino said the following on 14.08.2007 21:44:
> This is a wrapper for mkstemp() that performs error checking and
> calls die() when an error occur.
> 
> Signed-off-by: Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>
> ---
>  git-compat-util.h |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 362e040..ca0a597 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -303,6 +303,16 @@ static inline FILE *xfdopen(int fd, const char *mode)
>  	return stream;
>  }
>  
> +static inline int xmkstemp(char *template)
> +{
> +	int fd;
> +
> +	fd = mkstemp(template);
> +	if (fd < 0)
> +		die("Unable to create temporary file: %s", strerror(errno));
> +	return fd;
> +}
> +
>  static inline size_t xsize_t(off_t len)
>  {
>  	return (size_t)len;

This functions needs to be further down in the file for
MinGW builds, since mkstemp() is forward declared later
in the file. So, something like this:

diff --git a/git-compat-util.h b/git-compat-util.h
index 9e075b7..f9ba8b0 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -477,4 +477,14 @@ extern __attribute__((noreturn)) int git_exit(int code);

 #endif /* __MINGW32__ */

+static inline int xmkstemp(char *template)
+{
+       int fd;
+
+       fd = mkstemp(template);
+       if (fd < 0)
+               die("Unable to create temporary file: %s", strerror(errno));
+       return fd;
+}
+
 #endif

-- 
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH 1/2] Introduces xmkstemp()

[Johannes Sixt]

Marius Storm-Olsen schrieb:
> This functions needs to be further down in the file for
> MinGW builds, since mkstemp() is forward declared later
> in the file. So, something like this:

Please consider using the lates MinGW port. It's already fixed there.

-- Hannes

Re: [PATCH 1/2] Introduces xmkstemp()

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

Johannes Sixt said the following on 31.08.2007 10:34:
> Marius Storm-Olsen schrieb:
>> This functions needs to be further down in the file for MinGW
>> builds, since mkstemp() is forward declared later in the file.
>> So, something like this:
> 
> Please consider using the lates MinGW port. It's already fixed
> there.

Right. (That's the whole 4msysgit.git mingw.git discussion on the 
msysgit mailinglist these days)

Just wanted to let the original author know, so he would be aware of 
it, and we wouldn't have to go the extra round-trips with patches to 
fix patches. (And for MinGW awareness when adding code to 
git-compat-util.h)

-- 
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH 1/2] Introduces xmkstemp()

[Johannes Sixt]

Marius Storm-Olsen schrieb:
> Just wanted to let the original author know, so he would be aware of it, 
> and we wouldn't have to go the extra round-trips with patches to fix 
> patches. (And for MinGW awareness when adding code to git-compat-util.h)

You can't expect a lot of MinGW awareness from non-MinGW users when they 
make changes to git-compat-util.h since upstream does not have _any_ 
references to MinGW there, yet.

-- Hannes

Re: [PATCH 1/2] Introduces xmkstemp()

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

Johannes Sixt said the following on 31.08.2007 10:49:
> Marius Storm-Olsen schrieb:
>> Just wanted to let the original author know, so he would be aware
>> of it, and we wouldn't have to go the extra round-trips with
>> patches to fix patches. (And for MinGW awareness when adding code
>> to git-compat-util.h)
> 
> You can't expect a lot of MinGW awareness from non-MinGW users when
> they make changes to git-compat-util.h since upstream does not have
> _any_ references to MinGW there, yet.

D'oh!
/me smacks self

We really need to streamline the patches needed for MinGW, and get 
those merged into upstream when 1.5.3 is released.

-- 
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

MinGW merge plans, was Re: [PATCH 1/2] Introduces xmkstemp()

[Johannes Schindelin]

Hi,

On Fri, 31 Aug 2007, Marius Storm-Olsen wrote:

> We really need to streamline the patches needed for MinGW, and get those 
> merged into upstream when 1.5.3 is released.

Okay, with 1.5.3 just about to break free, I think we can think about what 
to do to flood the mailing list with MinGW compatibility patches.

So I went through the diff between mingw.git and git.git (I did not use 
4msysgit.git as a reference yet).  There are many, many changes that are 
not really dependent on each other, so many, many people can participate 
;-)  (And of course, development on mingw.git and/or 4msysgit.git will not 
stall through that time.)

AFAICS these are the topics that need to be whipped into shape as 
individual patch series:

- NO_ST_BLOCKS (struct stat has no "blocks" member on MinGW)

- handle_line() in fmt-merge-msg.c should handle CRLF, too.  Maybe this 
  does not need to be special cased for MinGW at all.

- we have a special handling for relative template paths in MinGW.  I am 
  not sure if we need it any more.

- struct lock_file needs an "fd" member (assigned in lock_file()) so it 
  can close the fd before renaming in remove_lock_file(), 
  commit_lock_file() and commit_locked_index(), since Windows
  does not allow renaming of a still open file.  This does not need to be 
  special cased for MinGW IMHO.

- is_absolute_path() needs to be aware of <DRIVE>:/.  We should probably 
  do this for Windows in general, not just MinGW (as pointed out by Robin 
  in a recent mail, Cygwin suffers from this, too)

- For some reason, we use "/usr/bin/sort" instead of "sort" in 
  check-builtins.h, generate-cmdlist.sh, git-ls-remote.sh, 
  git-mergetool.sh and some tests.  It should be checked if this is still 
  necessary (AFAIR it was some Vista fsckup)

- A similar issue is /usr/bin/find in git-clone.sh, git-ls-remote.sh, 
  git-repack.sh, and some tests.

- fnmatch.[ch] were added to compat/.  IIRC these were "borrowed" from 
  GNU libc.

- A lot of functions were added to compat/mingw.c; someone should check 
  which are still needed, and which still need implementation (some 
  functions just return an error)

- regex.[ch] were added to compat/.  These were "borrowed" from GNU regex.

- We have git_etc_config() instead of ETC_GITCONFIG.  Probably this should 
  be changed to "#define ETC_GITCONFIG git_etc_config()" for MinGW only.  
  (AFAIR this is a workaround for setenv() on MinGW refreshing _all_ 
  environment variables, invalidating the previous getenv()ed addresses.

- Hannes implemented a spawn-pipe.h interface, to replace the fork() in 
  git_connect(), spawn_prog(), get_pack().  This is probably the most 
  difficult part to merge into git.git.  I am not clear about how much of 
  this should be moved into run-command.h instead, for example.

- We have a cpio "emulator" script, since MinGW lacks this tool.

- gettimeofday() in compat/mingw.c needs my_mktime() from date.c to be 
  public.  I did not form an opinion yet if compat/mingw.c should be 
  introduced in one go, or in incremental steps, implementing one function 
  at a time.

- In exec_cmd.c, we introduced a _function_ builtin_exec_path() to allow 
  determining the path from the executable name.  This is needed when 
  calling the git programs from cmd.exe, _without_ going through sh first.  
  IMHO this is wrong, since those programs can no longer call shell 
  scripts, as needed e.g. in hooks, merge drivers or git-gc.  So we 
  probably should get rid of this part (but I imagine before this, there 
  will be a fierce discussion about that), and force going through 
  /bin/sh.

- Many places do not call is_absolute_path() (because it was not there 
  earlier).  These have to be replaced.  (This patch is orthogonal to 
  MinGW.)

- We have a spawnv_git_cmd(), which is probably a duplication of the 
  run-command.h API.  We should get rid of that function, and adjust the 
  existing API if necessary.

- For some reason, support for side-band-64k and side-band is commented 
  out for MinGW.  Find out the reasoning, and if it still holds true, 
  introduce NO_SIDE_BAND into the Makefile and fetch-pack.c.

- In git-clone, we call "pwd -W" (which is utterly unportable...).  Find 
  out if it is still necessary.

- In git-compat-util.h, quite a bunch of header files have to be guarded 
  between #ifndef __MINGW32__ ... #endif.

- A bunch of #defines are in git-compat-util.h, which would have been 
  defined in the headers that do not exist on MinGW.  These should go into 
  an #else clause, and it should be documented which header would have had 
  those definitions.

- There are a couple of workarounds in git-compat-util.h, like a mkdir() 
  replacement which takes an "int mode" parameter, so that we do not need 
  a dozen special cases scattered throughout the source code (MinGW's 
  mkdir() does not take a "mode" parameter, since MSCRT's mkdir() doesn't)

- We have no "sync" on MinGW, and in git-repack.sh, this is worked around 
  by "sync 2> /dev/null".  This should be changed to something like

	test ! -z "@@NO_SYNC@@" || sync

- In prepend_to_path(), we need to replace the PATH delimiter ":" by ";"

- In run_command(), we return "status & 0xff", because Windows' version of 
  exit() apparently replaces all values outside of 0-0xff with 0!  We 
  probably need this in handle_internal_command(), too.

- In handle_internal_command(), we need to strip the extension ".exe".  
  This is done by defining STRIP_EXTENSION to ".exe" in the Makefile.

- In git.c's main(), we check for "\\" in addition to "/".  This is 
  probably not the only place needing this...  (For example, in setup.c we 
  roll an own version of it, limited to setup.c, which is bad.)

- In gitk, we still delete ~/.gitk everytime.  Check if this is still 
  necessary.

- help.c #includes <sys/ioctl.h>.  Why?

- We have NO_ETC_PASSWD, but this is a misnomer.  It would be more 
  appropriately named NEEDS_GECOS, and there should be a replacement 
  function "getpwuid()" in compat/mingw.c, which calls NetUserGetInfo()
  and fills a (custom) struct passwd.  The changes in ident.c and path.c
  should be undone.

- We have special pager handling in pager.c

- In path.c, we fall back to TMP or TEMP when TMPDIR is not set.  Since I 
  propose to always run in a shell, I am torn if we should keep this code.

- For some reason or other, we need some changes in perl/Makefile.

- We have special progress handling in progress.c

- In refs.c, we catch an erroneous errno (open can return EACCES when 
  EISDIR would be appropriate).  IMHO this should go into 
  git-compat-util.h, where the #define'd open() should be replaced by a 
  git_open() thing a la git_mkdir().

- In prefix_path(), we replace all "\\" to "/" in the path name.

- In prefix_path(), there is an #ifndef __MINGW32__, where the #else 
  clause uses is_absolute_path().  IMHO this is wrong, do not special case 
  that for MinGW.

- In setup_git_directory_gently(), there is special code for absolute 
  paths of the form <DRIVE>:/bla.  IMHO this should be changed so that 
  such paths are converted to /<DRIVE>/bla first.

- In sha1_file.c, we have yet another is_path_absolute() implementation.  
  Use is_absolute_path() here instead.

- In sha1_file.c, some fcntl()s are guarded (IIRC since MinGW does not 
  have fcntl() at all).  Make this a NO_FCNTL Makefile variable.

- Plenty of tests have to be adjusted for the fact that there are no 
  symlinks in MinGW.  This probably benefits from the recent addition of a 
  test for symlinks in git-init... (Needs adjusting, though, e.g. in 
  test-lib.sh).

- t0001 uses "pwd -W".

- t1002 implements a "sum" function if it was not found (this 
  is a patch which is independent of MinGW).

- t1003, t4109 and t4110 implement cmp via git diff.  Maybe change them 
  over to git diff?

- t1301 skips some tests.  Look into that.

- t3100 uses "diff" instead of "git diff".  Why?

- t3402m t3404, t4116 and t6200 sleep 1 second.  Probably because of 
  some file date granularity thing.

- t3404 replaces HEAD^ by HEAD~1, and something weird seems to happen 
  with "@" in the command line, when passed to scripts.

- t3800 replaces one egrep with grep, because egrep is not available.  
  IMHO egrep should not be used in git anyway.

- t3901 skips iconv tests (in msysgit, we recently added iconv, so this 
  might be worth fixing in MinGW).

- t3902 tests if funny filenames are allowed on the current filesystem 
  anyway.  If not, the tests are skipped.

- t5300 works around the lack of /dev/zero.  It also splits a big test 
  case into nice mouthfuls.

- t5301 works around the lack of /dev/random.

- t5302 just skips a few tests without a comment.  Find out why.

- t5502 works around a missing "git remote" (probably because we did not 
  have perl scripts running for quite some time).  Probably unnecessary.

- t5515 escapes curly brackets, but since git-tag is no longer a shell 
  script, this should not be necessary.

- t7500 is skipped.  Find out why "vi is not cooperating".

- t7501 uses an unportable "sed -i".  (Should be fixed in git.git anyway.)

- t7501 also skips tests without explanation.  Fix.

- t9001 says it cannot invoke fake.sendmail.  Fix.

- We have a NOEXECTEMPL to make up for the lack of chmod a+x in MinGW 
  (scripts are executable if and only if the first line starts with a #!). 
  Rename this to NOEXEC_SUFFIX.

- upload-pack.c works around the only fork() that does not exec() in 
  git.git, by defining a function and spawning a thread.  We should get 
  rid of all fork() calls outside run-command.c, and have something like 
  BEGINTHREAD=_beginthread in the Makefile.  This is a little involved.

- maybe_flush_or_die() special cases MinGW for returning EINVAL from 
  fflush() sometimes.  Either change this to _always_ check for EINVAL, or 
  do the git_fflush() trick in git-compat-util.h to replace errno with 
  EPIPE when it was set to EINVAL by fflush().

- imap-send is not built.

- convert-objects still is not built.  I think it is that date function 
  which we could replace by some call to date.c.

Phew.

Ciao,
Dscho

Re: [PATCH 1/2] Introduces xmkstemp()

[Luiz Fernando N. Capitulino]

Em Fri, 31 Aug 2007 10:42:53 +0200
Marius Storm-Olsen <marius@trolltech.com> escreveu:

| Johannes Sixt said the following on 31.08.2007 10:34:
| > Marius Storm-Olsen schrieb:
| >> This functions needs to be further down in the file for MinGW
| >> builds, since mkstemp() is forward declared later in the file.
| >> So, something like this:
| > 
| > Please consider using the lates MinGW port. It's already fixed
| > there.
| 
| Right. (That's the whole 4msysgit.git mingw.git discussion on the 
| msysgit mailinglist these days)
| 
| Just wanted to let the original author know, so he would be aware of 
| it, and we wouldn't have to go the extra round-trips with patches to 
| fix patches. (And for MinGW awareness when adding code to 
| git-compat-util.h)

 ACKed, thank you Marius.

-- 
Luiz Fernando N. Capitulino

Re: MinGW merge plans, was Re: [PATCH 1/2] Introduces xmkstemp()

[Johannes Sixt]

[I fscked up my email address in my news reader setup. This mail is sent 
from a correct one. Sorry to all who might have received bounces.]

Johannes Schindelin schrieb:
> Hi,
> 
> On Fri, 31 Aug 2007, Marius Storm-Olsen wrote:
> 
>> We really need to streamline the patches needed for MinGW, and get
>> those merged into upstream when 1.5.3 is released.
> 
> Okay, with 1.5.3 just about to break free, I think we can think about
> what to do to flood the mailing list with MinGW compatibility
> patches.

Thanks for summarizing the changes and things to do. I'll respond to
this with more details later this evening, but only to the msysgit list.
(Interested parties are encouraged to join the list - see
http://groups.google.com/group/msysgit).

I've started to rearrange patches and separate out stuff that is not
MinGW specific, but this effort has stalled. I'm waiting for a new
notebook to arrive so that I can continue it during off-work hours.

-- Hannes

Re: [PATCH 1/2] Introduces xmkstemp()

[Luiz Fernando N. Capitulino]

Em Fri, 31 Aug 2007 10:49:16 +0200
Johannes Sixt <j.sixt@telecom.at> escreveu:

| Marius Storm-Olsen schrieb:
| > Just wanted to let the original author know, so he would be aware of it, 
| > and we wouldn't have to go the extra round-trips with patches to fix 
| > patches. (And for MinGW awareness when adding code to git-compat-util.h)
| 
| You can't expect a lot of MinGW awareness from non-MinGW users when they 
| make changes to git-compat-util.h since upstream does not have _any_ 
| references to MinGW there, yet.

 Droping people from the CC doesn't help either.

 Please, do not do that.

-- 
Luiz Fernando N. Capitulino