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;
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html