threads / patch / 48687

patchcheckout files in-place

Subject: [PATCH] checkout files in-place

## tl;dr

13 messages between Jun 10, 2018 and Jun 13, 2018. Diffs are folded; open one to read it.

replies: 12people: 6as markdown or json

Clemens Buchacher· Jun 10, 2018, 19:44 UTC · lore

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
Show changes to 6 files +72 −3

Documentation/config.txt, cache.h, config.c, entry.c, environment.c, 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
brian m. carlson· Jun 11, 2018, 02:04 UTC · re: Clemens Buchacher · lore

Re: [PATCH] checkout files in-place

On Sun, Jun 10, 2018 at 09:44:45PM +0200, Clemens Buchacher wrote:
Show 9 quoted lines
> 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.

Show 12 quoted lines
>  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
Clemens Buchacher· Jun 11, 2018, 17:48 UTC · re: brian m. carlson · lore

Re: [PATCH] checkout files in-place

On Mon, Jun 11, 2018 at 02:04:11AM +0000, brian m. carlson wrote:
Show 6 quoted lines
> 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.

Junio C Hamano· Jun 11, 2018, 18:02 UTC · re: brian m. carlson · lore

Re: [PATCH] checkout files in-place

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 5 quoted lines
>> +	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.

Clemens Buchacher· Jun 11, 2018, 20:22 UTC · re: Junio C Hamano · lore

Re: [PATCH] checkout files in-place

On Mon, Jun 11, 2018 at 11:02:42AM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> 
> 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.

Ævar Arnfjörð Bjarmason· Jun 11, 2018, 17:59 UTC · re: Clemens Buchacher · lore

Re: [PATCH] checkout files in-place

On Sun, Jun 10 2018, Clemens Buchacher wrote:
Show 16 quoted lines
> 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.

Show 7 quoted lines
> 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...
Show 8 quoted lines
> +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.

Show 110 quoted lines
>  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.
Show 24 quoted lines
> +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.

Edward Thomson· Jun 11, 2018, 20:35 UTC · re: Clemens Buchacher · lore

Re: [PATCH] checkout files in-place

On Sun, Jun 10, 2018 at 09:44:45PM +0200, Clemens Buchacher wrote:
Show 5 quoted lines
> 
> 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

Clemens Buchacher· Jun 11, 2018, 20:57 UTC · re: Edward Thomson · lore

Re: [PATCH] checkout files in-place

+Cc: Orgad Shaneh
On Mon, Jun 11, 2018 at 08:35:41PM +0000, Edward Thomson wrote:
Show 30 quoted lines
> 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
Clemens Buchacher· Jun 11, 2018, 20:39 UTC · re: Clemens Buchacher · lore

[PATCH v2] checkout files in-place

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
Show changes to 6 files +116 −3

Documentation/config.txt, cache.h, config.c, entry.c, environment.c, 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
Ævar Arnfjörð Bjarmason· Jun 11, 2018, 21:38 UTC · re: Clemens Buchacher · lore

Re: [PATCH v2] checkout files in-place

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...
Show 52 quoted lines
> 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...".
Show 52 quoted lines
>  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.
Show 83 quoted lines
>  	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.

Show 19 quoted lines
> +'
> +
> +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).

Show 22 quoted lines
> +'
> +
> +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
Junio C Hamano· Jun 11, 2018, 22:46 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2] checkout files in-place

Æ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.

Edward Thomson· Jun 12, 2018, 08:51 UTC · lore

Re: [PATCH] checkout files in-place

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
Orgad Shaneh· Jun 13, 2018, 07:39 UTC · re: Edward Thomson · lore

Re: [PATCH] checkout files in-place

On Tue, Jun 12, 2018 at 11:51 AM Edward Thomson <ethomson@edwardthomson.com> wrote:

Show 13 quoted lines
>
> 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.
Show 8 quoted lines
> > 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.

Show 5 quoted lines
> 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

← back to recent threads