archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <>
To: Christian Couder <>
Cc:, Christian Couder <>,
	Miriam Rubio <>,
	Johannes Schindelin <>
Subject: Re: [PATCH v2] bisect: don't use invalid oid as rev when starting
Date: Thu, 24 Sep 2020 13:53:50 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <> (Junio C. Hamano's message of "Thu, 24 Sep 2020 12:56:11 -0700")

Junio C Hamano <> 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

> 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

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

  reply	other threads:[~2020-09-24 20:53 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-23 17:09 [PATCH] " Christian Couder
2020-09-23 17:27 ` Junio C Hamano
2020-09-23 20:37 ` Johannes Schindelin
2020-09-23 21:05   ` Johannes Schindelin
2020-09-23 21:39     ` Junio C Hamano
2020-09-24  6:10       ` Christian Couder
2020-09-24  6:48         ` Junio C Hamano
2020-09-24  7:51         ` Johannes Schindelin
2020-09-24 16:39           ` Junio C Hamano
2020-09-24 18:38             ` Junio C Hamano
2020-09-25  7:13               ` Johannes Schindelin
2020-09-25  7:14                 ` Johannes Schindelin
2020-09-25 16:54                 ` Junio C Hamano
2020-09-24  6:03 ` [PATCH v2] " Christian Couder
2020-09-24  7:49   ` Johannes Schindelin
2020-09-24 11:08     ` Christian Couder
2020-09-24 16:44       ` Junio C Hamano
2020-09-24 18:55   ` Junio C Hamano
2020-09-24 19:25     ` Junio C Hamano
2020-09-24 19:56     ` Junio C Hamano
2020-09-24 20:53       ` Junio C Hamano [this message]
2020-09-25 13:09     ` Christian Couder
2020-09-25 13:01   ` [PATCH v3] " Christian Couder

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \
    --subject='Re: [PATCH v2] bisect: don'\''t use invalid oid as rev when starting' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).