git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: jrnieder@gmail.com
Cc: jonathantanmy@google.com, git@vger.kernel.org, gitster@pobox.com
Subject: Re: [PATCH v2] revision: allow missing promisor objects on CLI
Date: Thu,  2 Jan 2020 12:49:17 -0800	[thread overview]
Message-ID: <20200102204917.188987-1-jonathantanmy@google.com> (raw)
In-Reply-To: <20191231000957.GB13606@google.com>

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

OK.

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

[snip]

Ah, good catch. It should be "If parse_object() returns NULL, ..."

> By the way, why doesn't parse_object itself check the commit graph for
> commit objects?

Because that's how I coded it in ec0c5798ee ("revision: use commit graph
in get_reference()", 2018-12-28). In the commit message, I said:

> Another way to accomplish this effect would be to modify parse_object()
> to use the commit graph if possible; however, I did not want to change
> parse_object()'s current behavior of always checking the object
> signature of the returned object.

But that doesn't mean that we cannot change it.

> By "and treats it as missing" does this mean "and we should treat it
> as missing"?  (Forgive my ignorance.)

I don't fully know if we should, hence my specific wording :-P but see
my answer to your next question.

> Why do we treat corrupt objects as missing?  Is this for graceful
> degredation when trying to recover data from a corrupt repository?

This (at least, treating wrong-hash objects the same as missing) has
been true since acdeec62cb ("Don't ever return corrupt objects from
"parse_object()"", 2007-03-20) (and that commit message has no
explanation). I think this is best - I consider corrupt objects to be
similar to missing objects with respect to repository corruption, so for
me it is logical to treat them the same way.

  reply	other threads:[~2020-01-02 20:49 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-28  0:34 [PATCH] revision: allow missing promisor objects on CLI Jonathan Tan
2019-12-28  3:50 ` Junio C Hamano
2019-12-30 18:38   ` Jonathan Tan
2019-12-30 20:33     ` Junio C Hamano
2019-12-30 23:44 ` [PATCH v2] " Jonathan Tan
2019-12-31  0:09   ` Jonathan Nieder
2020-01-02 20:49     ` Jonathan Tan [this message]
2020-01-11 22:34 ` [PATCH v3 0/2] Un-regress rev-list --exclude-promisor-objects Jonathan Tan
2020-01-11 22:34   ` [PATCH v3 1/2] revision: document get_reference() Jonathan Tan
2020-03-25 20:46     ` Emily Shaffer
2020-01-11 22:34   ` [PATCH v3 2/2] revision: un-regress --exclude-promisor-objects Jonathan Tan
2020-03-25 20:50     ` Emily Shaffer

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=20200102204917.188987-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).