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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 23, 2016, 17:06 UTC
Message-ID: <xmqq1sy2nn1g.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161122100046.8341-3-pclouds@gmail.com>

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

Show 11 quoted lines

> 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.

Show 13 quoted lines

> @@ -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.

Show 16 quoted lines

> 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.

Show 7 quoted lines

> @@ -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.