patch, 5 partsA natural solution to the @ -> HEAD problem

> To emphasize what we're testing in @{1}@{u}, document that @{0}@{0} is
> also nonsense.  This makes it clear that @{<n>} does not resolve to a
> ref whose upstream we can determine with @{u}/ reflog we can dig with
> @{0}.
>
> Since HEAD is implicit in @{},...

> Just making sure.  HEAD@{$n} and @{$n} for non-negative $n mean
> totally different things.  @{0} and HEAD@{0} are almost always the
> same, and @{1} and HEAD@{1} may often happen to be the same, but as
> a blanket statement, I find "Since HEAD is implicit in @{}" very
> misleading.

> Junio C Hamano wrote:
> > Just making sure.  HEAD@{$n} and @{$n} for non-negative $n mean
> > totally different things.  @{0} and HEAD@{0} are almost always the
> > same, and @{1} and HEAD@{1} may often happen to be the same, but as
> > a blanket statement, I find "Since HEAD is implicit in @{}" very
> > misleading.
> 
> When will they be different?  I'm looking at this from the parser's
> point of view: when the part before @{} is missing, we dwim a "HEAD".

> On Thu, May 02, 2013 at 02:34:01AM +0530, Ramkumar Ramachandra wrote:
>
>> Junio C Hamano wrote:
>> > Just making sure.  HEAD@{$n} and @{$n} for non-negative $n mean
>> > totally different things.  @{0} and HEAD@{0} are almost always the
>> > same, and @{1} and HEAD@{1} may often happen to be the same, but as
>> > a blanket statement, I find "Since HEAD is implicit in @{}" very
>> > misleading.
>> 
>> When will they be different?  I'm looking at this from the parser's
>> point of view: when the part before @{} is missing, we dwim a "HEAD".
>
> The difference is that HEAD@{} refers to HEAD's reflog, but @{} refers
> to the reflog of the branch pointed to by HEAD. For example, try:
>
>   git init repo && cd repo
>   git commit --allow-empty -m one &&
>   git commit --allow-empty -m two &&
>   git checkout HEAD^ &&
>   git commit --allow-empty -m three &&
>   git checkout master &&
>   for i in 0 1 2; do
>     echo "HEAD@{$i}: $(git log -1 --oneline HEAD@{$i} 2>&1)"
>     echo "    @{$i}: $(git log -1 --oneline     @{$i} 2>&1)"
>   done
>
> which produces:
>
>   HEAD@{0}: 1fbb097 two
>       @{0}: 1fbb097 two
>   HEAD@{1}: 42f3f4d three
>       @{1}: 1089d0e one
>   HEAD@{2}: 1089d0e one
>       @{2}: fatal: Log for '' only has 2 entries.
>
> Unless your reflogs are screwed up, the 0th reflog should always point
> to the same commit (since you just moved HEAD there), but beyond that
> there is not necessarily any relation. And even for the 0th reflog
> entry, the reflog information is not the same. Try this on the repo
> above:
>
>   echo "HEAD@{0}: $(git log -g -1 --format='%gd %gs' HEAD@{0})"
>   echo "    @{0}: $(git log -g -1 --format='%gd %gs'     @{0})"
>
> which yields:
>
>   HEAD@{0}: HEAD@{0} checkout: moving from ... to master
>       @{0}: master@{0} commit: two
>
> -Peff

> As you and Felipe seem to be aiming for the same "Let's allow users
> to say '@' when they mean HEAD", I'll let you two figure the best
> approach out.
>
> One productive way forward might be to come up with a common test
> script pieces to document what constructs that spell @ in place of
> HEAD should be supported, and much more importantly, what constructs
> that happen to have @ in them should not mistakenly trigger the new
> machinery.

> Moreover, the symbolic-ref 'HEAD' is quite special, it's mentioned
> everywhere in the documentation, and the code has special cases for
> it. It's not reasonable to expect all relevant places to be updated
> for this functionality, and certainly 'Documentation/revisions.txt' is
> not the only one.

> For example, in Ramkumar's approach:
>
>  % git branch -u master @
>
> Would replace '@' with HEAD, however:
>
>  % git branch --edit-description @
>
> Would not.

