[PATCH] checkout files in-place

[PATCH] checkout files in-place

[Clemens Buchacher]

When replacing files with new content during checkout, we do not write
to them in-place. Instead we unlink and re-create the files in order to
let the system figure out ownership and permissions for the new file,
taking umask into account.

It is safe to do this on Linux file systems, even if open file handles
still exist, because unlink only removes the directory reference to the
file. On Windows, however, a file cannot be deleted until all handles to
it are closed. If a file cannot be deleted, its name cannot be reused.

This causes files to be deleted, but not checked out when switching
branches. This is frequently an issue with Qt Creator, which
continuously opens files in the work tree, as reported here:
https://github.com/git-for-windows/git/issues/1653

This change adds the core.checkout_inplace option. If enabled, checkout
will open files for writing the new content in-place. This fixes the
issue, but with this approach the system will not update file
permissions according to umask. Only essential updates of write and
executable permissions are performed.

The in-place checkout is therefore optional. It could be enabled by Git
installers on Windows, where umask is irrelevant.

Signed-off-by: Clemens Buchacher <drizzd@gmx.net>
---

I wonder if Git should be responsible for updating ownership and file
permissions when modifying existing files during checkout. We could
otherwise remove the unlink completely. Maybe this could even improve
performance in some cases. It made no difference in a short test on
Windows.

Regression tests are running. This will take a while.

 Documentation/config.txt    |  8 ++++++++
 cache.h                     |  2 ++
 config.c                    |  5 +++++
 entry.c                     | 18 +++++++++++++++---
 environment.c               |  1 +
 t/t2031-checkout-inplace.sh | 41 +++++++++++++++++++++++++++++++++++++++++
 6 files changed, 72 insertions(+), 3 deletions(-)
 create mode 100755 t/t2031-checkout-inplace.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index b6cb997164..17af0fe163 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -923,6 +923,14 @@ core.sparseCheckout::
 	Enable "sparse checkout" feature. See section "Sparse checkout" in
 	linkgit:git-read-tree[1] for more information.
 
+core.checkoutInplace::
+	Checkout file contents in-place. By default Git checkout removes existing
+	work tree files before it replaces them with different contents. If this
+	option is enabled Git will overwrite the contents of existing files
+	in-place. This is useful on systems where open file handles to a removed
+	file prevent creating new files at the same path. Note that Git will not
+	update read/write permissions according to umask.
+
 core.abbrev::
 	Set the length object names are abbreviated to.  If
 	unspecified or set to "auto", an appropriate value is
diff --git a/cache.h b/cache.h
index 2c640d4c31..c8fccd2a80 100644
--- a/cache.h
+++ b/cache.h
@@ -808,6 +808,7 @@ extern char *git_replace_ref_base;
 extern int fsync_object_files;
 extern int core_preload_index;
 extern int core_apply_sparse_checkout;
+extern int checkout_inplace;
 extern int precomposed_unicode;
 extern int protect_hfs;
 extern int protect_ntfs;
@@ -1530,6 +1531,7 @@ struct checkout {
 	unsigned force:1,
 		 quiet:1,
 		 not_new:1,
+		 inplace:1,
 		 refresh_cache:1;
 };
 #define CHECKOUT_INIT { NULL, "" }
diff --git a/config.c b/config.c
index cd2b404b14..4ac2407057 100644
--- a/config.c
+++ b/config.c
@@ -1231,6 +1231,11 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.checkoutinplace")) {
+		checkout_inplace = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "core.precomposeunicode")) {
 		precomposed_unicode = git_config_bool(var, value);
 		return 0;
diff --git a/entry.c b/entry.c
index 31c00816dc..54c98870b9 100644
--- a/entry.c
+++ b/entry.c
@@ -78,8 +78,13 @@ static void remove_subtree(struct strbuf *path)
 
 static int create_file(const char *path, unsigned int mode)
 {
+	int flags;
+	if (checkout_inplace)
+		flags = O_WRONLY | O_CREAT | O_TRUNC;
+	else
+		flags = O_WRONLY | O_CREAT | O_EXCL;
 	mode = (mode & 0100) ? 0777 : 0666;
-	return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);
+	return open(path, flags, mode);
 }
 
 static void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)
@@ -470,8 +475,15 @@ int checkout_entry(struct cache_entry *ce,
 			if (!state->force)
 				return error("%s is a directory", path.buf);
 			remove_subtree(&path);
-		} else if (unlink(path.buf))
-			return error_errno("unable to unlink old '%s'", path.buf);
+		} else if (checkout_inplace) {
+			if (!(st.st_mode & 0200) ||
+			    (trust_executable_bit && (st.st_mode & 0100) != (ce->ce_mode & 0100)))
+				if (chmod(path.buf, (ce->ce_mode & 0100) ? 0777 : 0666))
+					return error_errno("unable to change mode of '%s'", path.buf);
+		} else {
+			if (unlink(path.buf))
+				return error_errno("unable to unlink old '%s'", path.buf);
+		}
 	} else if (state->not_new)
 		return 0;
 
diff --git a/environment.c b/environment.c
index d1ac37dd18..6a8036b144 100644
--- a/environment.c
+++ b/environment.c
@@ -63,6 +63,7 @@ enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
 char *notes_ref_name;
 int grafts_replace_parents = 1;
 int core_apply_sparse_checkout;
