Re: [PATCH v2] bisect: don't use invalid oid as rev when starting

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> I didn't audit the following hits of get_oid_committish().  There
> might be a similar mistake as you made in v2, or there may not be.
>
> I am undecided if I should just move on, marking them as
> left-over-bits ;-)
>
>
>
> builtin/blame.c:		if (get_oid_committish(i->string, &oid))

This one throws the object name of revs to be skipped to a list, and
because revision traversal works on commit objects, if the user
gives an annotated tag and expects the underlying commit is ignored,
it may appear as a bug.  But in the same function a list of revs to
be ignored is read from file using oidset_parse_file() that in turn
uses parse_oid_hex() without even validating if the named object
exists, I would say it is OK---after all, if it hurts, the user can
refrain from doing so ;-)

But it would be nice to fix all issues around this caller.  After
collecting the object names to an oidset, somebody should go through
the list, peel them down to commit and make sure they exist, or
something like that.  A possible #leftoverbits.

> builtin/checkout.c:		repo_get_oid_committish(the_repository, branch->name, &branch->oid);

This one is probably OK as branch refs are supposed to point at
commits and not annotated tags that point at commits.

> builtin/rev-parse.c:	if (!get_oid_committish(start, &start_oid) && !get_oid_committish(end, &end_oid)) {

This one handles "rev-parse v1.0..v2.0" which gives "^v1.0 v2.0" but
using the (unpeeled) object name.  It is fine and should not be
changed to auto-peel.

> builtin/rev-parse.c:	if (get_oid_committish(arg, &oid) ||

This is immediately followed by lookup_commit_reference() to peel as
needed.  OK.

> commit.c:	if (get_oid_committish(name, &oid))

This is part of lookup_commit_reference_by_name(), which peels and
parses it down to an in-core commit object instance.  OK.


> revision.c:	if (get_oid_committish(arg, &oid))

This is followed by a loop to peel it as needed.  OK.

> sequencer.c:		    !get_oid_committish(buf.buf, &oid))

This feeds the contents of rebase-merge/stopped-sha file.  I presume
that the contents of this file (which is not directly shown to the
end users) is always a commit object name, so this is OK.  Use of
_committish() may probably be overkill for this internal bookkeeping
file.  If we stop make_patch() from shortening then probably we can
change it to parse_oid_hex() to expect and read the full object
name.

> sha1-name.c:		st = repo_get_oid_committish(r, sb.buf, &oid_tmp);
> sha1-name.c:	if (repo_get_oid_committish(r, dots[3] ? (dots + 3) : "HEAD", &oid_tmp))

Since I know those who wrote this old part of the codebase knew what
they were doing, I do not have to comment, but these are fine.  They
are all peeled to commit as appropriate by calling
lookup_commit_reference_gently() before feeding the result to
get_merge_bases().

> sha1-name.c:int repo_get_oid_committish(struct repository *r,

This is the implementation ;-)

> t/helper/test-reach.c:		if (get_oid_committish(buf.buf + 2, &oid))

This peels afterwards, so it is OK.


The true reason I went through all the callers was to see if _all_
the callers want to either ignore the resulting object name (i.e.
they want to make sure that the arg given can be peeled down to an
appropriate type) or wants the object name to be peeled to the type.
If that were the case (and from the above, it clearly isn't), we
could change the semantics of get_oid_*ish() so that the resulting
oid is already peeled down to the wanted type and that could simplify
the current callers that are peeling the result themselves.

But because some callers do not want to see the result peeled, we
shouldn't touch what get_oid_*ish() functions do.