Re: [PATCH v2 3/4] status: show more info than "currently not on any branch"
Duy Nguyen writes: >> 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. Not really. What is tricky is that reflog is an append-only file and we only have an API to let us read it in the oldest to newer order, which is natural for the file format, but unsuited for the purpose of finding out nth most recent anything. See the other thread I am going to send out soon on this. -- 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
Re: [PATCH v2 3/4] status: show more info than "currently not on any branch"
On Thu, Mar 7, 2013 at 2:16 AM, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy 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 " 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] status: show more info than "currently not on any branch"
Nguyễn Thái Ngọc Duy 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;
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?
--
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
