Re: [PATCH 6/7] sha1_name: cleanup interpret_branch_name()

From: Philip Oakley <philipoakley@iee.org>
Date: Dec 8, 2013, 05:56 UTC
Message-ID: <67130C4F312A414DAB7EA610C78C9103@PhilipOakley>
In-Reply-To: <1386482220-18558-7-git-send-email-felipe.contreras@gmail.com>

From: "Felipe Contreras" <felipe.contreras@gmail.com>

Show 33 quoted lines

> The 'upstream' variable doesn't hold an "upstream", but a branch, so
> make it clearer.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> sha1_name.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index e9c2999..e27db88 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -1072,7 +1072,7 @@ static int reinterpret(const char *name, int 
> namelen, int len, struct strbuf *bu
> int interpret_branch_name(const char *name, int namelen, struct strbuf 
> *buf)
> {
>  char *cp;
> - struct branch *upstream;
> + struct branch *branch;
>  int len = interpret_nth_prior_checkout(name, buf);
>  int tmp_len;
>
> @@ -1102,26 +1102,26 @@ int interpret_branch_name(const char *name, 
> int namelen, struct strbuf *buf)
>
>  len = cp + tmp_len - name;
>  cp = xstrndup(name, cp - name);
> - upstream = branch_get(*cp ? cp : NULL);
> + branch = branch_get(*cp ? cp : NULL);
>  /*
>  * Upstream can be NULL only if cp refers to HEAD and HEAD
>  * points to something different than a branch.

Doesn't this comment also need updating, or at least clarifying any contradictions between variable names and the concepts described. That is does "Upstream" refer to the original variable 'upstream', and now the final "branch" can be confused with the new variable name.

I'm not arguing against the change, rather just noting an additional consequence.

Show 34 quoted lines

>  */
> - if (!upstream)
> + if (!branch)
>  die(_("HEAD does not point to a branch"));
> - if (!upstream->merge || !upstream->merge[0]->dst) {
> - if (!ref_exists(upstream->refname))
> + if (!branch->merge || !branch->merge[0]->dst) {
> + if (!ref_exists(branch->refname))
>  die(_("No such branch: '%s'"), cp);
> - if (!upstream->merge) {
> + if (!branch->merge) {
>  die(_("No upstream configured for branch '%s'"),
> - upstream->name);
> + branch->name);
>  }
>  die(
>  _("Upstream branch '%s' not stored as a remote-tracking branch"),
> - upstream->merge[0]->src);
> + branch->merge[0]->src);
>  }
>  free(cp);
> - cp = shorten_unambiguous_ref(upstream->merge[0]->dst, 0);
> + cp = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
>  strbuf_reset(buf);
>  strbuf_addstr(buf, cp);
>  free(cp);
> -- 
> 1.8.5.1+fc1.2.gebd1fb1
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>