+int checkout_inplace;
 int merge_log_config = -1;
 int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
 unsigned long pack_size_limit_cfg;
diff --git a/t/t2031-checkout-inplace.sh b/t/t2031-checkout-inplace.sh
new file mode 100755
index 0000000000..60ea30cbf5
--- /dev/null
+++ b/t/t2031-checkout-inplace.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+test_description='checkout inplace'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+
+	git config core.checkoutInplace true &&
+	echo hello >world &&
+	git add world &&
+	git commit -m initial &&
+	git branch other &&
+	echo "hello again" >>world &&
+	git add world &&
+	git commit -m second
+'
+
+test_expect_success 'checkout overwrites open file' '
+
+	git checkout -f master &&
+	mkfifo input &&
+	{
+		cat >>world <input &
+	} &&
+	pid=$! &&
+	test_when_finished "kill -KILL $pid; wait $pid; rm -f input" &&
+	git checkout other &&
+	echo hello >expect &&
+	test_cmp expect world
+'
+
+test_expect_success 'checkout overwrites read-only file' '
+
+	git checkout -f master &&
+	chmod -w world &&
+	git checkout other &&
+	echo hello >expect &&
+	test_cmp expect world
+'
+
+test_done
-- 
2.16.1.windows.1

Re: [PATCH] checkout files in-place

[brian m. carlson]

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

On Sun, Jun 10, 2018 at 09:44:45PM +0200, Clemens Buchacher wrote:
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index b6cb997164..17af0fe163 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -923,6 +923,14 @@ core.sparseCheckout::
>  	Enable "sparse checkout" feature. See section "Sparse checkout" in
>  	linkgit:git-read-tree[1] for more information.
>  
> +core.checkoutInplace::

Perhaps "core.checkoutInPlace" (captialized "place")?

> +	Checkout file contents in-place. By default Git checkout removes existing

"Check out".

> +	work tree files before it replaces them with different contents. If this
> +	option is enabled Git will overwrite the contents of existing files
> +	in-place. This is useful on systems where open file handles to a removed

Here and above, uou don't need to hyphenate "in place" when used as an
adverb, only when using it as an adjective before the noun (e.g.
"in-place checkout").

> +	file prevent creating new files at the same path. Note that Git will not
> +	update read/write permissions according to umask.

I'm wondering if it's worth a mention that running out of disk space (or
quota) will cause data to be truncated.

