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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 6, 2013, 19:16 UTC
Message-ID: <7vzjygpcd5.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1362572482-20570-4-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 12 quoted lines

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