Re: [PATCH 2/2] setup: don't fail if commondir reference is deleted.
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Feb 21, 2019, 10:50 UTC
- Message-ID
- <CACsJy8AWezO7TFq8ne1a2pSAJZoc6oYqnNNxmVW_FkA9--ntbQ@mail.gmail.com>
- In-Reply-To
- <6f9c8775817117c2b36539eb048e2462a650ab8f.1550508544.git.msuchanek@suse.de>
On Tue, Feb 19, 2019 at 12:05 AM Michal Suchanek <msuchanek@suse.de> wrote:
> > When adding wotktrees git can die in get_common_dir_noenv while > examining existing worktrees because the commondir file does not exist. > Rather than testing if the file exists before reading it handle ENOENT.
I don't think we could go around fixing every access to incomplete worktrees like this. If this is because of racy 'worktree add', then perhaps a better solution is make it absolutely clear it's not ready for anybody to access.
For example, we can suffix the worktree directory name with ".lock" and make sure get_worktrees() ignores entries ending with ".lock". That should protect other commands while 'worktree add' is still running. Only when the worktree is complete that 'worktree add' should rename the directory to lose ".lock" and run external commands like git-checkout to populate the worktree.
Show 49 quoted lines
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
> v2:
> - do not test file existence first, just read it and handle ENOENT.
> - handle zero size file correctly
> ---
> setup.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index ca9e8a949ed8..dd865f280d34 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -270,12 +270,20 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
> {
> struct strbuf data = STRBUF_INIT;
> struct strbuf path = STRBUF_INIT;
> - int ret = 0;
> + int ret;
>
> 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)
> die_errno(_("failed to read %s"), path.buf);
> + strbuf_addstr(sb, gitdir);
> + ret = 0;
> + } else {
> while (data.len && (data.buf[data.len - 1] == '\n' ||
> data.buf[data.len - 1] == '\r'))
> data.len--;
> @@ -286,8 +294,6 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
> strbuf_addbuf(&path, &data);
> strbuf_add_real_path(sb, path.buf);
> ret = 1;
> - } else {
> - strbuf_addstr(sb, gitdir);
> }
>
> strbuf_release(&data);
> --
> 2.20.1
>-- Duy