[Question] Is extensions.partialClone defunct?

[Question] Is extensions.partialClone defunct?

[Derrick Stolee]

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

Re: [Question] Is extensions.partialClone defunct?

[Jonathan Tan]

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

[snip]

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

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.

> 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

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.

> 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 for bringing this up.

Re: [Question] Is extensions.partialClone defunct?

[Jonathan Nieder]

Jonathan Tan wrote:
> Derrick Stolee wrote:

>> but it appears that we rely on the "remote.<name>.promisor = true"
>> setting instead of this extension.
>
> 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.

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?

[...]
> 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 investigating.

Sincerely,
Jonathan

Re: [Question] Is extensions.partialClone defunct?

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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.

Re: [Question] Is extensions.partialClone defunct?

[Taylor Blau]

On Thu, Mar 12, 2020 at 10:51:51AM -0700, Jonathan Nieder wrote:
> Jonathan Tan wrote:
> > Derrick Stolee wrote:
>
> >> but it appears that we rely on the "remote.<name>.promisor = true"
> >> setting instead of this extension.
> >
> > 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.

Manipulating it how?

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

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

> [...]
> > 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 investigating.
>
> Sincerely,
> Jonathan

Thanks,
Taylor

Re: [Question] Is extensions.partialClone defunct?

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> On Thu, Mar 12, 2020 at 10:51:51AM -0700, Jonathan Nieder wrote:
>> Jonathan Tan wrote:
>> > Derrick Stolee wrote:
>>
>> >> but it appears that we rely on the "remote.<name>.promisor = true"
>> >> setting instead of this extension.
>> >
>> > 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.
>
> Manipulating it how?

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?

Re: [Question] Is extensions.partialClone defunct?

[Jeff King]

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

> Taylor Blau <me@ttaylorr.com> writes:
> 
> > On Thu, Mar 12, 2020 at 10:51:51AM -0700, Jonathan Nieder wrote:
> >> Jonathan Tan wrote:
> >> > Derrick Stolee wrote:
> >>
> >> >> but it appears that we rely on the "remote.<name>.promisor = true"
> >> >> setting instead of this extension.
> >> >
> >> > 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.
> >
> > Manipulating it how?
> 
> 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?

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

Re: [Question] Is extensions.partialClone defunct?

[Christian Couder]

On Thu, Mar 12, 2020 at 6:51 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Jonathan Tan wrote:
> > Derrick Stolee wrote:
>
> >> but it appears that we rely on the "remote.<name>.promisor = true"
> >> setting instead of this extension.
> >
> > 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.

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

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

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.

> [...]
> > 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.

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.

Re: [Question] Is extensions.partialClone defunct?

[Christian Couder]

On Thu, Mar 12, 2020 at 10:54 PM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Thu, Mar 12, 2020 at 6:51 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
> >
> > Jonathan Tan wrote:
> > > Derrick Stolee wrote:
> >
> > >> but it appears that we rely on the "remote.<name>.promisor = true"
> > >> setting instead of this extension.
> > >
> > > 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.
>
> That could be true of "remote.<name>.promisor = true".

I mean for future versions, not for old versions.

Re: [Question] Is extensions.partialClone defunct?

[Jonathan Nieder]

Hi,

Christian Couder wrote:
>> Jonathan Tan wrote:

>>> 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.
>
> That could be true of "remote.<name>.promisor = true".

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.

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

>> 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?
[...]
> So I'd rather obsolete "extensions.partialClone = <remote>" and to
> find other ways.

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

That suggests something like

	[extensions]
		multiplePromisors = true

[...]
>                                                              or maybe
> we could have another extension alltogether like
> "[extensions]\npromisorremotes=<bool>" and over time obsolete
> "extensions.partialClone" altogether. I prefer the later.

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

Re: [Question] Is extensions.partialClone defunct?

[Junio C Hamano]

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

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

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

Re: [Question] Is extensions.partialClone defunct?

[Christian Couder]

Hi,

On Fri, Mar 13, 2020 at 12:09 AM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Christian Couder wrote:
> >> Jonathan Tan wrote:
>
> >>> 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.
> >
> > That could be true of "remote.<name>.promisor = true".
>
> 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.

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

> > On Thu, Mar 12, 2020 at 6:51 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> >> 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?
> [...]
> > So I'd rather obsolete "extensions.partialClone = <remote>" and to
> > find other ways.
>
> I *think* that means "new repository extension".

Yeah.

> That suggests something like
>
>         [extensions]
>                 multiplePromisors = true

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

> >                                                              or maybe
> > we could have another extension alltogether like
> > "[extensions]\npromisorremotes=<bool>" and over time obsolete
> > "extensions.partialClone" altogether. I prefer the later.
>
> 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.

Yeah, I agree.

> I can start writing a proposed patch to send this evening or tomorrow.

That would be very much appreciated! Thanks!

> This behavior has been around for a few releases so it would want to
> cook until the 2.27 cycle.

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

Re: [Question] Is extensions.partialClone defunct?

[Tao Klerks]

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:
> On Fri, Mar 13, 2020 at 12:09 AM Jonathan Nieder <jrnieder@gmail.com> wrote:
> > [...]
> > >>> 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 was going to start a new thread about just this topic today, but
learned to use the archives instead, for better or for worse...

> > I can start writing a proposed patch to send this evening or tomorrow.
> That would be very much appreciated! Thanks!

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?

> > >> 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?
> > [...]
> > > So I'd rather obsolete "extensions.partialClone = <remote>" and to
> > > find other ways.
> >
> > I *think* that means "new repository extension".
> > [...]
> > That suggests something like
> >
> >         [extensions]
> >                 multiplePromisors = true
> > [...]
> > > or maybe
> > > we could have another extension alltogether like
> > > "[extensions]\npromisorremotes=<bool>" and over time obsolete
> > > "extensions.partialClone" altogether. I prefer the later.
> >
> > 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.
>
> > This behavior has been around for a few releases so it would want to
> > cook until the 2.27 cycle.
>
> Yeah, and partial clone is experimental, so I think it's ok.
>

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