Re: [PATCH] fetch: allow adding a filter after initial clone.

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 3457 bytes --]

On 2020-05-13 at 20:00:40, Xin Li wrote:
> Signed-off-by: Xin Li <delphij@google.com>
> ---
>  builtin/fetch.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 3ae52c015d..e5faa17ecd 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1790,8 +1790,16 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  	if (depth || deepen_since || deepen_not.nr)
>  		deepen = 1;
>  
> -	if (filter_options.choice && !has_promisor_remote())
> -		die("--filter can only be used when extensions.partialClone is set");
> +	if (filter_options.choice && !has_promisor_remote()) {
> +		char repo_version_string[10];
> +
> +		xsnprintf(repo_version_string, sizeof(repo_version_string),
> +			  "%d", (int)GIT_REPO_VERSION);
> +		git_config_set("core.repositoryformatversion",
> +			repo_version_string);
> +		git_config_set("extensions.partialclone", "origin");

Some things stood out to me here.  One, is this setting up the
repository if it's not already created?  If so, we'd probably want to
use one of the appropriate functions in setup.c.  Even if we're just
changing it, we should probably use a helper function.

Two, it isn't necessarily safe to automatically change the repository
version.  Keys that start with "extensions." are not special in format
version 0, but they are in format version 1.  I can technically have an
"extensions.tomatoSalad" in version 0 without any special meaning or
negative consequences, but as soon as we change to version 1, Git will
refuse to read it, since it doesn't know about the tomatoSalad extension
and in v1 unknown extensions are fatal.

My example may sound silly, but since extensions can be set in the
global config, users could well rely on v0 repositories ignoring them
and having them automatically turned on for their v1 repositories.  (I'm
thinking of the future extensions.objectFormat as something somebody
might try to do here, as dangerous as it may be.)

These aren't insurmountable problems, but they are things we'd need to
check for before just changing the repository version, so we'd want to
stuff some code in setup.c to handle this case.

Third, I'm not sure that "origin" is always the value we want to use
here.  At a previous employer, the upstream remote was called
"upstream", and your personal fork was called "origin", so I'd have
wanted upstream here..  We'd probably want to use whatever remote the
user is already using in this case, and gracefully handle the URL case
if that isn't allowed here (which it may be; I'm not that familiar with
partial clone).

I also agree with Junio's assessment that you'd probably want to explain
more about this feature in the commit message.  For example, I'd want to
know what this patch does and have some idea of how I might invoke this
feature, why it's safe to change the repository version, how one sets
the remote for fetch, and pretty much answers to all the other things I
asked here.  Even if I understand these things now, that doesn't mean a
future developer will in six months' time, and mentioning these things
in the commit message helps leave a note to them that you considered (or
did not consider, as the case may be) certain issues and helps them
understand the code as you saw and wrote it.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]