From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Clemens Buchacher <drizzd@gmx.net>
Cc: git@vger.kernel.org, Duy Nguyen <pclouds@gmail.com>,
"brian m. carlson" <sandals@crustytoothpaste.net>,
Junio C Hamano <gitster@pobox.com>,
Orgad Shaneh <orgads@gmail.com>
Subject: Re: [PATCH v2] checkout files in-place
Date: Mon, 11 Jun 2018 23:38:57 +0200 [thread overview]
Message-ID: <87a7s1vw9a.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20180611203958.GA1306@Sonnenschein.localdomain>
On Mon, Jun 11 2018, Clemens Buchacher wrote:
> When replacing files with new content during checkout, we do not write
> to them in place. Instead we unlink and recreate the files in order to
> let the system figure out ownership and permissions for the new file,
> taking umask into account.
Both this summary...
> It is safe to do this on Linux file systems, even if open file handles
> still exist, because unlink only removes the directory reference to the
> file. On Windows, however, a file cannot be deleted until all handles to
> it are closed. If a file cannot be deleted, its name cannot be reused.
>
> This causes files to be deleted, but not checked out when switching
> branches. This is frequently an issue with Qt Creator, which
> continuously opens files in the work tree, as reported here:
> https://github.com/git-for-windows/git/issues/1653
>
> This change adds the core.checkoutInPlace option. If enabled, checkout
> will open files for writing the new content in place. This fixes the
> issue, but with this approach the system will not update file
> permissions according to umask. Only essential updates of write and
> executable permissions are performed.
>
> The in-place checkout is therefore optional. It could be enabled by Git
> installers on Windows, where umask is irrelevant.
>
> Signed-off-by: Clemens Buchacher <drizzd@gmx.net>
> Reviewed-by: Orgad Shaneh <orgads@gmail.com>
> Reviewed-by: "brian m. carlson" <sandals@crustytoothpaste.net>
> Reviewed-by: Duy Nguyen <pclouds@gmail.com>
> Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> Tested on Windows with Git-for-Windows and with Windows Subsystem for
> Linux.
>
> Documentation/config.txt | 11 ++++++
> cache.h | 2 ++
> config.c | 5 +++
> entry.c | 18 ++++++++--
> environment.c | 1 +
> t/t2031-checkout-inplace.sh | 82 +++++++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 116 insertions(+), 3 deletions(-)
> create mode 100755 t/t2031-checkout-inplace.sh
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ab641bf..0860a81 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -912,6 +912,17 @@ core.sparseCheckout::
> Enable "sparse checkout" feature. See section "Sparse checkout" in
> linkgit:git-read-tree[1] for more information.
>
> +core.checkoutInPlace::
> + Check out file contents in place. By default Git checkout removes existing
> + work tree files before it replaces them with different content. If this
> + option is enabled, Git will overwrite the contents of existing files in
> + place. This is useful on Windows, where open file handles to a removed file
> + prevent creating new files at the same path.
...And this seems to conflict with what Junio's summarized in
xmqqvaapb3r1.fsf@gitster-ct.c.googlers.com. I.e. (if I'm reading it
correctly) it's not correct to say that we unlink the existing file,
then replace it, don't we create a new one, and then rename it in-place?
I don't know enough about this part of the code to know, but whatever it
is we should get it right here.
Also, as Junio notes that pattern will not result in a potentially
corrupt checkout where you've written 1/2 of a file, you note in
20180611174818.GA8395@Sonnenschein.localdomain that there are "no
guarantees", but I've never seen a case where being out of disk space
would cause a rename to fail, since that can happen in-place.
Whereas we definitely will end up in states where we've written 1MB of a
2MB file with this when the disk fills up, and thus when that's fixed
"git status" will show local changes, so we should note that caveat for
the user.
> + Note that the current implementation of in-place checkout makes no effort
> + to update read/write permissions according to umask. Permissions are
> + however modified to enable write access and to update executable
> + permissions.
I think we should have a paragraph break there before "Note...".
> core.abbrev::
> Set the length object names are abbreviated to. If
> unspecified or set to "auto", an appropriate value is
> diff --git a/cache.h b/cache.h
> index 89a107a..5b8c4d6 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -815,6 +815,7 @@ extern int fsync_object_files;
> extern int core_preload_index;
> extern int core_commit_graph;
> extern int core_apply_sparse_checkout;
> +extern int checkout_inplace;
> extern int precomposed_unicode;
> extern int protect_hfs;
> extern int protect_ntfs;
> @@ -1518,6 +1519,7 @@ struct checkout {
> unsigned force:1,
> quiet:1,
> not_new:1,
> + inplace:1,
> refresh_cache:1;
> };
> #define CHECKOUT_INIT { NULL, "" }
> diff --git a/config.c b/config.c
> index fbbf0f8..8b35ecd 100644
> --- a/config.c
> +++ b/config.c
> @@ -1318,6 +1318,11 @@ static int git_default_core_config(const char *var, const char *value)
> return 0;
> }
>
> + if (!strcmp(var, "core.checkoutinplace")) {
> + checkout_inplace = git_config_bool(var, value);
> + return 0;
> + }
> +
> if (!strcmp(var, "core.precomposeunicode")) {
> precomposed_unicode = git_config_bool(var, value);
> return 0;
> diff --git a/entry.c b/entry.c
> index 2101201..a599fc1 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -78,8 +78,13 @@ static void remove_subtree(struct strbuf *path)
>
> static int create_file(const char *path, unsigned int mode)
> {
> + int flags;
> + if (checkout_inplace)
> + flags = O_WRONLY | O_CREAT | O_TRUNC;
> + else
> + flags = O_WRONLY | O_CREAT | O_EXCL;
I'd find this sort of thing easier to read as:
int flags = O_WRONLY | O_CREAT;
if (checkout_inplace)
flags |= O_TRUNC;
else
flags |= O_EXCL;
Or even:
int flags = O_WRONLY | O_CREAT;
flags |= checkout_inplace ? O_TRUNC : O_EXCL;
I.e. less eyeballing the two lines to see if they're the same.
> mode = (mode & 0100) ? 0777 : 0666;
> - return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);
> + return open(path, flags, mode);
> }
>
> static void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)
> @@ -467,8 +472,15 @@ int checkout_entry(struct cache_entry *ce,
> if (!state->force)
> return error("%s is a directory", path.buf);
> remove_subtree(&path);
> - } else if (unlink(path.buf))
> - return error_errno("unable to unlink old '%s'", path.buf);
> + } else if (checkout_inplace) {
> + if (!(st.st_mode & 0200) ||
> + (trust_executable_bit && (st.st_mode & 0100) != (ce->ce_mode & 0100)))
> + if (chmod(path.buf, (ce->ce_mode & 0100) ? 0777 : 0666))
> + return error_errno(_("unable to change mode of '%s'"), path.buf);
> + } else {
> + if (unlink(path.buf))
> + return error_errno(_("unable to unlink old '%s'"), path.buf);
> + }
> } else if (state->not_new)
> return 0;
>
> diff --git a/environment.c b/environment.c
> index 2a6de23..5b91f30 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -68,6 +68,7 @@ char *notes_ref_name;
> int grafts_replace_parents = 1;
> int core_commit_graph;
> int core_apply_sparse_checkout;
> +int checkout_inplace;
> int merge_log_config = -1;
> int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
> unsigned long pack_size_limit_cfg;
> diff --git a/t/t2031-checkout-inplace.sh b/t/t2031-checkout-inplace.sh
> new file mode 100755
> index 0000000..d70ecc4
> --- /dev/null
> +++ b/t/t2031-checkout-inplace.sh
> @@ -0,0 +1,82 @@
> +#!/bin/sh
> +
> +test_description='in-place checkout'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +
> + test_commit hello world &&
> + git branch other &&
> + test_commit hello-again world
> +'
> +
> +test_expect_success 'in-place checkout overwrites open file' '
> +
> + git config core.checkoutInPlace true &&
> + git checkout -f master &&
> + exec 8<world &&
> + git checkout other &&
> + exec 8<&- &&
> + echo hello >expect &&
> + test_cmp expect world
> +'
> +
> +test_expect_success 'in-place checkout overwrites read-only file' '
> +
> + git config core.checkoutInPlace true &&
> + git checkout -f master &&
> + chmod -w world &&
> + git checkout other &&
> + echo hello >expect &&
> + test_cmp expect world
> +'
> +
> +test_expect_success 'in-place checkout updates executable permission' '
> +
> + git config core.checkoutInPlace true &&
> + git checkout -f master^0 &&
> + test_chmod +x world &&
> + git commit -m executable &&
> + git checkout other &&
> + test ! -x world
Worth testing switching branches here again & re-testing, since this
only tests +x -> -x, but not -x -> +x when we go back.
> +'
> +
> +test_expect_success POSIXPERM 'regular checkout respects umask' '
> +
> + git config core.checkoutInPlace false &&
> + git checkout -f master &&
> + chmod 0660 world &&
> + umask 0022 &&
> + git checkout other &&
> + actual=$(ls -l world) &&
> + case "$actual" in
> + -rw-r--r--*)
> + : happy
> + ;;
> + *)
> + echo Oops, world is not 0644 but $actual
> + false
> + ;;
> + esac
Is that "ls" parsing portable? And also couldn't this be accomplished
with something like "stat --format"? I'm not sure how portable that is,
we just have one use of it in the test suite (on Cygwin only).
> +'
> +
> +test_expect_success POSIXPERM 'in-place checkout ignores umask' '
> +
> + git config core.checkoutInPlace true &&
> + git checkout -f master &&
> + chmod 0660 world &&
> + umask 0022 &&
> + git checkout other &&
> + actual=$(ls -l world) &&
> + case "$actual" in
> + -rw-rw----*)
> + : happy
> + ;;
> + *)
> + echo Oops, world is not 0660 but $actual
> + false
> + ;;
> + esac
> +'
> +
> +test_done
next prev parent reply other threads:[~2018-06-11 21:39 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-10 19:44 [PATCH] checkout files in-place Clemens Buchacher
2018-06-11 2:04 ` brian m. carlson
2018-06-11 17:48 ` Clemens Buchacher
2018-06-11 18:02 ` Junio C Hamano
2018-06-11 20:22 ` Clemens Buchacher
2018-06-11 17:59 ` Ævar Arnfjörð Bjarmason
2018-06-11 20:35 ` Edward Thomson
2018-06-11 20:57 ` Clemens Buchacher
[not found] ` <CAGHpTBJFwToEwnk4P17AJ+z-55Nzc04OBbTvsbFRrkXJpfXAkQ@mail.gmail.com>
2018-06-12 8:51 ` Edward Thomson
2018-06-13 7:39 ` Orgad Shaneh
2018-06-11 20:39 ` [PATCH v2] " Clemens Buchacher
2018-06-11 21:38 ` Ævar Arnfjörð Bjarmason [this message]
2018-06-11 22:46 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87a7s1vw9a.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=drizzd@gmx.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=orgads@gmail.com \
--cc=pclouds@gmail.com \
--cc=sandals@crustytoothpaste.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).