Re: [PATCH 4/5] remote.c: teach branch_get() to treat symrefs other than HEAD

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: May 1, 2013, 18:16 UTC
Message-ID: <CAMP44s20F-QALd18VPHLF4Fj=eFFvXmkhC4XK__kxNhMoeN=ug@mail.gmail.com>
In-Reply-To: <1367425235-14998-5-git-send-email-artagnon@gmail.com>

On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 45 quoted lines

> Currently, branch_get() either accepts either a branch name, empty
> string, or the magic four-letter word "HEAD".  Make it additionally
> handle symbolic refs that point to a branch.
>
> Update sha1_name.c:interpret_branch_name() to look for "@{", not '@'
> (since '@' is a valid symbolic ref).
>
> These two changes together make the failing test in t1508
> (at-combinations) pass.  In other words, you can now do:
>
>     $ git symbolic-ref @ HEAD
>
> And expect the following to work:
>
>     $ git rev-parse @@{u}
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  remote.c                   | 14 ++++++++++++++
>  sha1_name.c                |  2 +-
>  t/t1508-at-combinations.sh |  2 +-
>  3 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 68eb99b..0f44e2e 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1470,6 +1470,20 @@ struct branch *branch_get(const char *name)
>                 ret = current_branch;
>         else
>                 ret = make_branch(name, 0);
> +
> +       if (name && *name && (!ret || !ret->remote_name)) {
> +               /* Is this a symref pointing to a valid branch, other
> +                * than HEAD?
> +                */
> +               const char *this_ref;
> +               unsigned char sha1[20];
> +               int flag;
> +
> +               this_ref = resolve_ref_unsafe(name, sha1, 0, &flag);
> +               if (this_ref && (flag & REF_ISSYMREF) &&
> +                       !prefixcmp(this_ref, "refs/heads/"))
> +                       ret = make_branch(this_ref + strlen("refs/heads/"), 0);
> +       }

But why? I'm not familiar with branch_get, but my intuition tells me you are changing the behavior, and now branch_get() is doing something it wasn't intended to do. And for what?

Your rationale is that it fixes the test cases below, but that's not reason enough, since there are other ways to fix them, as my patch series shows.

Show 13 quoted lines

>         if (ret && ret->remote_name) {
>                 ret->remote = remote_get(ret->remote_name);
>                 if (ret->merge_nr) {
> diff --git a/sha1_name.c b/sha1_name.c
> index f30e344..c4a3a54 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -1060,7 +1060,7 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
>                 return ret - used + len;
>         }
>
> -       cp = strchr(name, '@');
> +       cp = strstr(name, "@{");

This might make sense, but it feels totally sneaked in.

>         if (!cp)
>                 return -1;
>         tmp_len = upstream_mark(cp, namelen - (cp - name));

I think these are two patches should be introduced separately, and with a reason for them to exist independent of each other.

Cheers.

-- 
Felipe Contreras