From: "Michal Suchánek" <msuchanek@suse.de>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: "Git List" <git@vger.kernel.org>,
"Marketa Calabkova" <mcalabkova@suse.cz>,
"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
"Junio C Hamano" <gitster@pobox.com>
Subject: Re: [PATCH v3 2/2] setup: don't fail if commondir reference is deleted.
Date: Wed, 20 Feb 2019 18:16:05 +0100 [thread overview]
Message-ID: <20190220181605.60bbc28d@kitsune.suse.cz> (raw)
In-Reply-To: <CAPig+cQZNOWvaa5H2PKOs149KvRtEYRzrdLvzvFRDo4Qxaecaw@mail.gmail.com>
On Wed, 20 Feb 2019 11:55:46 -0500
Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Feb 20, 2019 at 11:17 AM Michal Suchanek <msuchanek@suse.de> wrote:
> > Apparently it can happen that stat() claims there is a commondir file but when
> > trying to open the file it is missing.
>
> Under what circumstances?
I would like to know that as well. The only command tested was worktree
add which should not remove the file. Nonetheless running many woktree
add commands in parallel can cause the file to go away for some of
them. For many commands git calls itself recursively so there is
probably much more going on than the single function that creates the
worktree.
>
> > Another even rarer issue is that the file might be zero size because another
> > process initializing a worktree opened the file but has not written is content
> > yet.
>
> Based upon the explanation thus far, I'm having trouble understanding
> under what circumstances these race conditions can arise. Are you
> trying to invoke Git commands in a particular worktree even as the
> worktree itself is being created?
It's explained in the following paragraph. If you have multiple
worktrees some *other* worktreee may be uninitialized.
>
> Without this information being spelled out clearly, it is going to be
> difficult for someone in the future to reason about why the code is
> the way it is following this change.
>
> > When any of this happnes git aborts failing to perform perfectly valid
> > command because unrelated worktree is not yet fully initialized.
>
> s/happnes/happens/
>
> > Rather than testing if the file exists before reading it handle ENOENT
> > and ENOTDIR.
>
> One more comment below...
>
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> > diff --git a/setup.c b/setup.c
> > @@ -270,12 +270,20 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
> > {
> > strbuf_addf(&path, "%s/commondir", gitdir);
> > - if (file_exists(path.buf)) {
> > - if (strbuf_read_file(&data, path.buf, 0) <= 0)
> > + ret = strbuf_read_file(&data, path.buf, 0);
> > + if (ret <= 0) {
> > + /*
> > + * if file is missing or zero size (just being written)
> > + * assume default, bail otherwise
> > + */
> > + if (ret && errno != ENOENT && errno != ENOTDIR)
> > die_errno(_("failed to read %s"), path.buf);
>
> It's not clear from the explanation given in the commit message if the
> new behavior is indeed sensible. The original intent of the code, as I
> understand it, is to validate "commondir", to ensure that it is not
> somehow corrupt (such as the user editing it and making it empty).
How is it validated in the code below when it is non-zero size?
There is *no* validation whatsoever. Yet zero size is somehow totally
unacceptable and requires that git working in *any* worktree aborts if
commondir file in *any* worktree is zero size.
> Following this change, that particular validation no longer takes
> place. But, more importantly, what does it mean to fall back to
> "default" for this particular worktree? I'm having trouble
> understanding how the new behavior can be correct or desirable. (Am I
> missing something obvious?)
If the file can be missing altogether and it is not an error how it is
incorrect or undesirable to ignore zero size file?
Thanks
Michal
next prev parent reply other threads:[~2019-02-20 17:16 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-18 17:04 [PATCH 0/2] worktree add race fix Michal Suchanek
2019-02-18 17:04 ` [PATCH 1/2] worktree: fix worktree add race Michal Suchanek
2019-02-18 17:04 ` [PATCH 2/2] setup: don't fail if commondir reference is deleted Michal Suchanek
2019-02-18 21:00 ` Eric Sunshine
2019-02-21 10:50 ` Duy Nguyen
2019-02-21 13:50 ` Michal Suchánek
2019-02-21 17:07 ` Phillip Wood
2019-02-21 17:12 ` Eric Sunshine
2019-02-21 17:27 ` Phillip Wood
2019-03-04 13:30 ` Michal Suchánek
2019-02-21 17:33 ` Michal Suchánek
2019-02-22 9:32 ` Duy Nguyen
2019-02-22 10:20 ` Phillip Wood
2019-02-22 9:26 ` Duy Nguyen
2019-02-20 16:16 ` [PATCH v3 1/2] worktree: fix worktree add race Michal Suchanek
2019-02-20 16:34 ` Eric Sunshine
2019-02-20 17:29 ` Michal Suchánek
2019-03-08 9:20 ` Duy Nguyen
2019-03-08 9:37 ` Eric Sunshine
2019-03-11 1:55 ` Junio C Hamano
2019-02-20 16:16 ` [PATCH v3 2/2] setup: don't fail if commondir reference is deleted Michal Suchanek
2019-02-20 16:55 ` Eric Sunshine
2019-02-20 17:16 ` Michal Suchánek [this message]
2019-02-20 18:35 ` Eric Sunshine
2019-02-21 9:27 ` Eric Sunshine
2019-02-21 11:13 ` Michal Suchánek
2019-02-21 11:19 ` Michal Suchánek
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=20190220181605.60bbc28d@kitsune.suse.cz \
--to=msuchanek@suse.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=mcalabkova@suse.cz \
--cc=pclouds@gmail.com \
--cc=sunshine@sunshineco.com \
/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).