From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 06 Mar 2013 18:48:41 GMT
Subject: Re: [PATCH v2 1/4] wt-status: split wt_status_state parsing function out
Message-ID: <7v4ngoqs8m.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1362572482-20570-2-git-send-email-pclouds@gmail.com>

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

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  wt-status.c | 52 +++++++++++++++++++++++++++++++---------------------
>  wt-status.h |  5 +++--
>  2 files changed, 34 insertions(+), 23 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index ef405d0..183aafe 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -970,7 +970,7 @@ static void show_bisect_in_progress(struct wt_status *s,
>   * Extract branch information from rebase/bisect
>   */
>  static void read_and_strip_branch(struct strbuf *sb,
> -				  const char **branch,
> +				  char **branch,
>  				  const char *path)
>  {
>  	unsigned char sha1[20];
> @@ -994,52 +994,62 @@ static void read_and_strip_branch(struct strbuf *sb,
>  		strbuf_addstr(sb, abbrev);
>  		*branch = sb->buf;
>  	} else if (!strcmp(sb->buf, "detached HEAD")) /* rebase */
> -		;
> +		*branch = NULL;
>  	else			/* bisect */
>  		*branch = sb->buf;
> +	if (*branch)
> +		*branch = xstrdup(*branch);
>  }

The reason why the original print_state() kept two strbufs in it was
because its use of the return value (in *branch) from this function
was private and it did not want to have to strdup anything.

With this change, I suspect that it is much saner to make this
function *not* take any external strbuf as input, because you are
always returning a piece of memory that belongs to the caller, or a
NULL.

In other words, with the new world order, wouldn't a saner function
signature be:

	static const char *read_and_strip_branch(const char **path);

after this patch?

Also I notice that the error-return cases of this function may be
wrong even before your patch.  Shouldn't it be doing *branch = NULL
(and 'return NULL' after the suggested change)?