Re: [PATCH 2/3] get_worktrees() must return main worktree as first item even on error

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 5c4854d..b835b91 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -388,7 +388,7 @@ static void show_worktree_porcelain(struct worktree *wt)
>  		printf("HEAD %s\n", sha1_to_hex(wt->head_sha1));
>  		if (wt->is_detached)
>  			printf("detached\n");
> -		else
> +		else if (wt->head_ref)
>  			printf("branch %s\n", wt->head_ref);

This change looks somewhat unrelated to what the title and the log
message claims to do, but the fix is to indicate an error condition
by leaving wt->head_ref as NULL, so this is a necessary adjustment.

Good.

> @@ -406,10 +406,12 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
>  	else {
>  		strbuf_addf(&sb, "%-*s ", abbrev_len,
>  				find_unique_abbrev(wt->head_sha1, DEFAULT_ABBREV));
> -		if (!wt->is_detached)
> +		if (wt->is_detached)
> +			strbuf_addstr(&sb, "(detached HEAD)");
> +		else if (wt->head_ref)
>  			strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(wt->head_ref, 0));
>  		else
> -			strbuf_addstr(&sb, "(detached HEAD)");
> +			strbuf_addstr(&sb, "(error)");
>  	}

Likewise.

> diff --git a/worktree.c b/worktree.c
> index f7c1b5e..a674efa 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -89,7 +89,7 @@ static struct worktree *get_main_worktree(void)
>  	strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
>  
>  	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
> -		goto done;
> +		strbuf_reset(&head_ref);
>  
>  	worktree = xcalloc(1, sizeof(*worktree));
>  	worktree->path = strbuf_detach(&worktree_path, NULL);
> @@ -97,7 +97,6 @@ static struct worktree *get_main_worktree(void)
>  	worktree->is_detached = is_detached;
>  	add_head_info(&head_ref, worktree);

OK.  The earlier call to _reset() and add_head_info() function
itself may want to be clarified that a zero-length strbuf signals an
error condition with additional comment.  It is all too unclear in
the code with this patch as it stands.

> -done:
>  	strbuf_release(&path);
>  	strbuf_release(&worktree_path);
>  	strbuf_release(&head_ref);

After this there is "return worktree" which used to return NULL
because of the "goto", but we never return NULL from the function
after this change, which is the whole point of this change.  Good.

> @@ -173,8 +172,7 @@ struct worktree **get_worktrees(void)
>  
>  	list = xmalloc(alloc * sizeof(struct worktree *));
>  
> -	if ((list[counter] = get_main_worktree()))
> -		counter++;
> +	list[counter++] = get_main_worktree();

Hence the conditional, while it does not hurt, becomes unnecessary
and we can unconditionally throw the primary one to the list.

Good.

Other than the "these need in-code commenting", and also that this
should have a new test, the patch makes sense to me.

Thanks.