Re: [PATCH v2 3/4] status: show more info than "currently not on any branch"

From: Duy Nguyen <pclouds@gmail.com>
Date: Mar 8, 2013, 11:04 UTC
Message-ID: <CACsJy8CnDoMRzR5ifH3+0AKRLfzrNpkc3-NOxoA85SiRUb40ng@mail.gmail.com>
In-Reply-To: <7vzjygpcd5.fsf@alter.siamese.dyndns.org>

On Thu, Mar 7, 2013 at 2:16 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 29 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@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.

OK

Show 13 quoted lines

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