[Question] Is extensions.partialClone defunct?

I was looking into an issue around partial clone in v2.26.0-rc1, but found several references in the code and documentation to "extensions.partialClone".

Here is the blurb from Documentation/technical/repository-version.txt:

  ==== `partialclone`

  When the config key `extensions.partialclone` is set, it indicates
  that the repo was created with a partial clone (or later performed
  a partial fetch) and that the remote may have omitted sending
  certain unwanted objects.  Such a remote is called a "promisor remote"
  and it promises that all such omitted objects can be fetched from it
  in the future.

  The value of this key is the name of the promisor remote.

and this error message in builtin/fetch.c:

	if (filter_options.choice && !has_promisor_remote())
		die("--filter can only be used when extensions.partialClone is set");

but it appears that we rely on the "remote.<name>.promisor = true" setting instead of this extension.

Indeed, if we run `git clone --filter=blob:none <url>" the resulting .git/config file is as follows:

[core]
        repositoryformatversion = 1
        filemode = false
        bare = false
        logallrefupdates = true
        symlinks = false
        ignorecase = true
[remote "origin"]
        url = <url>
        fetch = +refs/heads/*:refs/remotes/origin/*
        promisor = true
        partialclonefilter = blob:none
[branch "master"]
        remote = origin
        merge = refs/heads/master

So, I thought I would put this up on the list to point out the inconsistency, in case someone with more context has thoughts on the correct way forward here.

Thanks, -Stolee

[snip]

Hmm...besides giving the name of the promisor remote, the extensions.partialClone setting is there to prevent old versions of Git (that do not know this extension) from manipulating the repo.

I cloned and indeed it is as Stolee describes. Git still works as expected if I remove "promisor = true" and add "[extensions]\npartialclone=origin", so at least extensions.partialClone is still supported, even if not written by default.

Thanks for bringing this up.

Jonathan Tan wrote:

Yes, so the lack of setting is a bug.

Christian, what would your prefered way be to fix this? Should extensions.partialclone specify a particular "default" promisor remote, or should we use a new repository extension for multiple promisors?

[...]

Thanks for investigating.

Sincerely, Jonathan

Jonathan Nieder <jrnieder@gmail.com> writes:

It would depend on how well versions of Git that supported one promisor with extensions.partialClone works with a repository initialized with Christian's multi promisors when the extension points at only a single promisor. Is having other/secondary promisors meant merely to be performance thing, or would the repository be broken if these promisors are down because some objects are never available from the "origin" or the primary promisor remote? If the former, using a "default" promisor with extensions.partialClone would be sufficient and be preferred as we do not end up wasting a new extension, but if the latter, we would need to prevent Git that is capable of single-promisor from touching the multi-promisor repository with a new extension.

On Thu, Mar 12, 2020 at 10:51:51AM -0700, Jonathan Nieder wrote:

Manipulating it how?

This ambiguity makes me think that 'extensions.partialClone' is an awkward fit for repositories that have multiple promisor remotes.

Thanks, Taylor

Taylor Blau <me@ttaylorr.com> writes:

Presumably if the version of Git does not assume that sometimes missing objects are OK, its "fsck" and "repack" would become very upset when a repository lacks very many objects because the version of Git that created it assumes they can be lazily fetched, no?

On Thu, Mar 12, 2020 at 02:17:40PM -0700, Junio C Hamano wrote:

Yes (or even just "git log", which will barf when it hits a needed object). Unlike some other extensions, I don't think that case would be _dangerous_ (i.e., for any case where it matters the old version is likely to abort, thinking it's seeing corruption, rather than writing out a subtly wrong result).

But it is certainly more pleasant for the user to see "we don't support the partialClone extension" than a spew of "missing object" messages.

-Peff

On Thu, Mar 12, 2020 at 6:51 PM Jonathan Nieder <jrnieder@gmail.com> wrote:

That could be true of "remote.<name>.promisor = true".

My opinion has always been that "extensions.partialClone = <remote>" is a mistake as it is inherently making difficult to have many promisor remotes.

It is also not clear what a default should be. Should it be the last promisor remote that is tried when all the others couldn't provide all the needed objects? Or should it be the first one that is tried? And what if the order should be different when pushing and when fetching?

I think it is much clearer to have an order (that could be different when pushing and fetching) in which promisor remotes are tried.

So I'd rather obsolete "extensions.partialClone = <remote>" and to find other ways.

Yeah, I didn't remove support for it. Maybe we could have "[extensions]\npartialclone=<bool>" though that feels messy, or maybe we could have another extension alltogether like "[extensions]\npromisorremotes=<bool>" and over time obsolete "extensions.partialClone" altogether. I prefer the later.

On Thu, Mar 12, 2020 at 10:54 PM Christian Couder <christian.couder@gmail.com> wrote:

I mean for future versions, not for old versions.

Hi,

Christian Couder wrote:

To be clear, Jonathan is referring to a specific feature of the extensions.* settings. See Documentation/technical/repository-version.txt for details:

  2. If a version-1 repository specifies any `extensions.*` keys that
     the running git has not implemented, the operation MUST NOT
     proceed. Similarly, if the value of any known key is not understood
     by the implementation, the operation MUST NOT proceed.

No other config key has that property.

[...]

[...]

I *think* that means "new repository extension".

That suggests something like

	[extensions]
		multiplePromisors = true

[...]

I think we're going to have to continue to support extensions.partialClone=<remote> for a long time anyway (breaking the ability to work with existing repositories is expensive), so I'm reasonably comfortable with multiplePromisors being a separate extension. Some faraway day, we can introduce "repositoryFormatVersion = 2" that mandates support for these extensions by default, allowing us to clean up and simplify.

I can start writing a proposed patch to send this evening or tomorrow. This behavior has been around for a few releases so it would want to cook until the 2.27 cycle.

Thanks, Jonathan

Hi,

On Fri, Mar 13, 2020 at 12:09 AM Jonathan Nieder <jrnieder@gmail.com> wrote:

Yeah, right. I was very tired and it was late when I replied, so I was confused.

Yeah.

Yeah, I think I suggested "[extensions]\npromisorremotes=<bool>" but "multiplePromisors" is probably better.

Yeah, I agree.

That would be very much appreciated! Thanks!

Yeah, and partial clone is experimental, so I think it's ok.

Hi folks, my apologies if old thread reanimation is frowned upon, I have not been able to find any indications that this is the case.

On Sat, Mar 14, 2020 at 6:43 AM Christian Couder <christian.couder@gmail.com> wrote:

I was going to start a new thread about just this topic today, but learned to use the archives instead, for better or for worse...

Was this change ever attempted? Git's current behavior (as of 2.31.1) appears to still violate the semantics of core.repositoryformatversion=1 as documented at https://github.com/git/git/blob/master/Documentation/technical/repository-version.txt, and this has been the case since git 2.24. I assume the right fix at this point would be to do something like auto-detecting promisor remotes and/or packfiles and adding the extensions.partialClone config key automatically/transparently. The main question seems to be *what value* the config key should hold, if there are multiple promisor remotes?

I'm a little confused by suggestions to create a *new* extension key
here. In principle, this would mean that existing repositories
created/updated by the newest git version would declare themselves to
be incompatible with git clients that don't understand this
entirely-new key, even though:
 * promisor packfiles are a reasonably longstanding thing now (several
years old),
 * they have officially/theoretically been associated with the
"partialClone" extension key throughout that time (at least in the
docs), and
 * partial clone is no longer considered "experimental" in any public doc,

The kind of sudden backwards-incompatibility implied by a new "extensions.*" key seems... bad? (unless it's something that you *newly* opt in to - which doesn't seem to be the case here)

Thanks, Tao

Christian Couder <christian.couder@gmail.com> writes:

Is there a reason why extensions.partialClone cannot be multi-valued, just like say "remote.origin.fetch" is?