>  static void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)
> @@ -470,8 +475,15 @@ int checkout_entry(struct cache_entry *ce,
>  			if (!state->force)
>  				return error("%s is a directory", path.buf);
>  			remove_subtree(&path);
> -		} else if (unlink(path.buf))
> -			return error_errno("unable to unlink old '%s'", path.buf);
> +		} else if (checkout_inplace) {
> +			if (!(st.st_mode & 0200) ||
> +			    (trust_executable_bit && (st.st_mode & 0100) != (ce->ce_mode & 0100)))
> +				if (chmod(path.buf, (ce->ce_mode & 0100) ? 0777 : 0666))
> +					return error_errno("unable to change mode of '%s'", path.buf);

So in-place checkout won't work if the mode changes and we're not the
owner of the file.  One place where I could see people wanting to use
this on Unix is shared repositories with BSD group semantics, but that
wouldn't work reliably.

I don't see that as a problem, as that isn't the issue this patch is
trying to solve, but it may end up biting people.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: [PATCH] checkout files in-place

[Clemens Buchacher]

On Mon, Jun 11, 2018 at 02:04:11AM +0000, brian m. carlson wrote:
> On Sun, Jun 10, 2018 at 09:44:45PM +0200, Clemens Buchacher wrote:
> > +	file prevent creating new files at the same path. Note that Git will not
> > +	update read/write permissions according to umask.
> 
> I'm wondering if it's worth a mention that running out of disk space (or
> quota) will cause data to be truncated.

As far as I know we make no guarantees about the behavior when running
out of disk space. There could be other side effects, so I cannot safely
state anything here.

Re: [PATCH] checkout files in-place

[Ævar Arnfjörð Bjarmason]

On Sun, Jun 10 2018, Clemens Buchacher wrote:

> When replacing files with new content during checkout, we do not write
> to them in-place. Instead we unlink and re-create the files in order to
> let the system figure out ownership and permissions for the new file,
> taking umask into account.
>
> It is safe to do this on Linux file systems, even if open file handles
> still exist, because unlink only removes the directory reference to the
> file. On Windows, however, a file cannot be deleted until all handles to
> it are closed. If a file cannot be deleted, its name cannot be reused.
>
> This causes files to be deleted, but not checked out when switching
> branches. This is frequently an issue with Qt Creator, which
> continuously opens files in the work tree, as reported here:
> https://github.com/git-for-windows/git/issues/1653
>
> This change adds the core.checkout_inplace option. If enabled, checkout

The commit message should mention core.checkoutInPlace, not
core.checkout_inplace.

> will open files for writing the new content in-place. This fixes the
> issue, but with this approach the system will not update file
> permissions according to umask. Only essential updates of write and
> executable permissions are performed.
>
> The in-place checkout is therefore optional. It could be enabled by Git
> installers on Windows, where umask is irrelevant.

I think some of this...

> +core.checkoutInplace::
> +	Checkout file contents in-place. By default Git checkout removes existing
> +	work tree files before it replaces them with different contents. If this
> +	option is enabled Git will overwrite the contents of existing files
> +	in-place. This is useful on systems where open file handles to a removed
> +	file prevent creating new files at the same path. Note that Git will not
> +	update read/write permissions according to umask.
> +

...should be added to these docs. In particular let's not be coy and say
"on systems where", and instead describe that this is meant for Windows,
so users looking for that in the config man-page will see it.

We should also document the "doesn't update permissions" bit, and be
clear whether that's expected future behavior we aim to preserve, or
just a side-effect of the current implementation and may change.

>  core.abbrev::
>  	Set the length object names are abbreviated to.  If
>  	unspecified or set to "auto", an appropriate value is
> diff --git a/cache.h b/cache.h
> index 2c640d4c31..c8fccd2a80 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -808,6 +808,7 @@ extern char *git_replace_ref_base;
>  extern int fsync_object_files;
>  extern int core_preload_index;
>  extern int core_apply_sparse_checkout;
> +extern int checkout_inplace;
>  extern int precomposed_unicode;
>  extern int protect_hfs;
>  extern int protect_ntfs;
> @@ -1530,6 +1531,7 @@ struct checkout {
>  	unsigned force:1,
>  		 quiet:1,
>  		 not_new:1,
> +		 inplace:1,
>  		 refresh_cache:1;
>  };
>  #define CHECKOUT_INIT { NULL, "" }
> diff --git a/config.c b/config.c
> index cd2b404b14..4ac2407057 100644
> --- a/config.c
> +++ b/config.c
> @@ -1231,6 +1231,11 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
>  		return 0;
>  	}
>
> +	if (!strcmp(var, "core.checkoutinplace")) {
> +		checkout_inplace = git_config_bool(var, value);
> +		return 0;
> +	}
> +
>  	if (!strcmp(var, "core.precomposeunicode")) {
>  		precomposed_unicode = git_config_bool(var, value);
>  		return 0;
> diff --git a/entry.c b/entry.c
> index 31c00816dc..54c98870b9 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -78,8 +78,13 @@ static void remove_subtree(struct strbuf *path)
>
>  static int create_file(const char *path, unsigned int mode)
>  {
> +	int flags;
> +	if (checkout_inplace)
> +		flags = O_WRONLY | O_CREAT | O_TRUNC;
> +	else
> +		flags = O_WRONLY | O_CREAT | O_EXCL;
>  	mode = (mode & 0100) ? 0777 : 0666;
> -	return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);
> +	return open(path, flags, mode);
>  }
>
>  static void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)
> @@ -470,8 +475,15 @@ int checkout_entry(struct cache_entry *ce,
>  			if (!state->force)
>  				return error("%s is a directory", path.buf);
>  			remove_subtree(&path);
> -		} else if (unlink(path.buf))
> -			return error_errno("unable to unlink old '%s'", path.buf);
> +		} else if (checkout_inplace) {
> +			if (!(st.st_mode & 0200) ||
> +			    (trust_executable_bit && (st.st_mode & 0100) != (ce->ce_mode & 0100)))
> +				if (chmod(path.buf, (ce->ce_mode & 0100) ? 0777 : 0666))
> +					return error_errno("unable to change mode of '%s'", path.buf);
> +		} else {
> +			if (unlink(path.buf))
> +				return error_errno("unable to unlink old '%s'", path.buf);
> +		}
>  	} else if (state->not_new)
>  		return 0;
>
> diff --git a/environment.c b/environment.c
> index d1ac37dd18..6a8036b144 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -63,6 +63,7 @@ enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
>  char *notes_ref_name;
>  int grafts_replace_parents = 1;
>  int core_apply_sparse_checkout;
> +int checkout_inplace;
>  int merge_log_config = -1;
>  int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
>  unsigned long pack_size_limit_cfg;
> diff --git a/t/t2031-checkout-inplace.sh b/t/t2031-checkout-inplace.sh
> new file mode 100755
> index 0000000000..60ea30cbf5
> --- /dev/null
> +++ b/t/t2031-checkout-inplace.sh
> @@ -0,0 +1,41 @@
> +#!/bin/sh
> +
> +test_description='checkout inplace'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +
> +	git config core.checkoutInplace true &&
> +	echo hello >world &&
> +	git add world &&
> +	git commit -m initial &&
> +	git branch other &&
> +	echo "hello again" >>world &&
> +	git add world &&
> +	git commit -m second
> +'

Would be easier to read if you used the "test_commit" helper.

> +test_expect_success 'checkout overwrites open file' '
> +
> +	git checkout -f master &&
> +	mkfifo input &&
> +	{
> +		cat >>world <input &
> +	} &&
> +	pid=$! &&
> +	test_when_finished "kill -KILL $pid; wait $pid; rm -f input" &&
> +	git checkout other &&
> +	echo hello >expect &&
> +	test_cmp expect world
> +'
> +
> +test_expect_success 'checkout overwrites read-only file' '
> +
> +	git checkout -f master &&
> +	chmod -w world &&
> +	git checkout other &&
> +	echo hello >expect &&
> +	test_cmp expect world
> +'
> +
> +test_done

There seem to be no tests here for the chmod +x case you implemented,
and it would be worthwhile to have an explicit test where we change the
umask and observe that a file's permissions will change without this
setting, but not with it.

Re: [PATCH] checkout files in-place

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

