patchfetch: delay fetch_if_missing=0 until after config

When running "git fetch" in a partial clone with no blobs, for example, by:

  git clone --filter=blob:none --no-checkout \
    https://kernel.googlesource.com/pub/scm/git/git
  git -C git fetch

"git fetch" will fail to load the config blob object, printing "unable to load config blob object".

This is because fetch_if_missing is set to 0 before the config is
processed. Git must set fetch_if_missing to 0 before the fetch because
as part of the fetch, packfile negotiation happens (and we do not want
to fetch any missing objects when checking existence of objects), but we
do not need to set it so early. Move the setting of fetch_if_missing to
the earliest possible point in cmd_fetch(), right before any fetching
happens.
---
This is not a full solution, but this helps in the use case described in
the commit message. The full solution probably will involve teaching the
fetch mechanism to support arbitrary struct repository objects, and by
moving fetch_if_missing into the repository object. (Alternatively, we
could add the equivalent of OBJECT_INFO_SKIP_FETCH_OBJECT to functions
like parse_commit() that are used by files like negotiator/default.c, or
split up commit parsing into object reading - which already has that
flag - and commit parsing.)
---
 builtin/fetch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On Mon, Oct 07, 2019 at 11:18:25AM -0700, Jonathan Tan wrote:

I'm having some trouble figuring out which object is actually missing. Is this the .git/config object? (That doesn't make much sense to me...) Is it .gitmodules?

Doubts aside about what's actually failing, I definitely agree with the premise of not setting this until the last moment we need it. Plus, I may be alone here, but it'd make it easier for me to understand the code if I saw a note explaining *why* we don't want to fetch_if_missing in this case.

By the way, I think I understand that this is OK to go in
unconditionally because:
 - In the full clone case, it's a no-op; we haven't got anything that's
   missing, so who cares.
 - In the filter case, it's as you said - we don't want to
   fetch_if_missing because that will turn someone's partial clone into
   a a full clone.
   - This probably applies to bare checkout, too.

Of course if I'm wrong I'd like to know, but that's how I understand it at the moment.