> Felipe Contreras wrote:
>> [...]
>
> Yes, I'm working on a re-roll.
>
>> Moreover, the symbolic-ref 'HEAD' is quite special, it's mentioned
>> everywhere in the documentation, and the code has special cases for
>> it. It's not reasonable to expect all relevant places to be updated
>> for this functionality, and certainly 'Documentation/revisions.txt' is
>> not the only one.
>
> I'm not denying that HEAD is special.  I'm just improving the general
> support for symbolic-refs, so that the difference between HEAD and any
> other symbolic-ref is smaller.

>> For example, in Ramkumar's approach:
>>
>>  % git branch -u master @
>>
>> Would replace '@' with HEAD, however:
>>
>>  % git branch --edit-description @
>>
>> Would not.
>
> git branch -u master <any symbolic ref> will work just fine.  It has
> nothing to do with my series.

> Does git branch --edit-description HEAD work?  Then why do you expect
> git branch --edit-description <any other symbolic ref> to work?  git
> branch --edit-description operates on non-symbolic refs.
>
> Let me make this clear: @ is just another symbolic-ref that always
> points to the same thing as HEAD.  Nothing less, nothing more.

> Where is it documented that @ points to HEAD? Where is it documented
> that 'branch -u foo @' would replace @ with HEAD?
> Documentation/revisions.txt? Sorry, 'git branch -u foo' does not parse
> revisions, so that's not the answer. And there's many other places
> that don't do revision parsing either.

> Ramkumar Ramachandra wrote:
>> git branch X <any
>> expression with or without a symbolic ref> works fine, and it has
>> nothing to do with my series.
>
> No, it doesn't.
>
> % git symbolic-ref TEST refs/heads/master
> % git branch -u origin/master TEST
> fatal: branch 'TEST' does not exist
> % git branch --edit-description TEST
> error: No branch named 'TEST'.

>> Where is it documented that @ points to HEAD? Where is it documented
>> that 'branch -u foo @' would replace @ with HEAD?
>> Documentation/revisions.txt? Sorry, 'git branch -u foo' does not parse
>> revisions, so that's not the answer. And there's many other places
>> that don't do revision parsing either.
>
> You're confusing parsing revisions with parsing symbolic-refs.  I've
> documented @ right next to HEAD, FETCH_HEAD and the other symbolic
> refs in revisions.txt.  Yes, we have to update the documentation of
> commands like 'git branch -u foo' to make it clear that they can
> operate on symbolic-refs (not just "HEAD") that point to branches, not
> just plain branch refs.

>> Your approach is more like a hack,
>
> Now you're just saying rubbish.  Neither of the approaches is more of
> a "hack" than the other.  You've implemented @ as a revision, while
> I've implemented it as a symbolic-ref.

> Felipe Contreras wrote:
>> Your approach can NOT be documented.
>
> Ah, I missed that.  The explanation I was looking for is:
>
> HEAD has been special right from the start, and we cannot elevate
> anything else to its status now.

> Ramkumar Ramachandra wrote:
>> HEAD has been special right from the start, and we cannot elevate
>> anything else to its status now.
>
> As to why I agree with you: I audited the callers of branch_get() and
> found out there are some things that cannot be fixed just by fixing
> branch_get():
>
> For instance, 'git fetch origin M' will not work, and it's not
> branch_get()'s fault.  And that is just one instance: there are tons
> of other instances; and going and changing each one of them will take
> forever, and totally not be worth the pain.

> We probably should fix those, but that is orthogonal to the '@' shortcut.
>
> We can have the '@' shortcut *today*, with minimal changes to the code
> and the documentation, in a limited and understood scope, with no
> surprises.
>
> We can fix the symbolic ref stuff slowly, step by step, no need to
> delay the '@' shortcut for that.

> Felipe Contreras wrote:
>> We probably should fix those, but that is orthogonal to the '@' shortcut.
>>
>> We can have the '@' shortcut *today*, with minimal changes to the code
>> and the documentation, in a limited and understood scope, with no
>> surprises.
>>
>> We can fix the symbolic ref stuff slowly, step by step, no need to
>> delay the '@' shortcut for that.
>
> Agreed.

