On Thu, Mar 7, 2013 at 2:16 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclo...@gmail.com> writes:
>> +static void wt_status_get_detached_from(struct wt_status_state *state)
>> +{
>> +     struct grab_1st_switch_cbdata cb;
>> +     struct commit *commit;
>> +     unsigned char sha1[20];
>> +     char *ref = NULL;
>> +
>> +     strbuf_init(&cb.buf, 0);
>> +     if (for_each_recent_reflog_ent("HEAD", grab_1st_switch, 4096, &cb))
>> +             for_each_reflog_ent("HEAD", grab_1st_switch, &cb);
>> +     if (!cb.buf.len)
>> +             return;
> Is this correct?  What if the recent entries (i.e. the tail 4k) of
> the HEAD reflog did not have *any* checkout?  Your callback never
> returns non-zero, so as long as the HEAD reflog is sufficiently
> long, for_each_recent_reflog_ent() will return 0 to signal success,
> and you do not dig deeper by retrying the full reflog for HEAD,
> missing the checkout that exists before the final 4k, no?
> It should be more like this, I would think:
>         for_each_recent_reflog_ent();
>         if (!found)
>                 for_each_reflog_ent();
>         if (!found)
>                 return;

Yes. This "recent" optimization is tricky.

> Using cb.buf.len as the "found" marker may be correct, but I found
> it a bit subtle to my taste, without explanation.  Adding an
> explicit bit to "struct grab_1st_switch_cbdata" would be cleaner and
> more resistant to future changes, I think.


>> +
>> +     if (dwim_ref(cb.buf.buf, cb.buf.len, sha1, &ref) == 1 &&
>> +         (commit = lookup_commit_reference_gently(sha1, 1)) != NULL &&
>> +         !hashcmp(cb.nsha1, commit->object.sha1)) {
> That feels unnecessarily expensive.  Why not hashcmp before checking
> the type of the object to reject the case where the ref moved since
> the last checkout early?
> For that matter, does this even need to check the type of the object
> that currently sits at the ref?  Isn't it sufficient to reject this
> case by seeing if sha1 is the same as cb.nsha1?

nsha1 is always a commit sha-1, sha-1 could be a tag sha-1 that refers
to the same commit. hashcmp before lookup is a good idea. Although I
don't think it's expensive in the big picture. When HEAD is not
detached, we show "<n> commits ahead of @{u}" which is way more
expensive than this. As long as "git status" on detached HEAD does not
use up all the time that "git status" on branches normally does, I
think we're fine.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to