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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 12, 2009, 03:49 UTC
Message-ID: <7vskmkqpp4.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1234263701-95463-1-git-send-email-jaysoffian@gmail.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?

Show 13 quoted lines

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

Show 8 quoted lines

> +	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?

Show 10 quoted lines

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