>> +	file prevent creating new files at the same path. Note that Git will not
>> +	update read/write permissions according to umask.
>
> I'm wondering if it's worth a mention that running out of disk space (or
> quota) will cause data to be truncated.

Aside from us not having to worry about emulating the umask, another
reason why we prefer "create, complete the write, and then finally
rename" over "overwrite and let it fail in the middle" is that the
former makes sure we never leave the path in a bad state.  It either
has a complete copy of the old contents, or a complete copy of the
new contents, and a third-party process reading from sidelines would
not get a partial copy, regardless of disc-full issue.

Re: [PATCH] checkout files in-place

[Clemens Buchacher]

On Mon, Jun 11, 2018 at 11:02:42AM -0700, Junio C Hamano wrote:
> 
> Aside from us not having to worry about emulating the umask, another
> reason why we prefer "create, complete the write, and then finally
> rename" over "overwrite and let it fail in the middle" is that the
> former makes sure we never leave the path in a bad state.

But the current checkout implementation does not do this. It writes
directly to the target location. The only difference to in-place
checkout is that files are unlinked before they are opened for writing.

Re: [PATCH] checkout files in-place

[Edward Thomson]

On Sun, Jun 10, 2018 at 09:44:45PM +0200, Clemens Buchacher wrote:
> 
> It is safe to do this on Linux file systems, even if open file handles
> still exist, because unlink only removes the directory reference to the
> file. On Windows, however, a file cannot be deleted until all handles to
> it are closed. If a file cannot be deleted, its name cannot be reused.

I'm nervous about this proposed change, since it feels like it's
addressing an issue that only exists in QT Creator.

You've accurately described the default semantics in Win32.  A file
cannot be deleted until all handles to it are closed, unless it was
opened with `FILE_SHARE_DELETE` as their sharing mode.  This is not the
default sharing mode in either Win32 or .NET.

However, for your patch to have an effect, all processes with a handle
open must have specified `FILE_SHARE_WRITE`.  This is rather uncommon,
since it's also not included in the default Win32 or .NET sharing mode.
This is because it's uncommon that you would want other processes to
change the data underneath you in between ReadFile() calls.

So your patch will benefit people who have processes that have
`FILE_SHARE_WRITE` set but not `FILE_SHARE_DELETE` set, which I think is
generally an uncommon scenario to want to support.

Generally if you're willing to accept files changing underneath you,
then you probably want to allow them to be deleted, too.  So this feels
like something that's very specific to QT Creator.  Or are there other
IDEs or development tools that use these open semantics that I'm not
aware of?

Cheers-
-ed

[PATCH v2] checkout files in-place

[Clemens Buchacher]

When replacing files with new content during checkout, we do not write
to them in place. Instead we unlink and recreate the files in order to
let the system figure out ownership and permissions for the new file,
taking umask into account.

It is safe to do this on Linux file systems, even if open file handles
still exist, because unlink only removes the directory reference to the
file. On Windows, however, a file cannot be deleted until all handles to
it are closed. If a file cannot be deleted, its name cannot be reused.

This causes files to be deleted, but not checked out when switching
branches. This is frequently an issue with Qt Creator, which
continuously opens files in the work tree, as reported here:
https://github.com/git-for-windows/git/issues/1653

This change adds the core.checkoutInPlace option. If enabled, checkout
will open files for writing the new content in place. This fixes the
issue, but with this approach the system will not update file
permissions according to umask. Only essential updates of write and
executable permissions are performed.

The in-place checkout is therefore optional. It could be enabled by Git
installers on Windows, where umask is irrelevant.

Signed-off-by: Clemens Buchacher <drizzd@gmx.net>
Reviewed-by: Orgad Shaneh <orgads@gmail.com>
Reviewed-by: "brian m. carlson" <sandals@crustytoothpaste.net>
Reviewed-by: Duy Nguyen <pclouds@gmail.com>
Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Tested on Windows with Git-for-Windows and with Windows Subsystem for
Linux.

 Documentation/config.txt    | 11 ++++++
 cache.h                     |  2 ++
 config.c                    |  5 +++
 entry.c                     | 18 ++++++++--
 environment.c               |  1 +
 t/t2031-checkout-inplace.sh | 82 +++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 116 insertions(+), 3 deletions(-)
 create mode 100755 t/t2031-checkout-inplace.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf..0860a81 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -912,6 +912,17 @@ core.sparseCheckout::
 	Enable "sparse checkout" feature. See section "Sparse checkout" in
 	linkgit:git-read-tree[1] for more information.
 
+core.checkoutInPlace::
+	Check out file contents in place. By default Git checkout removes existing
+	work tree files before it replaces them with different content. If this
+	option is enabled, Git will overwrite the contents of existing files in
+	place. This is useful on Windows, where open file handles to a removed file
+	prevent creating new files at the same path.
+	Note that the current implementation of in-place checkout makes no effort
+	to update read/write permissions according to umask. Permissions are
+	however modified to enable write access and to update executable
+	permissions.
+
 core.abbrev::
 	Set the length object names are abbreviated to.  If
 	unspecified or set to "auto", an appropriate value is
diff --git a/cache.h b/cache.h
index 89a107a..5b8c4d6 100644
--- a/cache.h
+++ b/cache.h
@@ -815,6 +815,7 @@ extern int fsync_object_files;
 extern int core_preload_index;
 extern int core_commit_graph;
 extern int core_apply_sparse_checkout;
