On Thu, May 2, 2013 at 1:55 PM, Ramkumar Ramachandra <[email protected]> wrote:
> Felipe Contreras wrote:
>> [...]
>
> Okay, you used nth_prior in this one.
>
>> There is no need to call this function recursively with the branch of
>> @{-N} substituted because dwim_{ref,log} already replaces it.
>
> I figured that the recursion is because dwim_{ref,log} didn't exist
> when this was written.
They did, but they were not substituting the branches.
>> @@ -447,6 +448,10 @@ static int get_sha1_basic(const char *str, int len,
>> unsigned char *sha1)
>> if (len && str[len-1] == '}') {
>> for (at = len-2; at >= 0; at--) {
>> if (str[at] == '@' && str[at+1] == '{') {
>> + if (at == 0 && str[2] == '-') {
>> + nth_prior = 1;
>> + continue;
>> + }
>
> Looking at this closely once again.
> You've already hit the beginning. What are you continuing? Take the
> example of a compound expression with @{-
Yeah, we could break, but I would prefer the break to happen naturally
when in the for loop check.
> On another note, I think you've fixed a bug: @{-1}{0} was parsing to
> the same value as @{-1}@{0} before your patch.
Yeap.
>> @@ -460,20 +465,22 @@ 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) {
>> + if (nth_prior) {
>
> nth_prior makes this much cleaner overall.
>
>> 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;
>> + int detached;
>> +
>> + if (interpret_nth_prior_checkout(str, &buf) > 0) {
>> + detached = (buf.len == 40 && !get_sha1_hex(buf.buf,
>> sha1));
>> + strbuf_release(&buf);
>> + if (detached)
>> + return 0;
>
> Neat. I'd set reflog_len to zero and made sure that the last part of
> the function wouldn't be executed. How did you get away without
> setting refs_found to 1 though?
The rest of the code is not executed, there's no need if @{-N}
evaluates to a SHA-1. There's no ref to dwim, and there's no reflog
anyway. We just fetch the SHA-1 and return.
>> }
>> + }
>> +
>> + if (!len && reflog_len)
>> /* allow "@{...}" to mean the current branch reflog */
>> refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
>
> I got this part wrong too: I said dwim_log() instead of dwim_ref().
Fortunately we are not changing the code this time, which is the best
way to make sure that the behavior doesn't change.
It took me a long time to play with alternatives and find a clean
solution with minimal changes that is easy to understand, but I think
this code does the trick.
Cheers.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
