Re: [PATCH 2/2] mingw: ensure temporary file handles are not inherited by child processes

[Lars Schneider <larsxschneider@gmail.com>]

> On 17 Aug 2016, at 14:41, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> 
> From: Ben Wijen <ben@wijen.net>
> 
> When the index is locked and child processes inherit the handle to
> said lock and the parent process wants to remove the lock before the
> child process exits, on Windows there is a problem: it won't work
> because files cannot be deleted if a process holds a handle on them.
> The symptom:
> 
>    Rename from 'xxx/.git/index.lock' to 'xxx/.git/index' failed.
>    Should I try again? (y/n)
> 
> Spawning child processes with bInheritHandles==FALSE would not work
> because no file handles would be inherited, not even the hStdXxx
> handles in STARTUPINFO (stdin/stdout/stderr).
> 
> Opening every file with O_NOINHERIT does not work, either, as e.g.
> git-upload-pack expects inherited file handles.
> 
> This leaves us with the only way out: creating temp files with the
> O_NOINHERIT flag. This flag is Windows-specific, however. For our
> purposes, it is equivalent our purposes) to O_CLOEXEC (which does not
> exist on Windows), so let's just open temporary files with the
> O_CLOEXEC flag and map that flag to O_NOINHERIT on Windows.
> 
> This fixes the test that we just introduced to demonstrate the problem.
> 
> Signed-off-by: Ben Wijen <ben@wijen.net>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> compat/mingw.h        | 4 ++++
> t/t6026-merge-attr.sh | 2 +-
> tempfile.c            | 2 +-
> 3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 95e128f..753e641 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -67,6 +67,10 @@ typedef int pid_t;
> #define F_SETFD 2
> #define FD_CLOEXEC 0x1
> 
> +#if !defined O_CLOEXEC && defined O_NOINHERIT
> +#define O_CLOEXEC	O_NOINHERIT
> +#endif
> +
> #ifndef EAFNOSUPPORT
> #define EAFNOSUPPORT WSAEAFNOSUPPORT
> #endif
> diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
> index 3d28c78..dd8f88d 100755
> --- a/t/t6026-merge-attr.sh
> +++ b/t/t6026-merge-attr.sh
> @@ -181,7 +181,7 @@ test_expect_success 'up-to-date merge without common ancestor' '
> 	)
> '
> 
> -test_expect_success !MINGW 'custom merge does not lock index' '
> +test_expect_success 'custom merge does not lock index' '
> 	git reset --hard anchor &&
> 	write_script sleep-one-second.sh <<-\EOF &&
> 		sleep 1 &
> diff --git a/tempfile.c b/tempfile.c
> index 0af7ebf..db3981d 100644
> --- a/tempfile.c
> +++ b/tempfile.c
> @@ -120,7 +120,7 @@ int create_tempfile(struct tempfile *tempfile, const char *path)
> 	prepare_tempfile_object(tempfile);
> 
> 	strbuf_add_absolute_path(&tempfile->filename, path);
> -	tempfile->fd = open(tempfile->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
> +	tempfile->fd = open(tempfile->filename.buf, O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, 0666);

This solution works great for me. I struggled with a similar problem 
in my Git filter protocol series:
http://public-inbox.org/git/20160810130411.12419-16-larsxschneider@gmail.com/

I also tried selectively disabling inheritance for file handles but it looks like
as this is not easily possible right now: 
https://github.com/git-for-windows/git/issues/770#issuecomment-238816331

- Lars