> On Thu, May 2, 2013 at 10:09 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> Felipe Contreras wrote:
>>> We probably should fix those, but that is orthogonal to the '@' shortcut.
>>>
>>> We can have the '@' shortcut *today*, with minimal changes to the code
>>> and the documentation, in a limited and understood scope, with no
>>> surprises.
>>>
>>> We can fix the symbolic ref stuff slowly, step by step, no need to
>>> delay the '@' shortcut for that.
>>
>> Agreed.
>
> If only we didn't care about windows then we could have this feature today:
>
>     cd .git
>     ln -s HEAD @
>
> ...and everything works, in the most natural way.
> Has anyone else tweaked their repo this way?
>
> Is an alternative implementation to change the template repo to ship
> with a symlink?
>
> For windows, perhaps we can use this new code behind an #ifdef?
>
> Anyways, it's just a crazy idea.  I very much like this feature,
> and in a tweaked repo @{0}{1} actually works.
>
> Is there no way to tweak this at some really low level to trick git
> into believing the link exists (even when it doesn't)?
>
> I guess that's what these patches do, but the limitations seem unfortunate.

> The @-parsing loop unnecessarily checks for the sequence "@{" from
> len - 2 unnecessarily.  We can safely check from len - 4: write out a
> comment justifying this.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  sha1_name.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 3820f28..be1d12c 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -445,7 +445,23 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>         /* basic@{time or number or -number} format to query ref-log */
>         reflog_len = at = 0;
>         if (len && str[len-1] == '}') {
> -               for (at = len-2; at >= 0; at--) {
> +               /* str = @}
> +                *       ^
> +                *       len - 2; expression is senseless
> +                *
> +                * str = @{}
> +                *       ^
> +                *       len - 3; expression is still senseless
> +                *
> +                * str = @{.}
> +                *       ^
> +                *       len - 4 where . is any character; expression
> +                *       is worth investigating
> +                *
> +                * Therefore, if str ends with }, search three
> +                * characters earlier for @{
> +                */

> On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> The @-parsing loop unnecessarily checks for the sequence "@{" from
>> len - 2 unnecessarily.  We can safely check from len - 4: write out a
>> comment justifying this.
>>
>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>> ---
>>  sha1_name.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/sha1_name.c b/sha1_name.c
>> index 3820f28..be1d12c 100644
>> --- a/sha1_name.c
>> +++ b/sha1_name.c
>> @@ -445,7 +445,23 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>>         /* basic@{time or number or -number} format to query ref-log */
>>         reflog_len = at = 0;
>>         if (len && str[len-1] == '}') {
>> -               for (at = len-2; at >= 0; at--) {
>> +               /* str = @}
>> +                *       ^
>> +                *       len - 2; expression is senseless
>> +                *
>> +                * str = @{}
>> +                *       ^
>> +                *       len - 3; expression is still senseless
>> +                *
>> +                * str = @{.}
>> +                *       ^
>> +                *       len - 4 where . is any character; expression
>> +                *       is worth investigating
>> +                *
>> +                * Therefore, if str ends with }, search three
>> +                * characters earlier for @{
>> +                */
>
> I think this comment is overkill.
>
>> +               for (at = len - 4; at >= 0; at--) {
>
> The change seems OK to me, but there's no need to explain where you
> are starting, and if there's a need:
>
> /* start from where reflogs can start: @{.} */
>
> Does the trick nicely.

> Presently, the logic for @-parsing is horribly convoluted.  Prove that
> it is very straightforward by laying out the three cases (@{N},
> @{u[upstream], and @{-N}) and explaining how each case should be
> handled in comments.  All tests pass, and no functional changes have
> been introduced.

> @@ -463,9 +463,26 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>                  */
>                 for (at = len - 4; at >= 0; at--) {
>                         if (str[at] == '@' && str[at+1] == '{') {
> +                               /* Set reflog_len only if we
> +                                * don't see a @{u[pstream]}.  The
> +                                * @{N} and @{-N} forms have to do
> +                                * with reflog digging.
> +                                */
> +
> +                               /* Setting len to at means that we are
> +                                * only going to process the part
> +                                * before the @ until we reach "if
> +                                * (reflog)" at the end of the
> +                                * function.  That is only applicable
> +                                * in the @{N} case; in the @{-N} and
> +                                * @{u[pstream]} cases, we will run it
> +                                * through interpret_branch_name().
> +                                */
> +

> @@ -476,22 +493,34 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>         if (len && ambiguous_path(str, len))
>                 return -1;
>
> -       if (!len && reflog_len) {
> -               struct strbuf buf = STRBUF_INIT;
> -               int ret;
> -               /* try the @{-N} syntax for n-th checkout */
> -               ret = interpret_branch_name(str+at, &buf);
> -               if (ret > 0) {
> -                       /* substitute this branch name and restart */
> -                       return get_sha1_1(buf.buf, buf.len, sha1, 0);
> -               } else if (ret == 0) {
> -                       return -1;
> +       if (reflog_len) {
> +               if (!len)
> +                       /* In the @{N} case where there's nothing
> +                        * before the @.
> +                        */

> +                       refs_found = dwim_log("HEAD", 4, sha1, &real_ref);
> +               else {
> +                       /* The @{N} case where there is something
> +                        * before the @ for dwim_log to figure out,
> +                        * and the @{-N} case.
> +                        */

> +                               struct strbuf buf = STRBUF_INIT;
> +                               if (interpret_branch_name(str, &buf) > 0) {
> +                                       get_sha1_hex(buf.buf, sha1);
> +                                       refs_found = 1;
> +                                       reflog_len = 0;
> +                               }
> +                               strbuf_release(&buf);

> +                       }
>                 }
> -               /* allow "@{...}" to mean the current branch reflog */
> -               refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
> -       } else if (reflog_len)
> -               refs_found = dwim_log(str, len, sha1, &real_ref);
> -       else
> +       } else
> +               /* The @{u[pstream]} case */

> On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> +                       refs_found = dwim_log(str, len, sha1, &real_ref);
>> +
>> +                       if (!refs_found && str[2] == '-') {
>
> You are changing the behavior, there's no need for that in this
> reorganization patch.

>> +                               /* The @{-N} case that resolves to a
>> +                                * detached HEAD (ie. not a ref)
>> +                                */
>
> This is not true, it resolves to a ref.
>
> git rev-parse --symbolic-full-name @{-1}

>> +                               struct strbuf buf = STRBUF_INIT;
>> +                               if (interpret_branch_name(str, &buf) > 0) {
>> +                                       get_sha1_hex(buf.buf, sha1);
>> +                                       refs_found = 1;
>> +                                       reflog_len = 0;
>> +                               }
>> +                               strbuf_release(&buf);
>
> I'm pretty sure this is doing something totally different now. This is
> certainly not "no functional changes".

>> +                       }
>>                 }
>> -               /* allow "@{...}" to mean the current branch reflog */
>> -               refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
>> -       } else if (reflog_len)
>> -               refs_found = dwim_log(str, len, sha1, &real_ref);
>> -       else
>> +       } else
>> +               /* The @{u[pstream]} case */
>
> It's not true, there might not be any @{u} in there.

> Felipe Contreras wrote:
>> On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
>> <artagnon@gmail.com> wrote:
>>> +                       refs_found = dwim_log(str, len, sha1, &real_ref);
>>> +
>>> +                       if (!refs_found && str[2] == '-') {
>>
>> You are changing the behavior, there's no need for that in this
>> reorganization patch.
>
> This is not a reorganization patch.  I said "simplify": not refactor.

>>> +                               /* The @{-N} case that resolves to a
>>> +                                * detached HEAD (ie. not a ref)
>>> +                                */
>>
>> This is not true, it resolves to a ref.
>>
>> git rev-parse --symbolic-full-name @{-1}
>
>> git co @~1; git co -; git rev-parse --symbolic-full-name @{-1}
>
> If it did resolve to a ref, dwim_log() would have found it.  The
> constraint guarding this `if (!refs_found && str[2] == '-')` wouldn't
> have been satisfied, and we wouldn't be here.

>> Also, you removed a useful comment:
>>
>> /* try the @{-N} syntax for n-th checkout */
>
> I've changed the entire logic and written expensive comments; and I'm
> not allowed to remove one comment which I didn't find helpful?

>>> +                               struct strbuf buf = STRBUF_INIT;
>>> +                               if (interpret_branch_name(str, &buf) > 0) {
>>> +                                       get_sha1_hex(buf.buf, sha1);
>>> +                                       refs_found = 1;
>>> +                                       reflog_len = 0;
>>> +                               }
>>> +                               strbuf_release(&buf);
>>
>> I'm pretty sure this is doing something totally different now. This is
>> certainly not "no functional changes".
>
> I'm claiming that there is no functional change at the program level,
> with the commenting*.  If you want to disprove my claim, you have to
> write a test that breaks after this patch.

>>> +                       }
>>>                 }
>>> -               /* allow "@{...}" to mean the current branch reflog */
>>> -               refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
>>> -       } else if (reflog_len)
>>> -               refs_found = dwim_log(str, len, sha1, &real_ref);
>>> -       else
>>> +       } else
>>> +               /* The @{u[pstream]} case */
>>
>> It's not true, there might not be any @{u} in there.
>
> This entire structure is a success-filter.  At the end of this, if
> !refs_found, then there has been a failure.

>> With the principle of self-documenting code, if you need paragraphs to
>> explain what you are doing, you already lost.
>
> The Git project is already suffering from a severe shortage of
> comments [1], and you're complaining about too many comments?

>> I'm claiming that there is no functional change at the program level,
>> with the commenting*.  If you want to disprove my claim, you have to
>> write a test that breaks after this patch.
>
> The burden of proof resides in the one that makes the claim, I don't
> need to prove that there are functional changes, merely that you
> haven't met your burden of proof.

> That being said, perhaps there are no _behavioral_ changes, because
> you are relegating some of the current functionality to dwim_log, but
> the code most certainly is doing something completely different.
> Before, get_sha1_basic recursively called itself when @{-N} resolved
> to a branch name, now, you rely on dwim_log to do this for you without
> recursion, which is nice, but functionally different.

>>> It's not true, there might not be any @{u} in there.
>>
>> This entire structure is a success-filter.  At the end of this, if
>> !refs_found, then there has been a failure.
>
> That's irrelevant, this 'else' case is for !reflog_len, there might or
> might not be @{u} in there.

>> The Git project is already suffering from a severe shortage of
>> comments [1], and you're complaining about too many comments?
>
> Irrelevant conclusion fallacy; let's suppose that the Git project is
> indeed suffering from a shortage of comments, that doesn't imply that
> *these* comments in their present form are any good.

> Felipe Contreras wrote:
>> On Wed, May 1, 2013 at 1:36 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
>>> This is not a reorganization patch.  I said "simplify": not refactor.
>>
>> I'd say you should start with a reorganization, and then a simplification.
>
> You don't think I already tried that?  There is no way to sensibly
> reorganize the old logic sensibly, in a way that doesn't break
> anything.

> On Wed, May 1, 2013 at 2:40 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
>> You don't think I already tried that?  There is no way to sensibly
>> reorganize the old logic sensibly, in a way that doesn't break
>> anything.
>
> See, I tried to split your patch into logical changes, so I started with this:
>
> --- a/sha1_name.c
> +++ b/sha1_name.c

> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -473,7 +473,7 @@ static int get_sha1_basic(const char *str, int
> len, unsigned char *sha1)
>                                 return -1;
>                         }
>                         /* allow "@{...}" to mean the current branch reflog */
> -                       refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
> +                       refs_found = dwim_log("HEAD", 4, sha1, &real_ref);

>> There's no need to associate one comment with one line of code.
>> People can see clearly see the failure case following it.
>
> Is that the way you defend your comments? People can see that the
> comment is wrong?

>>> There's no need to associate one comment with one line of code.
>>> People can see clearly see the failure case following it.
>>
>> Is that the way you defend your comments? People can see that the
>> comment is wrong?
>
> In that case, all the comments are wrong.  Even the ones about @{N}
> and @{-N}, because we never really check @{\d+} or @{-\d+}.  Would you
> like to make the comments more painful to read and write?

> +               if (!len)
> +                       /* In the @{N} case where there's nothing
> +                        * before the @.
> +                        */
> +                       refs_found = dwim_log("HEAD", 4, sha1, &real_ref);

> 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);
> +       }