+extern int checkout_inplace;
 extern int precomposed_unicode;
 extern int protect_hfs;
 extern int protect_ntfs;
@@ -1518,6 +1519,7 @@ struct checkout {
 	unsigned force:1,
 		 quiet:1,
 		 not_new:1,
+		 inplace:1,
 		 refresh_cache:1;
 };
 #define CHECKOUT_INIT { NULL, "" }
diff --git a/config.c b/config.c
index fbbf0f8..8b35ecd 100644
--- a/config.c
+++ b/config.c
@@ -1318,6 +1318,11 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.checkoutinplace")) {
+		checkout_inplace = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "core.precomposeunicode")) {
 		precomposed_unicode = git_config_bool(var, value);
 		return 0;
diff --git a/entry.c b/entry.c
index 2101201..a599fc1 100644
--- a/entry.c
+++ b/entry.c
@@ -78,8 +78,13 @@ static void remove_subtree(struct strbuf *path)
 
 static int create_file(const char *path, unsigned int mode)
 {
+	int flags;
+	if (checkout_inplace)
+		flags = O_WRONLY | O_CREAT | O_TRUNC;
+	else
+		flags = O_WRONLY | O_CREAT | O_EXCL;
 	mode = (mode & 0100) ? 0777 : 0666;
-	return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);
+	return open(path, flags, mode);
 }
 
 static void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)
@@ -467,8 +472,15 @@ int checkout_entry(struct cache_entry *ce,
 			if (!state->force)
 				return error("%s is a directory", path.buf);
 			remove_subtree(&path);
-		} else if (unlink(path.buf))
-			return error_errno("unable to unlink old '%s'", path.buf);
+		} else if (checkout_inplace) {
+			if (!(st.st_mode & 0200) ||
+			    (trust_executable_bit && (st.st_mode & 0100) != (ce->ce_mode & 0100)))
+				if (chmod(path.buf, (ce->ce_mode & 0100) ? 0777 : 0666))
+					return error_errno(_("unable to change mode of '%s'"), path.buf);
+		} else {
+			if (unlink(path.buf))
+				return error_errno(_("unable to unlink old '%s'"), path.buf);
+		}
 	} else if (state->not_new)
 		return 0;
 
diff --git a/environment.c b/environment.c
index 2a6de23..5b91f30 100644
--- a/environment.c
+++ b/environment.c
@@ -68,6 +68,7 @@ char *notes_ref_name;
 int grafts_replace_parents = 1;
 int core_commit_graph;
 int core_apply_sparse_checkout;
+int checkout_inplace;
 int merge_log_config = -1;
 int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
 unsigned long pack_size_limit_cfg;
diff --git a/t/t2031-checkout-inplace.sh b/t/t2031-checkout-inplace.sh
new file mode 100755
index 0000000..d70ecc4
--- /dev/null
+++ b/t/t2031-checkout-inplace.sh
@@ -0,0 +1,82 @@
+#!/bin/sh
+
+test_description='in-place checkout'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+
+	test_commit hello world &&
+	git branch other &&
+	test_commit hello-again world
+'
+
+test_expect_success 'in-place checkout overwrites open file' '
+
+	git config core.checkoutInPlace true &&
+	git checkout -f master &&
+	exec 8<world &&
+	git checkout other &&
+	exec 8<&- &&
+	echo hello >expect &&
+	test_cmp expect world
+'
+
+test_expect_success 'in-place checkout overwrites read-only file' '
+
+	git config core.checkoutInPlace true &&
+	git checkout -f master &&
+	chmod -w world &&
+	git checkout other &&
+	echo hello >expect &&
+	test_cmp expect world
+'
+
+test_expect_success 'in-place checkout updates executable permission' '
+
+	git config core.checkoutInPlace true &&
+	git checkout -f master^0 &&
+	test_chmod +x world &&
+	git commit -m executable &&
+	git checkout other &&
+	test ! -x world
+'
+
+test_expect_success POSIXPERM 'regular checkout respects umask' '
+
+	git config core.checkoutInPlace false &&
+	git checkout -f master &&
+	chmod 0660 world &&
+	umask 0022 &&
+	git checkout other &&
+	actual=$(ls -l world) &&
+	case "$actual" in
+	-rw-r--r--*)
+		: happy
+		;;
+	*)
+		echo Oops, world is not 0644 but $actual
+		false
+		;;
+	esac
+'
+
+test_expect_success POSIXPERM 'in-place checkout ignores umask' '
+
+	git config core.checkoutInPlace true &&
+	git checkout -f master &&
+	chmod 0660 world &&
+	umask 0022 &&
+	git checkout other &&
+	actual=$(ls -l world) &&
+	case "$actual" in
+	-rw-rw----*)
+		: happy
+		;;
+	*)
+		echo Oops, world is not 0660 but $actual
+		false
+		;;
+	esac
+'
+
+test_done
-- 
2.7.4

Re: [PATCH] checkout files in-place

[Clemens Buchacher]

+Cc: Orgad Shaneh