Ah, I remember this was listed as one of the potential intern projects - I think we dismissed it as being too tech-debt-y for an intern to feel good about. :(

The diff, though, looks fine for me.

 - Emily

Suppose, from a repository that has ".gitmodules", we clone with --filter=blob:none:

  git clone --filter=blob:none --no-checkout \
    https://kernel.googlesource.com/pub/scm/git/git

Then we fetch:

  git -C git fetch

This will cause a "unable to load config blob object", because the fetch_config_from_gitmodules() invocation in cmd_fetch() will attempt to load ".gitmodules" (which Git knows to exist because the client has the tree of HEAD) while fetch_if_missing is set to 0.

fetch_if_missing is set to 0 too early - ".gitmodules" here should be lazily fetched. Git must set fetch_if_missing to 0 before the fetch because as part of the fetch, packfile negotiation happens (and we do not want to fetch any missing objects when checking existence of objects), but we do not need to set it so early. Move the setting of fetch_if_missing to the earliest possible point in cmd_fetch(), right before any fetching happens. --- No changes from v1 except that I improved the commit message.

Thanks, Emily, for taking a look.

Yes, it is indeed .gitmodules. I improved the commit message to further explain what is going on.

Yes, that is correct. What do you mean by bare checkout? If you mean the checkout that happens after clone (that we can suppress with --no-checkout), that indeed happens after fetch_if_missing=0, so we shouldn't have a problem there.

 builtin/fetch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On Wed, Oct 23, 2019 at 02:44:28PM -0700, Jonathan Tan wrote:

I think your sign-off is missing from the new commit message, right?

Otherwise it looks fine to me.

I meant bare clone, not checkout, my apologies, but as I understand it better, they're completely separate concepts - that is, you can certainly have a bare clone which is also a full clone. So, please disregard this comment.

 - Emily

Suppose, from a repository that has ".gitmodules", we clone with --filter=blob:none:

  git clone --filter=blob:none --no-checkout \
    https://kernel.googlesource.com/pub/scm/git/git

Then we fetch:

  git -C git fetch

This will cause a "unable to load config blob object", because the fetch_config_from_gitmodules() invocation in cmd_fetch() will attempt to load ".gitmodules" (which Git knows to exist because the client has the tree of HEAD) while fetch_if_missing is set to 0.

fetch_if_missing is set to 0 too early - ".gitmodules" here should be lazily fetched. Git must set fetch_if_missing to 0 before the fetch because as part of the fetch, packfile negotiation happens (and we do not want to fetch any missing objects when checking existence of objects), but we do not need to set it so early. Move the setting of fetch_if_missing to the earliest possible point in cmd_fetch(), right before any fetching happens.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
No change from v2 except that I've added Signed-off-by.
---
 builtin/fetch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Jonathan Tan <jonathantanmy@google.com> writes:

Is it only me to feel that this is piling band-aids on top of band-aids?

Perhaps the addition (and enabling) of lazy-fetch should have been done after "checking existence" calls are vetted and sifted into two categories? Some accesses to objects are "because we need it now---so let's lazily fetch if that is available as a fallback option to us", as opposed to "if we do not have it locally right now, we want to know the fact". And each callsite should be able to declare for what reason between the two it is making the access.

The single fetch-if-missing boolean may have been a quick-and-dirty way to get the ball rolling, but perhaps the codebase grew up enough so that it is time to wean off of it?

To me, this is moving a band-aid, not adding another one. But to the bigger point...

This is one of the alternatives I mentioned in the original email [1] after "---". But to elaborate on that, I prefer sifting regions over sifting calls.

Sifting calls into two categories might work, but it's error-prone in that we would have to do the same line-by-line analysis we did when we added the repository argument to many functions, and we would have to modify functions like parse_commit() to take a flag similar to OBJECT_INFO_SKIP_FETCH_OBJECT. Also, we would have to do the same careful inspection for future patches.

Instead, we can control whether a region of code lazy-fetches by moving fetch_if_missing to the struct repository object and using a struct repository that has fetch_if_missing==0 when we don't want lazy fetching. Besides being less error-prone, the infrastructure for this has already been built (if I remember correctly, for submodules, at first).

The microproject to put fetch_if_missing into struct repository and my Outreachy project 'Refactor "git index-pack" logic into library code' [2] are steps towards this goal. (I don't think that the latter is strictly necessary, but it will make things simpler. In particular, passing "-C" then the_repository->gitdir to index-pack makes a few tests fail - not many, but not zero; and even after we resolve that, we would need CLI parameters ensuring that we marshal everything we need from the_repository, including fetch_if_missing. It seems better to libify index-pack first.)

[1] https://public-inbox.org/git/20191007181825.13463-1-jonathantanmy@google.com/ [2] https://www.outreachy.org/apply/project-selection/#git

Jonathan Tan <jonathantanmy@google.com> writes:

Fair enough.

Absolutely.  That is the price to pay for the lazy-fetch feature.

The approach "from here to there, we can set global to forbid lazy-fetch" may prolong the life support of the quick-and-dirty mechanism, but it has to assume you can say "from here to there"; it would mean that we cannot go multi-threaded until we get off of it.

That is why I said it may be time for us to wean us off of the quick-and-dirty hack that helped us bootstrapping the initial implementation of lazy clone.

By "from here to there", I meant, for example, creating a struct repository in cmd_fetch() that has fetch_if_missing=0, then passing that repository to fetch_pack() (once fetch_pack() and all functions it calls support a repository object). In that way, from here (start of fetch_pack()) to there (end of fetch_pack()) there will be no lazy fetching. As a bonus, if we ever want to support fetching in repositories other than the_repository (e.g. submodules), this change will allow us to support that too. I don't think this is quick-and-dirty, and I don't see how this prevents multithreading.

Jonathan Tan <jonathantanmy@google.com> writes:

I know---but that means the struct cannot be shared among threads that are calling object layer, some of which want to lazily fetch missing objects while others only want to check the existence, at the same time.

Compared to that, judicious use of OBJECT_INFO_SKIP_FETCH_OBJECT and other flags by callers can tell the underlying machinery why we are interested in the object, which I think is the right direction to go in the longer term. What I am not certain about is if we are ready to move to the right direction for the longer term, or we should still be relying on the big-hammer global bit for expediency a bit longer.

That's true. Mixing lazy-fetch and non-lazy-fetch in a function is rare, but admittedly it does happen - in particular, when fetching, the server may send us objects delta-ed against a missing object, and we need to lazy-fetch those missing objects.

I've taken a look at this and figured out tests that can demonstrate what exactly is being lazy-fetched (and hopefully will be good enough to prevent future code changes from adding extra lazy-fetches). In this way, maybe we can at least reduce the scope of the big-hammer global.

I just sent out a patch [1].

[1] https://public-inbox.org/git/20191101203830.231676-1-jonathantanmy@google.com/