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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Jay Soffian <jaysoffian@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux