Re: [PATCH] builtin-branch: improve output when displaying remote branches

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

Jay Soffian <jaysoffian@gmail.com> writes:

> $ git branch -rv
>   origin/HEAD -> master
>   origin/html           6116912 Autogenerated HTML docs for v1.6.2-rc0-10-gf6b9

Doesn't the misalignment between the above two bother you?

> diff --git a/builtin-branch.c b/builtin-branch.c
> index 56a1971..03ad757 100644
> --- a/builtin-branch.c
> +++ b/builtin-branch.c
> @@ -181,7 +181,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
> +static char *resolve_remote_head_symref(const char *head_name) {
> +	unsigned char sha1[20];
> +	int flag;
> +	const char *refname;
> +	refname = resolve_ref(head_name, sha1, 0, &flag);
> +	if (refname && (flag & REF_ISSYMREF) &&
> +	    !prefixcmp(refname, "refs/remotes/"))
> +		return xstrdup(refname + strlen(head_name) - 4);

Here, head_name is like "refs/remotes/frotz/HEAD", and you are assuming
that resolved refname begins with "refs/remotes/frotz/" without checking
the "frotz" part. It may point at "refs/remotes/x/y" in a misconfigured
repository and your xstrdup() just ran past the end of the string.

If the ref you feed to this function turns out not to be a symbolic ref,
the caller does do the right thing.  It makes wonder if your caller should
always call this, so that you would still work sensibly even if the tracking
hierarchy has a funny symref refs/remotes/origin/TAIL that is not HEAD.

The caller is currently this dense code.

> +	newitem->len = strlen(newitem->name);
> +	newitem->dest = (newitem->kind == REF_REMOTE_BRANCH &&
> +			 newitem->len > 5 &&
> +			 !strcmp(newitem->name + newitem->len - 5, "/HEAD"))
> +			? resolve_remote_head_symref(refname - 13) : NULL;
> +	/* adjust for " -> " */
> +	if (newitem->dest)
> +		newitem->len += strlen(newitem->dest) + 4;

It can become something like:

	if (newitem->kind == REF_REMOTE_BRANCH)
		newitem->dest = resolve_remote_symref(refname - 13);
	else
		newitem->dest = NULL;
        if (newitem->dest)
        	...
	
no?

> @@ -250,8 +272,11 @@ static void free_ref_list(struct ref_list *ref_list)
>  {
>  	int i;
>  
> -	for (i = 0; i < ref_list->index; i++)
> +	for (i = 0; i < ref_list->index; i++) {
>  		free(ref_list->list[i].name);
> +		if (ref_list->list[i].dest)
> +			free(ref_list->list[i].dest);
> +	}

free(NULL) is Ok; omit the extra check.