From mboxrd@z Thu Jan 1 00:00:00 1970 From: Junio C Hamano Subject: Re: [PATCH] builtin-branch: improve output when displaying remote branches Date: Wed, 11 Feb 2009 19:49:59 -0800 Message-ID: <7vskmkqpp4.fsf@gitster.siamese.dyndns.org> References: <1234263701-95463-1-git-send-email-jaysoffian@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: git@vger.kernel.org To: Jay Soffian X-From: git-owner@vger.kernel.org Thu Feb 12 04:51:37 2009 Return-path: Envelope-to: gcvg-git-2@gmane.org Received: from vger.kernel.org ([209.132.176.167]) by lo.gmane.org with esmtp (Exim 4.50) id 1LXSbw-0001HW-IO for gcvg-git-2@gmane.org; Thu, 12 Feb 2009 04:51:37 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754777AbZBLDuK (ORCPT ); Wed, 11 Feb 2009 22:50:10 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752359AbZBLDuJ (ORCPT ); Wed, 11 Feb 2009 22:50:09 -0500 Received: from a-sasl-quonix.sasl.smtp.pobox.com ([208.72.237.25]:41346 "EHLO sasl.smtp.pobox.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751953AbZBLDuI (ORCPT ); Wed, 11 Feb 2009 22:50:08 -0500 Received: from localhost.localdomain (unknown [127.0.0.1]) by b-sasl-quonix.sasl.smtp.pobox.com (Postfix) with ESMTP id E21782AF31; Wed, 11 Feb 2009 22:50:03 -0500 (EST) Received: from pobox.com (unknown [68.225.240.211]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by b-sasl-quonix.sasl.smtp.pobox.com (Postfix) with ESMTPSA id E49972AF30; Wed, 11 Feb 2009 22:50:00 -0500 (EST) In-Reply-To: <1234263701-95463-1-git-send-email-jaysoffian@gmail.com> (Jay Soffian's message of "Tue, 10 Feb 2009 06:01:41 -0500") User-Agent: Gnus/5.110006 (No Gnus v0.6) Emacs/21.4 (gnu/linux) X-Pobox-Relay-ID: 3B091372-F8B8-11DD-BF54-6F7C8D1D4FD0-77302942!a-sasl-quonix.pobox.com Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: Jay Soffian 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.