On Mon, Jun 11, 2018 at 08:35:41PM +0000, Edward Thomson wrote:
> On Sun, Jun 10, 2018 at 09:44:45PM +0200, Clemens Buchacher wrote:
> > 
> > It is safe to do this on Linux file systems, even if open file handles
> > still exist, because unlink only removes the directory reference to the
> > file. On Windows, however, a file cannot be deleted until all handles to
> > it are closed. If a file cannot be deleted, its name cannot be reused.
> 
> I'm nervous about this proposed change, since it feels like it's
> addressing an issue that only exists in QT Creator.
> 
> You've accurately described the default semantics in Win32.  A file
> cannot be deleted until all handles to it are closed, unless it was
> opened with `FILE_SHARE_DELETE` as their sharing mode.  This is not the
> default sharing mode in either Win32 or .NET.
> 
> However, for your patch to have an effect, all processes with a handle
> open must have specified `FILE_SHARE_WRITE`.  This is rather uncommon,
> since it's also not included in the default Win32 or .NET sharing mode.
> This is because it's uncommon that you would want other processes to
> change the data underneath you in between ReadFile() calls.
> 
> So your patch will benefit people who have processes that have
> `FILE_SHARE_WRITE` set but not `FILE_SHARE_DELETE` set, which I think is
> generally an uncommon scenario to want to support.
> 
> Generally if you're willing to accept files changing underneath you,
> then you probably want to allow them to be deleted, too.  So this feels
> like something that's very specific to QT Creator.  Or are there other
> IDEs or development tools that use these open semantics that I'm not
> aware of?

I am also not aware of other IDEs which have this issue.

Orgad, you also mentioned FILE_SHARE_DELETE here [*1*]. Does the Qt
Creator issue persist despite this flag? You also just commented on
Github that "Regarding Qt Creator, the issue should be mostly solved by
now in 4.7". So a fix in Git is no longer needed?

[*1*] https://github.com/git-for-windows/git/pull/1666

Re: [PATCH v2] checkout files in-place

[Ævar Arnfjörð Bjarmason]

On Mon, Jun 11 2018, Clemens Buchacher wrote:

> When replacing files with new content during checkout, we do not write
> to them in place. Instead we unlink and recreate the files in order to
> let the system figure out ownership and permissions for the new file,
> taking umask into account.

Both this summary...

> It is safe to do this on Linux file systems, even if open file handles
> still exist, because unlink only removes the directory reference to the
> file. On Windows, however, a file cannot be deleted until all handles to
> it are closed. If a file cannot be deleted, its name cannot be reused.
>
> This causes files to be deleted, but not checked out when switching
> branches. This is frequently an issue with Qt Creator, which
> continuously opens files in the work tree, as reported here:
> https://github.com/git-for-windows/git/issues/1653
>
> This change adds the core.checkoutInPlace option. If enabled, checkout
> will open files for writing the new content in place. This fixes the
> issue, but with this approach the system will not update file
> permissions according to umask. Only essential updates of write and
> executable permissions are performed.
>
> The in-place checkout is therefore optional. It could be enabled by Git
> installers on Windows, where umask is irrelevant.
>
> Signed-off-by: Clemens Buchacher <drizzd@gmx.net>
> Reviewed-by: Orgad Shaneh <orgads@gmail.com>
> Reviewed-by: "brian m. carlson" <sandals@crustytoothpaste.net>
> Reviewed-by: Duy Nguyen <pclouds@gmail.com>
> Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> Tested on Windows with Git-for-Windows and with Windows Subsystem for
> Linux.
>
>  Documentation/config.txt    | 11 ++++++
>  cache.h                     |  2 ++
>  config.c                    |  5 +++
>  entry.c                     | 18 ++++++++--
>  environment.c               |  1 +
>  t/t2031-checkout-inplace.sh | 82 +++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 116 insertions(+), 3 deletions(-)
>  create mode 100755 t/t2031-checkout-inplace.sh
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ab641bf..0860a81 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -912,6 +912,17 @@ core.sparseCheckout::
>  	Enable "sparse checkout" feature. See section "Sparse checkout" in
>  	linkgit:git-read-tree[1] for more information.
>
> +core.checkoutInPlace::
> +	Check out file contents in place. By default Git checkout removes existing
> +	work tree files before it replaces them with different content. If this
> +	option is enabled, Git will overwrite the contents of existing files in
> +	place. This is useful on Windows, where open file handles to a removed file
> +	prevent creating new files at the same path.

...And this seems to conflict with what Junio's summarized in
xmqqvaapb3r1.fsf@gitster-ct.c.googlers.com. I.e. (if I'm reading it
correctly) it's not correct to say that we unlink the existing file,
then replace it, don't we create a new one, and then rename it in-place?

I don't know enough about this part of the code to know, but whatever it
is we should get it right here.

Also, as Junio notes that pattern will not result in a potentially
corrupt checkout where you've written 1/2 of a file, you note in
20180611174818.GA8395@Sonnenschein.localdomain that there are "no
guarantees", but I've never seen a case where being out of disk space
would cause a rename to fail, since that can happen in-place.

Whereas we definitely will end up in states where we've written 1MB of a
2MB file with this when the disk fills up, and thus when that's fixed
"git status" will show local changes, so we should note that caveat for
the user.

> +	Note that the current implementation of in-place checkout makes no effort
> +	to update read/write permissions according to umask. Permissions are
> +	however modified to enable write access and to update executable
> +	permissions.

I think we should have a paragraph break there before "Note...".