>         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, "@{");

>>         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.

> Felipe Contreras wrote:
>> 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?
>
> Why is there a commit message?  I've explained what the behavior change is.

>> 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.
>
> For what exactly.  To fix a real bug: H@{u} and @@{u} don't work where
> either H or @ are symbolic refs.  I want custom symbolic refs, because
> they are useful.  In other words, "HEAD" is not a sacred symbolic ref.

> Felipe Contreras wrote:
>> As I said, the @@{u} thing can be fixed through other ways.
>
> It's not just @@{u}.  I can have lots of custom symbolic refs working
> properly; I might choose D for my deleted-reflogs, M for master and so
> on.

> The point is that my solution solves the problem in the more general case.
>
>> Moreover, "HEAD" is still a special case in remote.c::branch_get()
>> that you just modified.
>
> It's not.  It's just a quick shortcut for the common case because
> read_config() already sets it up.

> Felipe Contreras wrote:
>> If you are arguing in favor of arbitrary symbolic refs plus @{u} to
>> work, a patch that allows that, and nothing else, should make sense.
>> Such patch would trigger further discussion, for example, if
>> get_branch() is the right place to achieve that.
>
> What kind of absurd argument is this?  I am trying to solve the @ ->
> HEAD problem correctly.  In the process, I fixed general symbolic-ref
> handling and cleaned up the @-parsing logic.  Does anyone have a
> problem with the fix, or am I missing something?

