{"thread":{"id":"48687","subject":"[PATCH] checkout files in-place","startedAt":"2018-06-10T19:44:48Z","lastAt":"2018-06-13T07:39:32Z","messageCount":13,"participants":["Clemens Buchacher","brian m. carlson","Ævar Arnfjörð Bjarmason","Junio C Hamano","Edward Thomson","Orgad Shaneh"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"349867","messageId":"20180610194444.GA1913@Sonnenschein.localdomain","threadId":"48687","inReplyTo":null,"subject":"[PATCH] checkout files in-place","fromName":"Clemens Buchacher","fromEmail":"drizzd@gmx.net","sentAt":"2018-06-10T19:44:45Z","receivedAt":"2018-06-10T19:44:48Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"When replacing files with new content during checkout, we do not write\nto them in-place. Instead we unlink and re-create the files in order to\nlet the system figure out ownership and permissions for the new file,\ntaking umask into account.\n\nIt is safe to do this on Linux file systems, even if open file handles\nstill exist, because unlink only removes the directory reference to the\nfile. On Windows, however, a file cannot be deleted until all handles to\nit are closed. If a file cannot be deleted, its name cannot be reused.\n\nThis causes files to be deleted, but not checked out when switching\nbranches. This is frequently an issue with Qt Creator, which\ncontinuously opens files in the work tree, as reported here:\nhttps://github.com/git-for-windows/git/issues/1653\n\nThis change adds the core.checkout_inplace option. If enabled, checkout\nwill open files for writing the new content in-place. This fixes the\nissue, but with this approach the system will not update file\npermissions according to umask. Only essential updates of write and\nexecutable permissions are performed.\n\nThe in-place checkout is therefore optional. It could be enabled by Git\ninstallers on Windows, where umask is irrelevant.\n\nSigned-off-by: Clemens Buchacher <drizzd@gmx.net>\n---\n\nI wonder if Git should be responsible for updating ownership and file\npermissions when modifying existing files during checkout. We could\notherwise remove the unlink completely. Maybe this could even improve\nperformance in some cases. It made no difference in a short test on\nWindows.\n\nRegression tests are running. This will take a while.\n\n Documentation/config.txt    |  8 ++++++++\n cache.h                     |  2 ++\n config.c                    |  5 +++++\n entry.c                     | 18 +++++++++++++++---\n environment.c               |  1 +\n t/t2031-checkout-inplace.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 72 insertions(+), 3 deletions(-)\n create mode 100755 t/t2031-checkout-inplace.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex b6cb997164..17af0fe163 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -923,6 +923,14 @@ core.sparseCheckout::\n \tEnable \"sparse checkout\" feature. See section \"Sparse checkout\" in\n \tlinkgit:git-read-tree[1] for more information.\n \n+core.checkoutInplace::\n+\tCheckout file contents in-place. By default Git checkout removes existing\n+\twork tree files before it replaces them with different contents. If this\n+\toption is enabled Git will overwrite the contents of existing files\n+\tin-place. This is useful on systems where open file handles to a removed\n+\tfile prevent creating new files at the same path. Note that Git will not\n+\tupdate read/write permissions according to umask.\n+\n core.abbrev::\n \tSet the length object names are abbreviated to.  If\n \tunspecified or set to \"auto\", an appropriate value is\ndiff --git a/cache.h b/cache.h\nindex 2c640d4c31..c8fccd2a80 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -808,6 +808,7 @@ extern char *git_replace_ref_base;\n extern int fsync_object_files;\n extern int core_preload_index;\n extern int core_apply_sparse_checkout;\n+extern int checkout_inplace;\n extern int precomposed_unicode;\n extern int protect_hfs;\n extern int protect_ntfs;\n@@ -1530,6 +1531,7 @@ struct checkout {\n \tunsigned force:1,\n \t\t quiet:1,\n \t\t not_new:1,\n+\t\t inplace:1,\n \t\t refresh_cache:1;\n };\n #define CHECKOUT_INIT { NULL, \"\" }\ndiff --git a/config.c b/config.c\nindex cd2b404b14..4ac2407057 100644\n--- a/config.c\n+++ b/config.c\n@@ -1231,6 +1231,11 @@ static int git_default_core_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.checkoutinplace\")) {\n+\t\tcheckout_inplace = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"core.precomposeunicode\")) {\n \t\tprecomposed_unicode = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/entry.c b/entry.c\nindex 31c00816dc..54c98870b9 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -78,8 +78,13 @@ static void remove_subtree(struct strbuf *path)\n \n static int create_file(const char *path, unsigned int mode)\n {\n+\tint flags;\n+\tif (checkout_inplace)\n+\t\tflags = O_WRONLY | O_CREAT | O_TRUNC;\n+\telse\n+\t\tflags = O_WRONLY | O_CREAT | O_EXCL;\n \tmode = (mode & 0100) ? 0777 : 0666;\n-\treturn open(path, O_WRONLY | O_CREAT | O_EXCL, mode);\n+\treturn open(path, flags, mode);\n }\n \n static void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)\n@@ -470,8 +475,15 @@ int checkout_entry(struct cache_entry *ce,\n \t\t\tif (!state->force)\n \t\t\t\treturn error(\"%s is a directory\", path.buf);\n \t\t\tremove_subtree(&path);\n-\t\t} else if (unlink(path.buf))\n-\t\t\treturn error_errno(\"unable to unlink old '%s'\", path.buf);\n+\t\t} else if (checkout_inplace) {\n+\t\t\tif (!(st.st_mode & 0200) ||\n+\t\t\t    (trust_executable_bit && (st.st_mode & 0100) != (ce->ce_mode & 0100)))\n+\t\t\t\tif (chmod(path.buf, (ce->ce_mode & 0100) ? 0777 : 0666))\n+\t\t\t\t\treturn error_errno(\"unable to change mode of '%s'\", path.buf);\n+\t\t} else {\n+\t\t\tif (unlink(path.buf))\n+\t\t\t\treturn error_errno(\"unable to unlink old '%s'\", path.buf);\n+\t\t}\n \t} else if (state->not_new)\n \t\treturn 0;\n \ndiff --git a/environment.c b/environment.c\nindex d1ac37dd18..6a8036b144 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -63,6 +63,7 @@ enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;\n char *notes_ref_name;\n int grafts_replace_parents = 1;\n int core_apply_sparse_checkout;\n+int checkout_inplace;\n int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n unsigned long pack_size_limit_cfg;\ndiff --git a/t/t2031-checkout-inplace.sh b/t/t2031-checkout-inplace.sh\nnew file mode 100755\nindex 0000000000..60ea30cbf5\n--- /dev/null\n+++ b/t/t2031-checkout-inplace.sh\n@@ -0,0 +1,41 @@\n+#!/bin/sh\n+\n+test_description='checkout inplace'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\n+\tgit config core.checkoutInplace true &&\n+\techo hello >world &&\n+\tgit add world &&\n+\tgit commit -m initial &&\n+\tgit branch other &&\n+\techo \"hello again\" >>world &&\n+\tgit add world &&\n+\tgit commit -m second\n+'\n+\n+test_expect_success 'checkout overwrites open file' '\n+\n+\tgit checkout -f master &&\n+\tmkfifo input &&\n+\t{\n+\t\tcat >>world <input &\n+\t} &&\n+\tpid=$! &&\n+\ttest_when_finished \"kill -KILL $pid; wait $pid; rm -f input\" &&\n+\tgit checkout other &&\n+\techo hello >expect &&\n+\ttest_cmp expect world\n+'\n+\n+test_expect_success 'checkout overwrites read-only file' '\n+\n+\tgit checkout -f master &&\n+\tchmod -w world &&\n+\tgit checkout other &&\n+\techo hello >expect &&\n+\ttest_cmp expect world\n+'\n+\n+test_done\n-- \n2.16.1.windows.1\n\n"},{"id":"349869","messageId":"20180611020411.GE38834@genre.crustytoothpaste.net","threadId":"48687","inReplyTo":"20180610194444.GA1913@Sonnenschein.localdomain","subject":"Re: [PATCH] checkout files in-place","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-06-11T02:04:11Z","receivedAt":"2018-06-11T02:04:21Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Jun 10, 2018 at 09:44:45PM +0200, Clemens Buchacher wrote:\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index b6cb997164..17af0fe163 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -923,6 +923,14 @@ core.sparseCheckout::\n>  \tEnable \"sparse checkout\" feature. See section \"Sparse checkout\" in\n>  \tlinkgit:git-read-tree[1] for more information.\n>  \n> +core.checkoutInplace::\n\nPerhaps \"core.checkoutInPlace\" (captialized \"place\")?\n\n> +\tCheckout file contents in-place. By default Git checkout removes existing\n\n\"Check out\".\n\n> +\twork tree files before it replaces them with different contents. If this\n> +\toption is enabled Git will overwrite the contents of existing files\n> +\tin-place. This is useful on systems where open file handles to a removed\n\nHere and above, uou don't need to hyphenate \"in place\" when used as an\nadverb, only when using it as an adjective before the noun (e.g.\n\"in-place checkout\").\n\n> +\tfile prevent creating new files at the same path. Note that Git will not\n> +\tupdate read/write permissions according to umask.\n\nI'm wondering if it's worth a mention that running out of disk space (or\nquota) will cause data to be truncated.\n\n>  static void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)\n> @@ -470,8 +475,15 @@ int checkout_entry(struct cache_entry *ce,\n>  \t\t\tif (!state->force)\n>  \t\t\t\treturn error(\"%s is a directory\", path.buf);\n>  \t\t\tremove_subtree(&path);\n> -\t\t} else if (unlink(path.buf))\n> -\t\t\treturn error_errno(\"unable to unlink old '%s'\", path.buf);\n> +\t\t} else if (checkout_inplace) {\n> +\t\t\tif (!(st.st_mode & 0200) ||\n> +\t\t\t    (trust_executable_bit && (st.st_mode & 0100) != (ce->ce_mode & 0100)))\n> +\t\t\t\tif (chmod(path.buf, (ce->ce_mode & 0100) ? 0777 : 0666))\n> +\t\t\t\t\treturn error_errno(\"unable to change mode of '%s'\", path.buf);\n\nSo in-place checkout won't work if the mode changes and we're not the\nowner of the file.  One place where I could see people wanting to use\nthis on Unix is shared repositories with BSD group semantics, but that\nwouldn't work reliably.\n\nI don't see that as a problem, as that isn't the issue this patch is\ntrying to solve, but it may end up biting people.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"349920","messageId":"20180611174818.GA8395@Sonnenschein.localdomain","threadId":"48687","inReplyTo":"20180611020411.GE38834@genre.crustytoothpaste.net","subject":"Re: [PATCH] checkout files in-place","fromName":"Clemens Buchacher","fromEmail":"drizzd@gmx.net","sentAt":"2018-06-11T17:48:18Z","receivedAt":"2018-06-11T17:48:30Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Mon, Jun 11, 2018 at 02:04:11AM +0000, brian m. carlson wrote:\n> On Sun, Jun 10, 2018 at 09:44:45PM +0200, Clemens Buchacher wrote:\n> > +\tfile prevent creating new files at the same path. Note that Git will not\n> > +\tupdate read/write permissions according to umask.\n> \n> I'm wondering if it's worth a mention that running out of disk space (or\n> quota) will cause data to be truncated.\n\nAs far as I know we make no guarantees about the behavior when running\nout of disk space. There could be other side effects, so I cannot safely\nstate anything here.\n"},{"id":"349922","messageId":"87d0wxw6f9.fsf@evledraar.gmail.com","threadId":"48687","inReplyTo":"20180610194444.GA1913@Sonnenschein.localdomain","subject":"Re: [PATCH] checkout files in-place","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-11T17:59:22Z","receivedAt":"2018-06-11T17:59:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Jun 10 2018, Clemens Buchacher wrote:\n\n> When replacing files with new content during checkout, we do not write\n> to them in-place. Instead we unlink and re-create the files in order to\n> let the system figure out ownership and permissions for the new file,\n> taking umask into account.\n>\n> It is safe to do this on Linux file systems, even if open file handles\n> still exist, because unlink only removes the directory reference to the\n> file. On Windows, however, a file cannot be deleted until all handles to\n> it are closed. If a file cannot be deleted, its name cannot be reused.\n>\n> This causes files to be deleted, but not checked out when switching\n> branches. This is frequently an issue with Qt Creator, which\n> continuously opens files in the work tree, as reported here:\n> https://github.com/git-for-windows/git/issues/1653\n>\n> This change adds the core.checkout_inplace option. If enabled, checkout\n\nThe commit message should mention core.checkoutInPlace, not\ncore.checkout_inplace.\n\n> will open files for writing the new content in-place. This fixes the\n> issue, but with this approach the system will not update file\n> permissions according to umask. Only essential updates of write and\n> executable permissions are performed.\n>\n> The in-place checkout is therefore optional. It could be enabled by Git\n> installers on Windows, where umask is irrelevant.\n\nI think some of this...\n\n> +core.checkoutInplace::\n> +\tCheckout file contents in-place. By default Git checkout removes existing\n> +\twork tree files before it replaces them with different contents. If this\n> +\toption is enabled Git will overwrite the contents of existing files\n> +\tin-place. This is useful on systems where open file handles to a removed\n> +\tfile prevent creating new files at the same path. Note that Git will not\n> +\tupdate read/write permissions according to umask.\n> +\n\n...should be added to these docs. In particular let's not be coy and say\n\"on systems where\", and instead describe that this is meant for Windows,\nso users looking for that in the config man-page will see it.\n\nWe should also document the \"doesn't update permissions\" bit, and be\nclear whether that's expected future behavior we aim to preserve, or\njust a side-effect of the current implementation and may change.\n\n>  core.abbrev::\n>  \tSet the length object names are abbreviated to.  If\n>  \tunspecified or set to \"auto\", an appropriate value is\n> diff --git a/cache.h b/cache.h\n> index 2c640d4c31..c8fccd2a80 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -808,6 +808,7 @@ extern char *git_replace_ref_base;\n>  extern int fsync_object_files;\n>  extern int core_preload_index;\n>  extern int core_apply_sparse_checkout;\n> +extern int checkout_inplace;\n>  extern int precomposed_unicode;\n>  extern int protect_hfs;\n>  extern int protect_ntfs;\n> @@ -1530,6 +1531,7 @@ struct checkout {\n>  \tunsigned force:1,\n>  \t\t quiet:1,\n>  \t\t not_new:1,\n> +\t\t inplace:1,\n>  \t\t refresh_cache:1;\n>  };\n>  #define CHECKOUT_INIT { NULL, \"\" }\n> diff --git a/config.c b/config.c\n> index cd2b404b14..4ac2407057 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1231,6 +1231,11 @@ static int git_default_core_config(const char *var, const char *value, void *cb)\n>  \t\treturn 0;\n>  \t}\n>\n> +\tif (!strcmp(var, \"core.checkoutinplace\")) {\n> +\t\tcheckout_inplace = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n> +\n>  \tif (!strcmp(var, \"core.precomposeunicode\")) {\n>  \t\tprecomposed_unicode = git_config_bool(var, value);\n>  \t\treturn 0;\n> diff --git a/entry.c b/entry.c\n> index 31c00816dc..54c98870b9 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -78,8 +78,13 @@ static void remove_subtree(struct strbuf *path)\n>\n>  static int create_file(const char *path, unsigned int mode)\n>  {\n> +\tint flags;\n> +\tif (checkout_inplace)\n> +\t\tflags = O_WRONLY | O_CREAT | O_TRUNC;\n> +\telse\n> +\t\tflags = O_WRONLY | O_CREAT | O_EXCL;\n>  \tmode = (mode & 0100) ? 0777 : 0666;\n> -\treturn open(path, O_WRONLY | O_CREAT | O_EXCL, mode);\n> +\treturn open(path, flags, mode);\n>  }\n>\n>  static void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)\n> @@ -470,8 +475,15 @@ int checkout_entry(struct cache_entry *ce,\n>  \t\t\tif (!state->force)\n>  \t\t\t\treturn error(\"%s is a directory\", path.buf);\n>  \t\t\tremove_subtree(&path);\n> -\t\t} else if (unlink(path.buf))\n> -\t\t\treturn error_errno(\"unable to unlink old '%s'\", path.buf);\n> +\t\t} else if (checkout_inplace) {\n> +\t\t\tif (!(st.st_mode & 0200) ||\n> +\t\t\t    (trust_executable_bit && (st.st_mode & 0100) != (ce->ce_mode & 0100)))\n> +\t\t\t\tif (chmod(path.buf, (ce->ce_mode & 0100) ? 0777 : 0666))\n> +\t\t\t\t\treturn error_errno(\"unable to change mode of '%s'\", path.buf);\n> +\t\t} else {\n> +\t\t\tif (unlink(path.buf))\n> +\t\t\t\treturn error_errno(\"unable to unlink old '%s'\", path.buf);\n> +\t\t}\n>  \t} else if (state->not_new)\n>  \t\treturn 0;\n>\n> diff --git a/environment.c b/environment.c\n> index d1ac37dd18..6a8036b144 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -63,6 +63,7 @@ enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;\n>  char *notes_ref_name;\n>  int grafts_replace_parents = 1;\n>  int core_apply_sparse_checkout;\n> +int checkout_inplace;\n>  int merge_log_config = -1;\n>  int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n>  unsigned long pack_size_limit_cfg;\n> diff --git a/t/t2031-checkout-inplace.sh b/t/t2031-checkout-inplace.sh\n> new file mode 100755\n> index 0000000000..60ea30cbf5\n> --- /dev/null\n> +++ b/t/t2031-checkout-inplace.sh\n> @@ -0,0 +1,41 @@\n> +#!/bin/sh\n> +\n> +test_description='checkout inplace'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +\n> +\tgit config core.checkoutInplace true &&\n> +\techo hello >world &&\n> +\tgit add world &&\n> +\tgit commit -m initial &&\n> +\tgit branch other &&\n> +\techo \"hello again\" >>world &&\n> +\tgit add world &&\n> +\tgit commit -m second\n> +'\n\nWould be easier to read if you used the \"test_commit\" helper.\n\n> +test_expect_success 'checkout overwrites open file' '\n> +\n> +\tgit checkout -f master &&\n> +\tmkfifo input &&\n> +\t{\n> +\t\tcat >>world <input &\n> +\t} &&\n> +\tpid=$! &&\n> +\ttest_when_finished \"kill -KILL $pid; wait $pid; rm -f input\" &&\n> +\tgit checkout other &&\n> +\techo hello >expect &&\n> +\ttest_cmp expect world\n> +'\n> +\n> +test_expect_success 'checkout overwrites read-only file' '\n> +\n> +\tgit checkout -f master &&\n> +\tchmod -w world &&\n> +\tgit checkout other &&\n> +\techo hello >expect &&\n> +\ttest_cmp expect world\n> +'\n> +\n> +test_done\n\nThere seem to be no tests here for the chmod +x case you implemented,\nand it would be worthwhile to have an explicit test where we change the\numask and observe that a file's permissions will change without this\nsetting, but not with it.\n"},{"id":"349923","messageId":"xmqqvaapb3r1.fsf@gitster-ct.c.googlers.com","threadId":"48687","inReplyTo":"20180611020411.GE38834@genre.crustytoothpaste.net","subject":"Re: [PATCH] checkout files in-place","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-11T18:02:42Z","receivedAt":"2018-06-11T18:02:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n>> +\tfile prevent creating new files at the same path. Note that Git will not\n>> +\tupdate read/write permissions according to umask.\n>\n> I'm wondering if it's worth a mention that running out of disk space (or\n> quota) will cause data to be truncated.\n\nAside from us not having to worry about emulating the umask, another\nreason why we prefer \"create, complete the write, and then finally\nrename\" over \"overwrite and let it fail in the middle\" is that the\nformer makes sure we never leave the path in a bad state.  It either\nhas a complete copy of the old contents, or a complete copy of the\nnew contents, and a third-party process reading from sidelines would\nnot get a partial copy, regardless of disc-full issue.\n\n"},{"id":"349937","messageId":"20180611202247.GA1236@Sonnenschein.localdomain","threadId":"48687","inReplyTo":"xmqqvaapb3r1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] checkout files in-place","fromName":"Clemens Buchacher","fromEmail":"drizzd@gmx.net","sentAt":"2018-06-11T20:22:47Z","receivedAt":"2018-06-11T20:23:00Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Mon, Jun 11, 2018 at 11:02:42AM -0700, Junio C Hamano wrote:\n> \n> Aside from us not having to worry about emulating the umask, another\n> reason why we prefer \"create, complete the write, and then finally\n> rename\" over \"overwrite and let it fail in the middle\" is that the\n> former makes sure we never leave the path in a bad state.\n\nBut the current checkout implementation does not do this. It writes\ndirectly to the target location. The only difference to in-place\ncheckout is that files are unlinked before they are opened for writing.\n"},{"id":"349938","messageId":"20180611203541.GA6@606faba9ba17","threadId":"48687","inReplyTo":"20180610194444.GA1913@Sonnenschein.localdomain","subject":"Re: [PATCH] checkout files in-place","fromName":"Edward Thomson","fromEmail":"ethomson@edwardthomson.com","sentAt":"2018-06-11T20:35:41Z","receivedAt":"2018-06-11T20:35:55Z","isPatch":true,"sender":{"key":"ethomson@edwardthomson.com","avatar":"https://avatars.githubusercontent.com/u/1130014?v=4"},"body":"On Sun, Jun 10, 2018 at 09:44:45PM +0200, Clemens Buchacher wrote:\n> \n> It is safe to do this on Linux file systems, even if open file handles\n> still exist, because unlink only removes the directory reference to the\n> file. On Windows, however, a file cannot be deleted until all handles to\n> it are closed. If a file cannot be deleted, its name cannot be reused.\n\nI'm nervous about this proposed change, since it feels like it's\naddressing an issue that only exists in QT Creator.\n\nYou've accurately described the default semantics in Win32.  A file\ncannot be deleted until all handles to it are closed, unless it was\nopened with `FILE_SHARE_DELETE` as their sharing mode.  This is not the\ndefault sharing mode in either Win32 or .NET.\n\nHowever, for your patch to have an effect, all processes with a handle\nopen must have specified `FILE_SHARE_WRITE`.  This is rather uncommon,\nsince it's also not included in the default Win32 or .NET sharing mode.\nThis is because it's uncommon that you would want other processes to\nchange the data underneath you in between ReadFile() calls.\n\nSo your patch will benefit people who have processes that have\n`FILE_SHARE_WRITE` set but not `FILE_SHARE_DELETE` set, which I think is\ngenerally an uncommon scenario to want to support.\n\nGenerally if you're willing to accept files changing underneath you,\nthen you probably want to allow them to be deleted, too.  So this feels\nlike something that's very specific to QT Creator.  Or are there other\nIDEs or development tools that use these open semantics that I'm not\naware of?\n\nCheers-\n-ed\n"},{"id":"349939","messageId":"20180611203958.GA1306@Sonnenschein.localdomain","threadId":"48687","inReplyTo":"20180610194444.GA1913@Sonnenschein.localdomain","subject":"[PATCH v2] checkout files in-place","fromName":"Clemens Buchacher","fromEmail":"drizzd@gmx.net","sentAt":"2018-06-11T20:39:58Z","receivedAt":"2018-06-11T20:40:15Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"When replacing files with new content during checkout, we do not write\nto them in place. Instead we unlink and recreate the files in order to\nlet the system figure out ownership and permissions for the new file,\ntaking umask into account.\n\nIt is safe to do this on Linux file systems, even if open file handles\nstill exist, because unlink only removes the directory reference to the\nfile. On Windows, however, a file cannot be deleted until all handles to\nit are closed. If a file cannot be deleted, its name cannot be reused.\n\nThis causes files to be deleted, but not checked out when switching\nbranches. This is frequently an issue with Qt Creator, which\ncontinuously opens files in the work tree, as reported here:\nhttps://github.com/git-for-windows/git/issues/1653\n\nThis change adds the core.checkoutInPlace option. If enabled, checkout\nwill open files for writing the new content in place. This fixes the\nissue, but with this approach the system will not update file\npermissions according to umask. Only essential updates of write and\nexecutable permissions are performed.\n\nThe in-place checkout is therefore optional. It could be enabled by Git\ninstallers on Windows, where umask is irrelevant.\n\nSigned-off-by: Clemens Buchacher <drizzd@gmx.net>\nReviewed-by: Orgad Shaneh <orgads@gmail.com>\nReviewed-by: \"brian m. carlson\" <sandals@crustytoothpaste.net>\nReviewed-by: Duy Nguyen <pclouds@gmail.com>\nReviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nTested on Windows with Git-for-Windows and with Windows Subsystem for\nLinux.\n\n Documentation/config.txt    | 11 ++++++\n cache.h                     |  2 ++\n config.c                    |  5 +++\n entry.c                     | 18 ++++++++--\n environment.c               |  1 +\n t/t2031-checkout-inplace.sh | 82 +++++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 116 insertions(+), 3 deletions(-)\n create mode 100755 t/t2031-checkout-inplace.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex ab641bf..0860a81 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -912,6 +912,17 @@ core.sparseCheckout::\n \tEnable \"sparse checkout\" feature. See section \"Sparse checkout\" in\n \tlinkgit:git-read-tree[1] for more information.\n \n+core.checkoutInPlace::\n+\tCheck out file contents in place. By default Git checkout removes existing\n+\twork tree files before it replaces them with different content. If this\n+\toption is enabled, Git will overwrite the contents of existing files in\n+\tplace. This is useful on Windows, where open file handles to a removed file\n+\tprevent creating new files at the same path.\n+\tNote that the current implementation of in-place checkout makes no effort\n+\tto update read/write permissions according to umask. Permissions are\n+\thowever modified to enable write access and to update executable\n+\tpermissions.\n+\n core.abbrev::\n \tSet the length object names are abbreviated to.  If\n \tunspecified or set to \"auto\", an appropriate value is\ndiff --git a/cache.h b/cache.h\nindex 89a107a..5b8c4d6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -815,6 +815,7 @@ extern int fsync_object_files;\n extern int core_preload_index;\n extern int core_commit_graph;\n extern int core_apply_sparse_checkout;\n+extern int checkout_inplace;\n extern int precomposed_unicode;\n extern int protect_hfs;\n extern int protect_ntfs;\n@@ -1518,6 +1519,7 @@ struct checkout {\n \tunsigned force:1,\n \t\t quiet:1,\n \t\t not_new:1,\n+\t\t inplace:1,\n \t\t refresh_cache:1;\n };\n #define CHECKOUT_INIT { NULL, \"\" }\ndiff --git a/config.c b/config.c\nindex fbbf0f8..8b35ecd 100644\n--- a/config.c\n+++ b/config.c\n@@ -1318,6 +1318,11 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.checkoutinplace\")) {\n+\t\tcheckout_inplace = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"core.precomposeunicode\")) {\n \t\tprecomposed_unicode = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/entry.c b/entry.c\nindex 2101201..a599fc1 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -78,8 +78,13 @@ static void remove_subtree(struct strbuf *path)\n \n static int create_file(const char *path, unsigned int mode)\n {\n+\tint flags;\n+\tif (checkout_inplace)\n+\t\tflags = O_WRONLY | O_CREAT | O_TRUNC;\n+\telse\n+\t\tflags = O_WRONLY | O_CREAT | O_EXCL;\n \tmode = (mode & 0100) ? 0777 : 0666;\n-\treturn open(path, O_WRONLY | O_CREAT | O_EXCL, mode);\n+\treturn open(path, flags, mode);\n }\n \n static void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)\n@@ -467,8 +472,15 @@ int checkout_entry(struct cache_entry *ce,\n \t\t\tif (!state->force)\n \t\t\t\treturn error(\"%s is a directory\", path.buf);\n \t\t\tremove_subtree(&path);\n-\t\t} else if (unlink(path.buf))\n-\t\t\treturn error_errno(\"unable to unlink old '%s'\", path.buf);\n+\t\t} else if (checkout_inplace) {\n+\t\t\tif (!(st.st_mode & 0200) ||\n+\t\t\t    (trust_executable_bit && (st.st_mode & 0100) != (ce->ce_mode & 0100)))\n+\t\t\t\tif (chmod(path.buf, (ce->ce_mode & 0100) ? 0777 : 0666))\n+\t\t\t\t\treturn error_errno(_(\"unable to change mode of '%s'\"), path.buf);\n+\t\t} else {\n+\t\t\tif (unlink(path.buf))\n+\t\t\t\treturn error_errno(_(\"unable to unlink old '%s'\"), path.buf);\n+\t\t}\n \t} else if (state->not_new)\n \t\treturn 0;\n \ndiff --git a/environment.c b/environment.c\nindex 2a6de23..5b91f30 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -68,6 +68,7 @@ char *notes_ref_name;\n int grafts_replace_parents = 1;\n int core_commit_graph;\n int core_apply_sparse_checkout;\n+int checkout_inplace;\n int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n unsigned long pack_size_limit_cfg;\ndiff --git a/t/t2031-checkout-inplace.sh b/t/t2031-checkout-inplace.sh\nnew file mode 100755\nindex 0000000..d70ecc4\n--- /dev/null\n+++ b/t/t2031-checkout-inplace.sh\n@@ -0,0 +1,82 @@\n+#!/bin/sh\n+\n+test_description='in-place checkout'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\n+\ttest_commit hello world &&\n+\tgit branch other &&\n+\ttest_commit hello-again world\n+'\n+\n+test_expect_success 'in-place checkout overwrites open file' '\n+\n+\tgit config core.checkoutInPlace true &&\n+\tgit checkout -f master &&\n+\texec 8<world &&\n+\tgit checkout other &&\n+\texec 8<&- &&\n+\techo hello >expect &&\n+\ttest_cmp expect world\n+'\n+\n+test_expect_success 'in-place checkout overwrites read-only file' '\n+\n+\tgit config core.checkoutInPlace true &&\n+\tgit checkout -f master &&\n+\tchmod -w world &&\n+\tgit checkout other &&\n+\techo hello >expect &&\n+\ttest_cmp expect world\n+'\n+\n+test_expect_success 'in-place checkout updates executable permission' '\n+\n+\tgit config core.checkoutInPlace true &&\n+\tgit checkout -f master^0 &&\n+\ttest_chmod +x world &&\n+\tgit commit -m executable &&\n+\tgit checkout other &&\n+\ttest ! -x world\n+'\n+\n+test_expect_success POSIXPERM 'regular checkout respects umask' '\n+\n+\tgit config core.checkoutInPlace false &&\n+\tgit checkout -f master &&\n+\tchmod 0660 world &&\n+\tumask 0022 &&\n+\tgit checkout other &&\n+\tactual=$(ls -l world) &&\n+\tcase \"$actual\" in\n+\t-rw-r--r--*)\n+\t\t: happy\n+\t\t;;\n+\t*)\n+\t\techo Oops, world is not 0644 but $actual\n+\t\tfalse\n+\t\t;;\n+\tesac\n+'\n+\n+test_expect_success POSIXPERM 'in-place checkout ignores umask' '\n+\n+\tgit config core.checkoutInPlace true &&\n+\tgit checkout -f master &&\n+\tchmod 0660 world &&\n+\tumask 0022 &&\n+\tgit checkout other &&\n+\tactual=$(ls -l world) &&\n+\tcase \"$actual\" in\n+\t-rw-rw----*)\n+\t\t: happy\n+\t\t;;\n+\t*)\n+\t\techo Oops, world is not 0660 but $actual\n+\t\tfalse\n+\t\t;;\n+\tesac\n+'\n+\n+test_done\n-- \n2.7.4\n"},{"id":"349940","messageId":"20180611205704.GA1399@Sonnenschein.localdomain","threadId":"48687","inReplyTo":"20180611203541.GA6@606faba9ba17","subject":"Re: [PATCH] checkout files in-place","fromName":"Clemens Buchacher","fromEmail":"drizzd@gmx.net","sentAt":"2018-06-11T20:57:04Z","receivedAt":"2018-06-11T20:57:11Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"+Cc: Orgad Shaneh\n\nOn Mon, Jun 11, 2018 at 08:35:41PM +0000, Edward Thomson wrote:\n> On Sun, Jun 10, 2018 at 09:44:45PM +0200, Clemens Buchacher wrote:\n> > \n> > It is safe to do this on Linux file systems, even if open file handles\n> > still exist, because unlink only removes the directory reference to the\n> > file. On Windows, however, a file cannot be deleted until all handles to\n> > it are closed. If a file cannot be deleted, its name cannot be reused.\n> \n> I'm nervous about this proposed change, since it feels like it's\n> addressing an issue that only exists in QT Creator.\n> \n> You've accurately described the default semantics in Win32.  A file\n> cannot be deleted until all handles to it are closed, unless it was\n> opened with `FILE_SHARE_DELETE` as their sharing mode.  This is not the\n> default sharing mode in either Win32 or .NET.\n> \n> However, for your patch to have an effect, all processes with a handle\n> open must have specified `FILE_SHARE_WRITE`.  This is rather uncommon,\n> since it's also not included in the default Win32 or .NET sharing mode.\n> This is because it's uncommon that you would want other processes to\n> change the data underneath you in between ReadFile() calls.\n> \n> So your patch will benefit people who have processes that have\n> `FILE_SHARE_WRITE` set but not `FILE_SHARE_DELETE` set, which I think is\n> generally an uncommon scenario to want to support.\n> \n> Generally if you're willing to accept files changing underneath you,\n> then you probably want to allow them to be deleted, too.  So this feels\n> like something that's very specific to QT Creator.  Or are there other\n> IDEs or development tools that use these open semantics that I'm not\n> aware of?\n\nI am also not aware of other IDEs which have this issue.\n\nOrgad, you also mentioned FILE_SHARE_DELETE here [*1*]. Does the Qt\nCreator issue persist despite this flag? You also just commented on\nGithub that \"Regarding Qt Creator, the issue should be mostly solved by\nnow in 4.7\". So a fix in Git is no longer needed?\n\n[*1*] https://github.com/git-for-windows/git/pull/1666\n"},{"id":"349946","messageId":"87a7s1vw9a.fsf@evledraar.gmail.com","threadId":"48687","inReplyTo":"20180611203958.GA1306@Sonnenschein.localdomain","subject":"Re: [PATCH v2] checkout files in-place","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-11T21:38:57Z","receivedAt":"2018-06-11T21:39:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 11 2018, Clemens Buchacher wrote:\n\n> When replacing files with new content during checkout, we do not write\n> to them in place. Instead we unlink and recreate the files in order to\n> let the system figure out ownership and permissions for the new file,\n> taking umask into account.\n\nBoth this summary...\n\n> It is safe to do this on Linux file systems, even if open file handles\n> still exist, because unlink only removes the directory reference to the\n> file. On Windows, however, a file cannot be deleted until all handles to\n> it are closed. If a file cannot be deleted, its name cannot be reused.\n>\n> This causes files to be deleted, but not checked out when switching\n> branches. This is frequently an issue with Qt Creator, which\n> continuously opens files in the work tree, as reported here:\n> https://github.com/git-for-windows/git/issues/1653\n>\n> This change adds the core.checkoutInPlace option. If enabled, checkout\n> will open files for writing the new content in place. This fixes the\n> issue, but with this approach the system will not update file\n> permissions according to umask. Only essential updates of write and\n> executable permissions are performed.\n>\n> The in-place checkout is therefore optional. It could be enabled by Git\n> installers on Windows, where umask is irrelevant.\n>\n> Signed-off-by: Clemens Buchacher <drizzd@gmx.net>\n> Reviewed-by: Orgad Shaneh <orgads@gmail.com>\n> Reviewed-by: \"brian m. carlson\" <sandals@crustytoothpaste.net>\n> Reviewed-by: Duy Nguyen <pclouds@gmail.com>\n> Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>\n> Tested on Windows with Git-for-Windows and with Windows Subsystem for\n> Linux.\n>\n>  Documentation/config.txt    | 11 ++++++\n>  cache.h                     |  2 ++\n>  config.c                    |  5 +++\n>  entry.c                     | 18 ++++++++--\n>  environment.c               |  1 +\n>  t/t2031-checkout-inplace.sh | 82 +++++++++++++++++++++++++++++++++++++++++++++\n>  6 files changed, 116 insertions(+), 3 deletions(-)\n>  create mode 100755 t/t2031-checkout-inplace.sh\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index ab641bf..0860a81 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -912,6 +912,17 @@ core.sparseCheckout::\n>  \tEnable \"sparse checkout\" feature. See section \"Sparse checkout\" in\n>  \tlinkgit:git-read-tree[1] for more information.\n>\n> +core.checkoutInPlace::\n> +\tCheck out file contents in place. By default Git checkout removes existing\n> +\twork tree files before it replaces them with different content. If this\n> +\toption is enabled, Git will overwrite the contents of existing files in\n> +\tplace. This is useful on Windows, where open file handles to a removed file\n> +\tprevent creating new files at the same path.\n\n...And this seems to conflict with what Junio's summarized in\nxmqqvaapb3r1.fsf@gitster-ct.c.googlers.com. I.e. (if I'm reading it\ncorrectly) it's not correct to say that we unlink the existing file,\nthen replace it, don't we create a new one, and then rename it in-place?\n\nI don't know enough about this part of the code to know, but whatever it\nis we should get it right here.\n\nAlso, as Junio notes that pattern will not result in a potentially\ncorrupt checkout where you've written 1/2 of a file, you note in\n20180611174818.GA8395@Sonnenschein.localdomain that there are \"no\nguarantees\", but I've never seen a case where being out of disk space\nwould cause a rename to fail, since that can happen in-place.\n\nWhereas we definitely will end up in states where we've written 1MB of a\n2MB file with this when the disk fills up, and thus when that's fixed\n\"git status\" will show local changes, so we should note that caveat for\nthe user.\n\n> +\tNote that the current implementation of in-place checkout makes no effort\n> +\tto update read/write permissions according to umask. Permissions are\n> +\thowever modified to enable write access and to update executable\n> +\tpermissions.\n\nI think we should have a paragraph break there before \"Note...\".\n\n>  core.abbrev::\n>  \tSet the length object names are abbreviated to.  If\n>  \tunspecified or set to \"auto\", an appropriate value is\n> diff --git a/cache.h b/cache.h\n> index 89a107a..5b8c4d6 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -815,6 +815,7 @@ extern int fsync_object_files;\n>  extern int core_preload_index;\n>  extern int core_commit_graph;\n>  extern int core_apply_sparse_checkout;\n> +extern int checkout_inplace;\n>  extern int precomposed_unicode;\n>  extern int protect_hfs;\n>  extern int protect_ntfs;\n> @@ -1518,6 +1519,7 @@ struct checkout {\n>  \tunsigned force:1,\n>  \t\t quiet:1,\n>  \t\t not_new:1,\n> +\t\t inplace:1,\n>  \t\t refresh_cache:1;\n>  };\n>  #define CHECKOUT_INIT { NULL, \"\" }\n> diff --git a/config.c b/config.c\n> index fbbf0f8..8b35ecd 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1318,6 +1318,11 @@ static int git_default_core_config(const char *var, const char *value)\n>  \t\treturn 0;\n>  \t}\n>\n> +\tif (!strcmp(var, \"core.checkoutinplace\")) {\n> +\t\tcheckout_inplace = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n> +\n>  \tif (!strcmp(var, \"core.precomposeunicode\")) {\n>  \t\tprecomposed_unicode = git_config_bool(var, value);\n>  \t\treturn 0;\n> diff --git a/entry.c b/entry.c\n> index 2101201..a599fc1 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -78,8 +78,13 @@ static void remove_subtree(struct strbuf *path)\n>\n>  static int create_file(const char *path, unsigned int mode)\n>  {\n> +\tint flags;\n> +\tif (checkout_inplace)\n> +\t\tflags = O_WRONLY | O_CREAT | O_TRUNC;\n> +\telse\n> +\t\tflags = O_WRONLY | O_CREAT | O_EXCL;\n\nI'd find this sort of thing easier to read as:\n\n\tint flags = O_WRONLY | O_CREAT;\n\tif (checkout_inplace)\n\t\tflags |= O_TRUNC;\n\telse\n\t\tflags |= O_EXCL;\n\nOr even:\n\n\tint flags = O_WRONLY | O_CREAT;\n\tflags |= checkout_inplace ? O_TRUNC : O_EXCL;\n\nI.e. less eyeballing the two lines to see if they're the same.\n\n>  \tmode = (mode & 0100) ? 0777 : 0666;\n> -\treturn open(path, O_WRONLY | O_CREAT | O_EXCL, mode);\n> +\treturn open(path, flags, mode);\n>  }\n>\n>  static void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)\n> @@ -467,8 +472,15 @@ int checkout_entry(struct cache_entry *ce,\n>  \t\t\tif (!state->force)\n>  \t\t\t\treturn error(\"%s is a directory\", path.buf);\n>  \t\t\tremove_subtree(&path);\n> -\t\t} else if (unlink(path.buf))\n> -\t\t\treturn error_errno(\"unable to unlink old '%s'\", path.buf);\n> +\t\t} else if (checkout_inplace) {\n> +\t\t\tif (!(st.st_mode & 0200) ||\n> +\t\t\t    (trust_executable_bit && (st.st_mode & 0100) != (ce->ce_mode & 0100)))\n> +\t\t\t\tif (chmod(path.buf, (ce->ce_mode & 0100) ? 0777 : 0666))\n> +\t\t\t\t\treturn error_errno(_(\"unable to change mode of '%s'\"), path.buf);\n> +\t\t} else {\n> +\t\t\tif (unlink(path.buf))\n> +\t\t\t\treturn error_errno(_(\"unable to unlink old '%s'\"), path.buf);\n> +\t\t}\n>  \t} else if (state->not_new)\n>  \t\treturn 0;\n>\n> diff --git a/environment.c b/environment.c\n> index 2a6de23..5b91f30 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -68,6 +68,7 @@ char *notes_ref_name;\n>  int grafts_replace_parents = 1;\n>  int core_commit_graph;\n>  int core_apply_sparse_checkout;\n> +int checkout_inplace;\n>  int merge_log_config = -1;\n>  int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n>  unsigned long pack_size_limit_cfg;\n> diff --git a/t/t2031-checkout-inplace.sh b/t/t2031-checkout-inplace.sh\n> new file mode 100755\n> index 0000000..d70ecc4\n> --- /dev/null\n> +++ b/t/t2031-checkout-inplace.sh\n> @@ -0,0 +1,82 @@\n> +#!/bin/sh\n> +\n> +test_description='in-place checkout'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +\n> +\ttest_commit hello world &&\n> +\tgit branch other &&\n> +\ttest_commit hello-again world\n> +'\n> +\n> +test_expect_success 'in-place checkout overwrites open file' '\n> +\n> +\tgit config core.checkoutInPlace true &&\n> +\tgit checkout -f master &&\n> +\texec 8<world &&\n> +\tgit checkout other &&\n> +\texec 8<&- &&\n> +\techo hello >expect &&\n> +\ttest_cmp expect world\n> +'\n> +\n> +test_expect_success 'in-place checkout overwrites read-only file' '\n> +\n> +\tgit config core.checkoutInPlace true &&\n> +\tgit checkout -f master &&\n> +\tchmod -w world &&\n> +\tgit checkout other &&\n> +\techo hello >expect &&\n> +\ttest_cmp expect world\n> +'\n> +\n> +test_expect_success 'in-place checkout updates executable permission' '\n> +\n> +\tgit config core.checkoutInPlace true &&\n> +\tgit checkout -f master^0 &&\n> +\ttest_chmod +x world &&\n> +\tgit commit -m executable &&\n> +\tgit checkout other &&\n> +\ttest ! -x world\n\nWorth testing switching branches here again & re-testing, since this\nonly tests +x -> -x, but not -x -> +x when we go back.\n\n> +'\n> +\n> +test_expect_success POSIXPERM 'regular checkout respects umask' '\n> +\n> +\tgit config core.checkoutInPlace false &&\n> +\tgit checkout -f master &&\n> +\tchmod 0660 world &&\n> +\tumask 0022 &&\n> +\tgit checkout other &&\n> +\tactual=$(ls -l world) &&\n> +\tcase \"$actual\" in\n> +\t-rw-r--r--*)\n> +\t\t: happy\n> +\t\t;;\n> +\t*)\n> +\t\techo Oops, world is not 0644 but $actual\n> +\t\tfalse\n> +\t\t;;\n> +\tesac\n\nIs that \"ls\" parsing portable? And also couldn't this be accomplished\nwith something like \"stat --format\"? I'm not sure how portable that is,\nwe just have one use of it in the test suite (on Cygwin only).\n\n> +'\n> +\n> +test_expect_success POSIXPERM 'in-place checkout ignores umask' '\n> +\n> +\tgit config core.checkoutInPlace true &&\n> +\tgit checkout -f master &&\n> +\tchmod 0660 world &&\n> +\tumask 0022 &&\n> +\tgit checkout other &&\n> +\tactual=$(ls -l world) &&\n> +\tcase \"$actual\" in\n> +\t-rw-rw----*)\n> +\t\t: happy\n> +\t\t;;\n> +\t*)\n> +\t\techo Oops, world is not 0660 but $actual\n> +\t\tfalse\n> +\t\t;;\n> +\tesac\n> +'\n> +\n> +test_done\n"},{"id":"349955","messageId":"xmqqa7s1aqlm.fsf@gitster-ct.c.googlers.com","threadId":"48687","inReplyTo":"87a7s1vw9a.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] checkout files in-place","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-11T22:46:45Z","receivedAt":"2018-06-11T22:46:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> ...And this seems to conflict with what Junio's summarized in\n> xmqqvaapb3r1.fsf@gitster-ct.c.googlers.com. I.e. (if I'm reading it\n> correctly) it's not correct to say that we unlink the existing file,\n> then replace it, don't we create a new one, and then rename it in-place?\n\nNo, my recollection was incorrect.  entry.c::checkout_entry() does\nan unlink() then write_entry() to the final place without any\nrename-to-finish phase.\n"},{"id":"349973","messageId":"20180612085119.GA5@aaaa10152750","threadId":"48687","inReplyTo":"CAGHpTBJFwToEwnk4P17AJ+z-55Nzc04OBbTvsbFRrkXJpfXAkQ@mail.gmail.com","subject":"Re: [PATCH] checkout files in-place","fromName":"Edward Thomson","fromEmail":"ethomson@edwardthomson.com","sentAt":"2018-06-12T08:51:19Z","receivedAt":"2018-06-12T08:51:51Z","isPatch":true,"sender":{"key":"ethomson@edwardthomson.com","avatar":"https://avatars.githubusercontent.com/u/1130014?v=4"},"body":"On Tue, Jun 12, 2018 at 09:13:54AM +0300, Orgad Shaneh wrote:\n> Some of my colleagues use an ancient version of Source Insight, which also\n> locks files for write.\n\nIf that application is locking files for writing (that is to say, it did\nnot specify the `FILE_SHARE_WRITE` bit in the sharing modes during\n`CreateFile`) then this patch would not help.\n\nApplications, generally speaking, should be locking files for write.\nIt's the default in Win32 and .NET's file open APIs because few\napplications are prepared to detect and support a file changing out from\nunderneath them in the middle of a read.\n\n> It's less important than it was before those fixes, but it is still needed\n> for users of Qt Creator 4.6 (previous versions just avoided mmap, 4.7 uses\n> mmap only for system headers). Other tools on Windows might as well\n> misbehave.\n\nI don't understand what mmap'ing via `CreateFileMapping` has to do with\nthis.  It takes an existing `HANDLE` that was opened with `CreateFile`,\nwhich is where the sharing mode was supplied.\n\nI would be surprised if there are other tools on Windows that have\nspecified `FILE_SHARE_WRITE` but not `FILE_SHARE_DELETE`.  Generally\nspeaking, if you don't care about another process changing a file\nunderneath you then you should specify both.  If you do then you should\nspecify neither.\n\nI'm not saying that git shouldn't work around a bug in QT Creator -\nthat's not my call, though I would be loathe to support this\nconfiguration option in libgit2.  But I am saying that it seems like\nthis patch doesn't have broad applicability beyond that particular tool.\n\n-ed\n"},{"id":"350049","messageId":"CAGHpTBJ9WiWdJw=SgxJpWqP9CucANatafx6iwCRCRY15wTBsVg@mail.gmail.com","threadId":"48687","inReplyTo":"20180612085119.GA5@aaaa10152750","subject":"Re: [PATCH] checkout files in-place","fromName":"Orgad Shaneh","fromEmail":"orgads@gmail.com","sentAt":"2018-06-13T07:39:17Z","receivedAt":"2018-06-13T07:39:32Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"On Tue, Jun 12, 2018 at 11:51 AM Edward Thomson\n<ethomson@edwardthomson.com> wrote:\n>\n> On Tue, Jun 12, 2018 at 09:13:54AM +0300, Orgad Shaneh wrote:\n> > Some of my colleagues use an ancient version of Source Insight, which also\n> > locks files for write.\n>\n> If that application is locking files for writing (that is to say, it did\n> not specify the `FILE_SHARE_WRITE` bit in the sharing modes during\n> `CreateFile`) then this patch would not help.\n>\n> Applications, generally speaking, should be locking files for write.\n> It's the default in Win32 and .NET's file open APIs because few\n> applications are prepared to detect and support a file changing out from\n> underneath them in the middle of a read.\n\nI agree.\n\n> > It's less important than it was before those fixes, but it is still needed\n> > for users of Qt Creator 4.6 (previous versions just avoided mmap, 4.7 uses\n> > mmap only for system headers). Other tools on Windows might as well\n> > misbehave.\n>\n> I don't understand what mmap'ing via `CreateFileMapping` has to do with\n> this.  It takes an existing `HANDLE` that was opened with `CreateFile`,\n> which is where the sharing mode was supplied.\n\nI'm not completely sure. The file is opened using CreateFile[1] with\nFILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE.\nThen this handle is passed to CreateFileMapping[2]. For a reason I don't\nunderstand, when mapping is used, the handle is never released (until\nthe file is closed), but when it is not used, the file is being read, then the\nhandle is released.\n\nMaybe Ivan or Nikolai can shed some light on this process.\n\nAnyway, with Qt Creator 4.7 this should be a non-issue, so I'm reluctant about\nthis change here.\n\n> I would be surprised if there are other tools on Windows that have\n> specified `FILE_SHARE_WRITE` but not `FILE_SHARE_DELETE`.  Generally\n> speaking, if you don't care about another process changing a file\n> underneath you then you should specify both.  If you do then you should\n> specify neither.\n\nThe problem is that even if you specify FILE_SHARE_WRITE and FILE_SHARE_DELETE,\nthe file can be unlinked, but it cannot be created with the same name\nuntil its handle\nis closed, unless you rename it *before* unlinking.\n\n- Orgad\n\n[1] https://github.com/llvm-mirror/llvm/blob/371257e/lib/Support/Windows/Path.inc#L1045\n[2] https://github.com/llvm-mirror/llvm/blob/371257e/lib/Support/Windows/Path.inc#L836\n"}]}