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

[Jeff King <peff@peff.net>]

On Thu, Jan 28, 2010 at 12:02:53PM -0800, Junio C Hamano wrote:

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

I considered that, but I didn't think it was really worth it. If we
later want to make @{-foobar} meaningful, we can loosen the safety check
then.

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

You are not the only one who is puzzled. :)

But yes, your analysis of what is there now looks right to me.

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

Ooh, gross. I didn't try @{u}@{u} in my tests, but it should work.

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

I guess the original rationale was that you might have reflog'd one, so
by asking for "foo@{yesterday}" you are disambiguating as "the one with
a reflog". But that seems kind of useless to me since:

  1. It is somewhat error-prone, as it assumes that from the user's
     perspective, the fact that one ref has a log and the other does not
     is somehow a meaningful disambiguation. Which implies that users
     carefully figure out which refs have reflogs and which do not, and
     I don't think that is true.

  2. For quite a while, we have had logallrefupdates on by default (and
     I don't remember the exact semantics before that, but wasn't it
     enough to simply create a "logs" directory, which meant that you
     either logged everything or nothing?). So I don't even know how you
     would get into a situation where one ref has a log and the other
     does not.

In other words, I totally agree with your statement, and we could
probably just drop the dwim_log code.

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

Your logic makes sense to me. I think we could also simply do a
left-to-right parse, eating refs, @{-N}, and @{u} as we go and
converting them into a "real ref". If we get to something else, we stop.
If we have a @{} left, it's a reflog.  Otherwise, it's bogus (I think ^
suffixes and such have already been stripped off at this point).

Interpret_branch_name already does most of the "eating..." part above
(and it needs to remain separate from get_sha1_basic, as things like
"checkout" need to use it directly).

But I didn't really look too hard at it, as:

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

Yeah, the code was sufficiently nasty and sufficiently core that I
didn't really want to risk breaking it for the sake of cleanup
(especially not during the -rc cycle).

-Peff