git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: gitster@pobox.com
Cc: jonathantanmy@google.com, git@vger.kernel.org
Subject: Re: [PATCH] apply: do not fetch when checking object existence
Date: Tue, 28 Jul 2020 11:23:40 -0700	[thread overview]
Message-ID: <20200728182341.342162-1-jonathantanmy@google.com> (raw)
In-Reply-To: <xmqqwo2oe8r9.fsf@gitster.c.googlers.com>

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > There have been a few bugs wherein Git fetches missing objects whenever
> > the existence of an object is checked, even though it does not need to
> > perform such a fetch. To resolve these bugs, we could look at all the
> > places that has_object_file() (or a similar function) is used. As a
> > first step, introduce a new function has_object() that checks for the
> > existence of an object, with a default behavior of not fetching if the
> > object is missing and the repository is a partial clone. As we verify
> > each has_object_file() (or similar) usage, we can replace it with
> > has_object(), and we will know that we are done when we can delete
> > has_object_file() (and the other similar functions).
> 
> I wonder if we want to name the two (i.e. one variant that refuses
> to go to network because it is trying to see if a lazy fetch is
> needed, and the other that goes to network behind caller's back for
> ease of use in a lazy clone) a bit more distinctly so that which one
> could potentially go outside.
> 
> Depending on one's view which one is _normal_ access pattern, giving
> an explicit adverb to one variant while leaving the other one bland
> might be sufficient.  For example, I _think_ most of the places do
> not want to handle the details of lazily fetching themselves, and I
> suspect that the traditional has_object_file() semantics without "do
> not trigger lazy fetch" option would be the normal access pattern.

Right now, I think that most (if not all) places would not want to fetch
at all - so *with* "do not trigger lazy fetch" would be the normal
access pattern. This is because (in my opinion) if a caller checks the
existence of an object, it most likely can tolerate the object's
absence; if the caller couldn't tolerate it, it would just directly
query for its type or contents or something like that.

I tried to communicate this in my documentation of the deprecated
functions/macros, but perhaps it could be written better.

(One other option to consider is to just change has_object_file() to
never fetch, although I think this is more risky.)

> In which case, renaming your new "has_object" to something like
> "has_object_locally()" would be a good name for a special case
> codepath that wants to care---if the object does not exist locally
> and needs to be obtained lazily from elsewhere, the function would
> say "no".
> 
> And all the other names like has_object_file() that by default gives
> callers a transparent access to lazily fetched objects can stay the
> same.

If my analysis above is wrong, then yes I agree that we should do this.
But we might need to find another way to indicate which has_object_file()
has been checked and which hasn't - changing away from has_object_file()
completely gives us a way to indicate this, but if we're sticking with
has_object_file(), we have to find another way of indicating that we've
looked at this call and it is OK.

> > I mentioned the idea for this change here:
> > https://lore.kernel.org/git/20200721225020.1352772-1-jonathantanmy@google.com/
> 
> Yup, I think that is going in a good direction.  I suspect that
> apply will not be the only remaining case we need to "fix", and
> using the new helper function, codepaths that have already been
> "fixed" by passing "do not lazily fetch" option to the traditional
> API functions would become easier to read.  And if that is the case,
> let's have the introduction of the helper function as a separate
> patch, with each of [PATCH 2-N/N] be a fix for separate codepaths.
> 
> Thanks.

OK - I'll separate out the helper function into its own patch in version
2.

  reply	other threads:[~2020-07-28 18:24 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-28  1:04 Jonathan Tan
2020-07-28  1:19 ` Junio C Hamano
2020-07-28 18:23   ` Jonathan Tan [this message]
2020-08-05 23:06 ` [PATCH v2 0/4] No-lazy-fetch has_object() and some fixes Jonathan Tan
2020-08-05 23:06   ` [PATCH v2 1/4] sha1-file: introduce no-lazy-fetch has_object() Jonathan Tan
2020-08-05 23:06   ` [PATCH v2 2/4] apply: do not lazy fetch when applying binary Jonathan Tan
2020-08-05 23:06   ` [PATCH v2 3/4] pack-objects: no fetch when allow-{any,promisor} Jonathan Tan
2020-08-05 23:06   ` [PATCH v2 4/4] fsck: do not lazy fetch known non-promisor object Jonathan Tan
2020-08-06 20:00   ` [PATCH v2 0/4] No-lazy-fetch has_object() and some fixes Junio C Hamano

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=20200728182341.342162-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --subject='Re: [PATCH] apply: do not fetch when checking object existence' \
    /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

Code repositories for project(s) associated with this 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).