Re: [PATCH] fetch: allow adding a filter after initial clone.
- From
brian m. carlson <sandals@crustytoothpaste.net>
- Date
- May 13, 2020, 23:44 UTC
- Message-ID
- <20200513234435.GG6605@camp.crustytoothpaste.net>
- In-Reply-To
- <20200513200040.68968-1-delphij@google.com>
On 2020-05-13 at 20:00:40, Xin Li wrote:
Show 23 quoted lines
> 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