> So, start the discussion.  What are you waiting for?
>
>> I still see this in the code:
>>
>>         if (!name || !*name || !strcmp(name, "HEAD"))
>
> Remove the !strcmp(name, "HEAD") and tell me what you see.  "HEAD"
> will get resolved just like any other symbolic ref, via the hunk I
> added.

> First, make sure that check_refname_format() rejects the a refname
> beginning with a '@'.  Add a test to t1400 (update-ref) demonstrating
> that update-ref forbids the user from updating a ref named "@".
>
> Now, resolve_ref_unsafe() is built to resolve any refs that have a
> corresponding file inside $GITDIR.  Our "@" ref is a special
> pseudo-ref and does not have a filesystem counterpart.  So,
> hard-interpret "@" as "HEAD" and resolve .git/HEAD as usual.  This
> means that we can drop the 'git symbolic-ref @ HEAD' line in t1508
> (at-combinations), and everything will continue working as usual.
>
> If the user does manage to create a '.git/@' unsafely (via
> symbolic-ref or otherwise), it will be ignored.
>
> In practice, this means that you will now be able to do:
>
>     $ git show @~1
>     $ git log @^2
>
> Advertise these features in the tests and documentation.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  Documentation/git-check-ref-format.txt |  2 ++
>  Documentation/revisions.txt            |  8 ++++++--
>  refs.c                                 | 12 ++++++++++--
>  t/t1400-update-ref.sh                  |  3 +++
>  t/t1508-at-combinations.sh             |  7 ++++---
>  5 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
> index ec1739a..3de9adc 100644
> --- a/Documentation/git-check-ref-format.txt
> +++ b/Documentation/git-check-ref-format.txt
> @@ -52,6 +52,8 @@ Git imposes the following rules on how references are named:
>
>  . They cannot end with a dot `.`.
>
> +. They cannot be the single character `@`.
> +
>  . They cannot contain a sequence `@{`.
>
>  . They cannot contain a `\`.
> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
> index d477b3f..9b2e653 100644
> --- a/Documentation/revisions.txt
> +++ b/Documentation/revisions.txt
> @@ -27,6 +27,10 @@ blobs contained in a commit.
>    When ambiguous, a '<refname>' is disambiguated by taking the
>    first match in the following rules:
>
> +  . '@' is a special pseudo-ref that refers to HEAD.

>> An '@' followed
>> +    by '\{' has no relationship to this and means something entirely
>> +    different (see below).
>
> If the user cares about that, the user can see that below, otherwise
> there's no point in mentioning that.

> Felipe Contreras wrote:
>> Does the user really cares if it's a pseudo-ref or not? Also, what
>> does it mean that "refers" to HEAD?
>
> It's not about whether the user cares or not; it's about saying it in
> a way that doesn't make it less precise.  I'm saying "@ is like a
> symbolic-ref .git/@ ref referring to HEAD, except it doesn't sit on
> the filesystem".

> Ramkumar Ramachandra (5):
>   t1508 (at-combinations): more tests; document failures
>   sha1_name.c: don't waste cycles in the @-parsing loop
>   sha1_name.c: simplify @-parsing in get_sha1_basic()
>   remote.c: teach branch_get() to treat symrefs other than HEAD
>   refs.c: make @ a pseudo-ref alias to HEAD