>  core.abbrev::
>  	Set the length object names are abbreviated to.  If
>  	unspecified or set to "auto", an appropriate value is
> diff --git a/cache.h b/cache.h
> index 89a107a..5b8c4d6 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -815,6 +815,7 @@ extern int fsync_object_files;
>  extern int core_preload_index;
>  extern int core_commit_graph;
>  extern int core_apply_sparse_checkout;
> +extern int checkout_inplace;
>  extern int precomposed_unicode;
>  extern int protect_hfs;
>  extern int protect_ntfs;
> @@ -1518,6 +1519,7 @@ struct checkout {
>  	unsigned force:1,
>  		 quiet:1,
>  		 not_new:1,
> +		 inplace:1,
>  		 refresh_cache:1;
>  };
>  #define CHECKOUT_INIT { NULL, "" }
> diff --git a/config.c b/config.c
> index fbbf0f8..8b35ecd 100644
> --- a/config.c
> +++ b/config.c
> @@ -1318,6 +1318,11 @@ static int git_default_core_config(const char *var, const char *value)
>  		return 0;
>  	}
>
> +	if (!strcmp(var, "core.checkoutinplace")) {
> +		checkout_inplace = git_config_bool(var, value);
> +		return 0;
> +	}
> +
>  	if (!strcmp(var, "core.precomposeunicode")) {
>  		precomposed_unicode = git_config_bool(var, value);
>  		return 0;
> diff --git a/entry.c b/entry.c
> index 2101201..a599fc1 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -78,8 +78,13 @@ static void remove_subtree(struct strbuf *path)
>
>  static int create_file(const char *path, unsigned int mode)
>  {
> +	int flags;
> +	if (checkout_inplace)
> +		flags = O_WRONLY | O_CREAT | O_TRUNC;
> +	else
> +		flags = O_WRONLY | O_CREAT | O_EXCL;

I'd find this sort of thing easier to read as:

	int flags = O_WRONLY | O_CREAT;
	if (checkout_inplace)
		flags |= O_TRUNC;
	else
		flags |= O_EXCL;

Or even:

	int flags = O_WRONLY | O_CREAT;
	flags |= checkout_inplace ? O_TRUNC : O_EXCL;

I.e. less eyeballing the two lines to see if they're the same.

>  	mode = (mode & 0100) ? 0777 : 0666;
> -	return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);
> +	return open(path, flags, mode);
>  }
>
>  static void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)
> @@ -467,8 +472,15 @@ int checkout_entry(struct cache_entry *ce,
>  			if (!state->force)
>  				return error("%s is a directory", path.buf);
>  			remove_subtree(&path);
> -		} else if (unlink(path.buf))
> -			return error_errno("unable to unlink old '%s'", path.buf);
> +		} else if (checkout_inplace) {
> +			if (!(st.st_mode & 0200) ||
> +			    (trust_executable_bit && (st.st_mode & 0100) != (ce->ce_mode & 0100)))
> +				if (chmod(path.buf, (ce->ce_mode & 0100) ? 0777 : 0666))
> +					return error_errno(_("unable to change mode of '%s'"), path.buf);
> +		} else {
> +			if (unlink(path.buf))
> +				return error_errno(_("unable to unlink old '%s'"), path.buf);
> +		}
>  	} else if (state->not_new)
>  		return 0;
>
> diff --git a/environment.c b/environment.c
> index 2a6de23..5b91f30 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -68,6 +68,7 @@ char *notes_ref_name;
>  int grafts_replace_parents = 1;
>  int core_commit_graph;
>  int core_apply_sparse_checkout;
> +int checkout_inplace;
>  int merge_log_config = -1;
>  int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
>  unsigned long pack_size_limit_cfg;
> diff --git a/t/t2031-checkout-inplace.sh b/t/t2031-checkout-inplace.sh
> new file mode 100755
> index 0000000..d70ecc4
> --- /dev/null
> +++ b/t/t2031-checkout-inplace.sh
> @@ -0,0 +1,82 @@
> +#!/bin/sh
> +
> +test_description='in-place checkout'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +
> +	test_commit hello world &&
> +	git branch other &&
> +	test_commit hello-again world
> +'
> +
> +test_expect_success 'in-place checkout overwrites open file' '
> +
> +	git config core.checkoutInPlace true &&
> +	git checkout -f master &&
> +	exec 8<world &&
> +	git checkout other &&
> +	exec 8<&- &&
> +	echo hello >expect &&
> +	test_cmp expect world
> +'
> +
> +test_expect_success 'in-place checkout overwrites read-only file' '
> +
> +	git config core.checkoutInPlace true &&
> +	git checkout -f master &&
> +	chmod -w world &&
> +	git checkout other &&
> +	echo hello >expect &&
> +	test_cmp expect world
> +'
> +
> +test_expect_success 'in-place checkout updates executable permission' '
> +
> +	git config core.checkoutInPlace true &&
> +	git checkout -f master^0 &&
> +	test_chmod +x world &&
> +	git commit -m executable &&
> +	git checkout other &&
> +	test ! -x world

Worth testing switching branches here again & re-testing, since this
only tests +x -> -x, but not -x -> +x when we go back.

