Re: [PATCH 3/3] reject @{-1} not at beginning of object name

Jeff King [off-list ref] writes:

Something like foo@{-1} is nonsensical, as the @{-N} syntax
is reserved for "the Nth last branch", and is not an actual
reflog selector. We should not feed such nonsense to
approxidate at all.

Signed-off-by: Jeff King <redacted>
---
We didn't discuss this one, but I came across it while trying to be
complete in testing the combinations. Right now "foo@{-1}" is
interpreted as a reflog entry at approxidate "-1". Approxidate doesn't
signal an error because it thinks it has found something useful. But
AFAIK we have declared all @{-...} to be Nth last branch, so it is
simply a semantic error.

Let me know if that is not the case (that is, if it was intentional to
leave foo@{-1} as the reflog at date "-1" because it has some meaning
that I am missing) and we can drop this patch.

I think the patch is fine as is.

We might want to use @{-some string that has non digit} for other purposes
and it may be a safer change to tweak the "do we only have digits" check
in the post-context to detect and reject only @{-<all digits>}.

But what I am puzzled by the code structure of get_sha1_basic(), which
looks like this:

    get_sha1_basic() {

     - do we have @{...} at end?  If so, and if it is not a magic like
       @{u}, set "len" (points at the end of stuff that should name a ref)
       and "reflog_len" (the length of the reflog time/num specifier that
       is applied to that ref).

     - did we find @{...} in the above check, and is it at the beginning?
       Then it is not a reflog syntax, but is a N-th branch switch.
       Substitute that @{...} with the real refname and retry.  If it is
       not @{-N}, then that @{...} reflog derefence should apply to the
       current branch, so set it to real_ref and go to "reflog" part.

     - if we have @{...} at the end, get the canonical name of the ref the
       reflog notation is applied to.

     - otherwise, get the canonical name of the ref; in this case,
       there is no @{...} at the end, si this is what is eventually
       returned.

     - "reflog" part:
       by now, real_ref holds the ref @{...} is being applied to.  Read
       from its reflog.

   }

And the place that parses @{-1} and @{u} are different, even though both
dwim_log() called by the third one and dwim_ref() called by the fourth one
call substitute_branch_name() and they are perfectly capable of resolving
@{-1} and @{u} (and even nested stuff like @{-1}@{u}@{u} with your patch).

But somehow we kept the special case code to parse @{-1} in the second
one.

    Side note.  I am wondering if dwim_log()'s current implementation is
    even correct in the first place.  When you have two "ambiguous" refs,
    it appears to me that you will get a warning from dwim_ref(), but if
    only one of them has a reflog associated with it, dwim_log() won't
    complain.  Why isn't the function be (1) dwim_ref() to find the ref
    from abbreviated refname given in str; and then (2) check if the log
    exists for that ref?

It might be cleaner if the logic went like this instead:

    - find the last @{...} in the string that is not what i-b-n should
      resolve (i.e. @{-1} and @{u}); that is @{time/num} reflog
      reference.  You can have at most one reflog reference and it always
      has to come at the end.

    - feed the remainder to (an updated) i-b-n that knows how to grok:

      - @{-N} is nth-priour checkout; it has to come at the beginning and
        you can have at most one.  If you find it, substitute it with the
        real branch name and continue. (e.g. @{-1}@{u} becomes master@{u})

      - does it begin with @{...}?  If not, the part before @{...} is the
        name of the ref (e.g. "next" in "next@{u}@{u}") the later magic
        sequence (e.g. "@{u}@{u}") is applied to; otherwise apply the
        magic to what HEAD points at (e.g. "@{u}" applies @{u} to the
        current branch).  Remember that ref and strip it away from the
        input.

      - while we see sequence of @{...}, apply the magic to the ref
        repeatedly (e.g. "next@{u}@{u}" has remembered "refs/heads/next" in
        the previous step, and @{u} is applied to produce "master" if next
        follows master, and then applying @{u} to that result will tell us
        that it follows "refs/remotes/origin/master").

    - now we have what ref the caller was talking about with the beginning
      part of the input (i.e. without "@{time/num}" at the end).  If we
      had @{time/num} in the original input, open the reflog and find an
      appropriate entry in it.  Otherwise what we received from (the
      updated) i-b-n is what we found.  Find out what object the ref
      points at.

But that is a kind of code churn that may not be worth doing.  I dunno.