Re: [PATCH] worktree add: add --lock option

From: Duy Nguyen <pclouds@gmail.com>
Date: Apr 15, 2017, 11:34 UTC
Message-ID: <CACsJy8A=Pi8uaEumhddzySEywxAS8vau5mv-sEewmuwNUvZp_A@mail.gmail.com>
In-Reply-To: <xmqqmvbim6vi.fsf@gitster.mtv.corp.google.com>

On Sat, Apr 15, 2017 at 3:07 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 19 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> -    unlink_or_warn(sb.buf);
>>> +    if (!ret && opts->keep_locked) {
>>> +            /*
>>> +             * Don't keep the confusing "initializing" message
>>> +             * after it's already over.
>>> +             */
>>> +            truncate(sb.buf, 0);
>>> +    } else {
>>> +            unlink_or_warn(sb.buf);
>>> +    }
>>
>> builtin/worktree.c: In function 'add_worktree':
>> builtin/worktree.c:314:11: error: ignoring return value of 'truncate', declared with attribute warn_unused_result [-Werror=unused-result]
>>    truncate(sb.buf, 0);
>>            ^

Yes it's supposed to be safe to ignore the error in this case. I thought of adding (void) last minute but didn't do it.

Show 6 quoted lines

>> cc1: all warnings being treated as errors
>> make: *** [builtin/worktree.o] Error 1
>
> I wonder why we need to have "initializing" and then remove the
> string.  Wouldn't it be simpler to do something like this instead?
> Does an empty lockfile have some special meaning?

It's mostly for troubleshooting. If "git worktree add" fails in a later phase with a valid/locked worktree remains, this gives us a clue.

Show 50 quoted lines

>  builtin/worktree.c      | 16 +++++++---------
>  t/t2025-worktree-add.sh |  3 +--
>  2 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 3dab07c829..5ebdcce793 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -243,7 +243,10 @@ static int add_worktree(const char *path, const char *refname,
>          * after the preparation is over.
>          */
>         strbuf_addf(&sb, "%s/locked", sb_repo.buf);
> -       write_file(sb.buf, "initializing");
> +       if (!opts->keep_locked)
> +               write_file(sb.buf, "initializing");
> +       else
> +               write_file(sb.buf, "added with --lock");
>
>         strbuf_addf(&sb_git, "%s/.git", path);
>         if (safe_create_leading_directories_const(sb_git.buf))
> @@ -306,15 +309,10 @@ static int add_worktree(const char *path, const char *refname,
>  done:
>         strbuf_reset(&sb);
>         strbuf_addf(&sb, "%s/locked", sb_repo.buf);
> -       if (!ret && opts->keep_locked) {
> -               /*
> -                * Don't keep the confusing "initializing" message
> -                * after it's already over.
> -                */
> -               truncate(sb.buf, 0);
> -       } else {
> +       if (!ret && opts->keep_locked)
> +               ;
> +       else
>                 unlink_or_warn(sb.buf);
> -       }
>         argv_array_clear(&child_env);
>         strbuf_release(&sb);
>         strbuf_release(&symref);
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 6dce920c03..304f25fcd1 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -65,8 +65,7 @@ test_expect_success '"add" worktree' '
>
>  test_expect_success '"add" worktree with lock' '
>         git rev-parse HEAD >expect &&
> -       git worktree add --detach --lock here-with-lock master &&
> -       test_must_be_empty .git/worktrees/here-with-lock/locked
> +       git worktree add --detach --lock here-with-lock master

I think you still need to check for the presence of "locked" file.

>  '
>
>  test_expect_success '"add" worktree from a subdir' '

-- 
Duy