> +'
> +
> +test_expect_success POSIXPERM 'regular checkout respects umask' '
> +
> +	git config core.checkoutInPlace false &&
> +	git checkout -f master &&
> +	chmod 0660 world &&
> +	umask 0022 &&
> +	git checkout other &&
> +	actual=$(ls -l world) &&
> +	case "$actual" in
> +	-rw-r--r--*)
> +		: happy
> +		;;
> +	*)
> +		echo Oops, world is not 0644 but $actual
> +		false
> +		;;
> +	esac

Is that "ls" parsing portable? And also couldn't this be accomplished
with something like "stat --format"? I'm not sure how portable that is,
we just have one use of it in the test suite (on Cygwin only).

> +'
> +
> +test_expect_success POSIXPERM 'in-place checkout ignores umask' '
> +
> +	git config core.checkoutInPlace true &&
> +	git checkout -f master &&
> +	chmod 0660 world &&
> +	umask 0022 &&
> +	git checkout other &&
> +	actual=$(ls -l world) &&
> +	case "$actual" in
> +	-rw-rw----*)
> +		: happy
> +		;;
> +	*)
> +		echo Oops, world is not 0660 but $actual
> +		false
> +		;;
> +	esac
> +'
> +
> +test_done

Re: [PATCH v2] checkout files in-place

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> ...And this seems to conflict with what Junio's summarized in
> xmqqvaapb3r1.fsf@gitster-ct.c.googlers.com. I.e. (if I'm reading it
> correctly) it's not correct to say that we unlink the existing file,
> then replace it, don't we create a new one, and then rename it in-place?

No, my recollection was incorrect.  entry.c::checkout_entry() does
an unlink() then write_entry() to the final place without any
rename-to-finish phase.

Re: [PATCH] checkout files in-place

[Edward Thomson]

On Tue, Jun 12, 2018 at 09:13:54AM +0300, Orgad Shaneh wrote:
> Some of my colleagues use an ancient version of Source Insight, which also
> locks files for write.

If that application is locking files for writing (that is to say, it did
not specify the `FILE_SHARE_WRITE` bit in the sharing modes during
`CreateFile`) then this patch would not help.

Applications, generally speaking, should be locking files for write.
It's the default in Win32 and .NET's file open APIs because few
applications are prepared to detect and support a file changing out from
underneath them in the middle of a read.

> It's less important than it was before those fixes, but it is still needed
> for users of Qt Creator 4.6 (previous versions just avoided mmap, 4.7 uses
> mmap only for system headers). Other tools on Windows might as well
> misbehave.

I don't understand what mmap'ing via `CreateFileMapping` has to do with
this.  It takes an existing `HANDLE` that was opened with `CreateFile`,
which is where the sharing mode was supplied.

I would be surprised if there are other tools on Windows that have
specified `FILE_SHARE_WRITE` but not `FILE_SHARE_DELETE`.  Generally
speaking, if you don't care about another process changing a file
underneath you then you should specify both.  If you do then you should
specify neither.

I'm not saying that git shouldn't work around a bug in QT Creator -
that's not my call, though I would be loathe to support this
configuration option in libgit2.  But I am saying that it seems like
this patch doesn't have broad applicability beyond that particular tool.

-ed

Re: [PATCH] checkout files in-place

[Orgad Shaneh]

On Tue, Jun 12, 2018 at 11:51 AM Edward Thomson
<ethomson@edwardthomson.com> wrote:
>
> On Tue, Jun 12, 2018 at 09:13:54AM +0300, Orgad Shaneh wrote:
> > Some of my colleagues use an ancient version of Source Insight, which also
> > locks files for write.
>
> If that application is locking files for writing (that is to say, it did
> not specify the `FILE_SHARE_WRITE` bit in the sharing modes during
> `CreateFile`) then this patch would not help.
>
> Applications, generally speaking, should be locking files for write.
> It's the default in Win32 and .NET's file open APIs because few
> applications are prepared to detect and support a file changing out from
> underneath them in the middle of a read.

I agree.

> > It's less important than it was before those fixes, but it is still needed
> > for users of Qt Creator 4.6 (previous versions just avoided mmap, 4.7 uses
> > mmap only for system headers). Other tools on Windows might as well
> > misbehave.
>
> I don't understand what mmap'ing via `CreateFileMapping` has to do with
> this.  It takes an existing `HANDLE` that was opened with `CreateFile`,
> which is where the sharing mode was supplied.

I'm not completely sure. The file is opened using CreateFile[1] with
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE.
Then this handle is passed to CreateFileMapping[2]. For a reason I don't
understand, when mapping is used, the handle is never released (until
the file is closed), but when it is not used, the file is being read, then the
handle is released.

Maybe Ivan or Nikolai can shed some light on this process.

Anyway, with Qt Creator 4.7 this should be a non-issue, so I'm reluctant about
this change here.

> I would be surprised if there are other tools on Windows that have
> specified `FILE_SHARE_WRITE` but not `FILE_SHARE_DELETE`.  Generally
> speaking, if you don't care about another process changing a file
> underneath you then you should specify both.  If you do then you should
> specify neither.

The problem is that even if you specify FILE_SHARE_WRITE and FILE_SHARE_DELETE,
the file can be unlinked, but it cannot be created with the same name
until its handle
is closed, unless you rename it *before* unlinking.

- Orgad

[1] https://github.com/llvm-mirror/llvm/blob/371257e/lib/Support/Windows/Path.inc#L1045
[2] https://github.com/llvm-mirror/llvm/blob/371257e/lib/Support/Windows/Path.inc#L836