On Thu, Mar 7, 2013 at 2:16 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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.
OK
>
>> +
>> + 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.
--
Duy
--
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