Re: [PATCH v2] revision: allow missing promisor objects on CLI
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Dec 31, 2019, 00:09 UTC
- Message-ID
- <20191231000957.GB13606@google.com>
- In-Reply-To
- <20191230234453.255082-1-jonathantanmy@google.com>
Jonathan Tan wrote:
Show 13 quoted lines
> Commit 4cf67869b2 ("list-objects.c: don't segfault for missing cmdline
> objects", 2018-12-06) prevented some segmentation faults from occurring
> by tightening handling of missing objects provided through the CLI: if
> --ignore-missing is set, then it is OK (and the missing object ignored,
> just like one would if encountered in traversal).
>
> However, in the case that --ignore-missing is not set but
> --exclude-promisor-objects is set, there is still no distinction between
> the case wherein the missing object is a promisor object and the case
> wherein it is not. This is unnecessarily restrictive, since if a missing
> promisor object is encountered in traversal, it is ignored; likewise it
> should be ignored if provided through the CLI. Therefore, distinguish
> between these 2 cases. (As a bonus, the code is now simpler.)nit about tenses, not worth a reroll on its own: "As a bonus, this makes the code simpler" (since commit messages describe the status quo before the patch in the present tense).
[...]
Show 15 quoted lines
> --- a/revision.c
> +++ b/revision.c
> @@ -370,8 +370,18 @@ static struct object *get_reference(struct rev_info *revs, const char *name,
> if (!repo_parse_commit(revs->repo, c))
> object = (struct object *) c;
> else
> + /*
> + * There is something wrong with the commit.
> + * repo_parse_commit() will have already printed an
> + * error message. For our purposes, treat as missing.
> + */
> object = NULL;
> } else {
> + /*
> + * There is something wrong with the object. parse_object()If we got here, we are in the !commit case, which is not inherently wrong, right? Is the intent something like the following?
if (oid_object_info(...) == OBJ_COMMIT && !repo_parse_commit(...)) {
... good ...
} else if (parse_object(...)) {
... good ...
} else {
/*
* An error occured while parsing the object.
* parse_commit or parse_object has already printed an
* error message. For our purposes, it's safe to
* assume the object as missing.
*/
object = NULL;
}This might be easiest to understand as a separate mini-function. Something like
/*
* Like parse_object, but optimized by reading commits from the
* commit graph.
*
* If the repository has commit graphs, repo_parse_commit() avoids
* reading the object buffer, so use it whenever possible.
*/
static struct object *parse_object_probably_commit(
struct repository *r, const struct object_id *oid)
{
struct commit *c;if (oid_object_info(r, oid, NULL) != OBJ_COMMIT) return parse_object(r, oid);
c = lookup_commit(r, oid); if (repo_parse_commit(r, c)) return NULL; return (struct object *) c; }
static struct object *get_reference(struct rev_info *revs, ...)
{
struct object *object = parse_object_probably_commit(revs->repo, oid);
if (!object)
/*
* An error occured while parsing the object.
* parse_object_probably_commit has already printed an
* error message. For our purposes, it's safe to
* assume the object as missing.
*/
;By the way, why doesn't parse_object itself check the commit graph for commit objects?
[...]
Show 8 quoted lines
> @@ -1907,7 +1917,18 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi > verify_non_filename(revs->prefix, arg); > object = get_reference(revs, arg, &oid, flags ^ local_flags); > if (!object) > - return revs->ignore_missing ? 0 : -1; > + /* > + * If this object is corrupt, get_reference() prints an error > + * message and treats it as missing.
By "and treats it as missing" does this mean "and we should treat it as missing"? (Forgive my ignorance.)
Why do we treat corrupt objects as missing? Is this for graceful degredation when trying to recover data from a corrupt repository?
Thanks, Jonathan