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