On Sat, Apr 15, 2017 at 3:07 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
>> Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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.
>> 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.
> 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