On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
<artagnon@xxxxxxxxx> wrote:
> 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@xxxxxxxxx>
> ---
> 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.
> 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
--
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