Re: [PATCH v2] fetch: delay fetch_if_missing=0 until after config

From: Emily Shaffer <emilyshaffer@google.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2] fetch: delay fetch_if_missing=0 until after config
Date: Wed, 23 Oct 2019 16:30:26 -0700	[thread overview]
Message-ID: <20191023233026.GC124161@google.com> (raw)
In-Reply-To: <20191023214428.129593-1-jonathantanmy@google.com>

On Wed, Oct 23, 2019 at 02:44:28PM -0700, Jonathan Tan wrote:
> 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.

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

Otherwise it looks fine to me.

> ---
> No changes from v1 except that I improved the commit message.
> 
> Thanks, Emily, for taking a look.
> 
> > 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?
> 
> Yes, it is indeed .gitmodules. I improved the commit message to further
> explain what is going on.
> 
> > 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.
> 
> 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.

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