Re: [PATCH v2 1/1] fetch: allow adding a filter after initial clone.
Show 17 quoted lines
> Retroactively adding filter can be useful for existing shallow clones as
> they allow users to see earlier change histories without downloading all
> git objects in a regular --unshallow fetch.
>
> Previously this is possible by manually amending the repository
> configuration to make git think there is an existing promisor. Because
> the code already does most of the hard work, it's safer for git to
> just perform the configuration change automatically instead.
>
> Instead of bailing out immediately when no promisor is available, make
> the code check more specific issue (extension became special in
> repository version 1, while it can have any value in version 0, so
> upgrade should not happen if the repository have an unsupported
> configuration that would render it invalid if we upgraded).
>
> Signed-off-by: Xin Li <delphij@google.com>
> ---
nit: the cover letter contains
Show 14 quoted lines
> Previously, to retroactively add filter to an existing (shallow) clone
> one would have to manually change the repository configuration to make
> git to believe that there was an existing promisor, like:
>
> git config core.repositoryFormatVersion 1
> git config extensions.partialClone origin
> git fetch --unshallow --filter=blob:none origin
>
> Because the code can already set up promisor, it would be safer and more
> convenient to just do that in git itself.
>
> This version of change will also prevent the code from making damaging
> repository upgrades (when non-standard extensions exists) as pointed out
> by earlier reviewers.
I think that would make a good commit message itself.
[...]
Show 9 quoted lines
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1790,9 +1790,6 @@ 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");
> -Show 9 quoted lines
> --- a/list-objects-filter-options.c
> +++ b/list-objects-filter-options.c
> @@ -326,7 +326,8 @@ void partial_clone_register(
>
> /* Check if it is already registered */
> if (!promisor_remote_find(remote)) {
> - git_config_set("core.repositoryformatversion", "1");
> + if (upgrade_repository_format(the_repository, 1) < 0)
> + die(_("Unable to upgrade repository format to support partial clone"));nit: Git's error messages tend to use lowercase (e.g., "fatal: cannot [etc]" instead of "fatal: Unable [etc]").
> /* Add promisor config for the remote */
> cfg_name = xstrfmt("remote.%s.promisor", remote);not about this patch: By the way, the repository format version bump is not sufficient to achieve its intended aim: we also need to set an extensions.* setting to ensure Git is new enough to know about partial clone. More discussion about this is in [1] (apologies for not having finished solving that). This isn't a regression introduced in this patch, and this patch does the right thing in the context of the current code.
[...]
Show 10 quoted lines
> --- a/repository.h
> +++ b/repository.h
> @@ -196,4 +196,10 @@ void repo_update_index_if_able(struct repository *, struct lock_file *);
>
> void prepare_repo_settings(struct repository *r);
>
> +/*
> + * Return 1 if upgrade repository format to target_version succeeded,
> + * 0 if no upgrade is necessary.
> + */
Probably also worth mentioning that this returns -1 on error.
> +int upgrade_repository_format(struct repository *r, int target_version);
> +
> #endif /* REPOSITORY_H */
Show 8 quoted lines
> +++ b/setup.c
> @@ -538,6 +538,36 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
> return 0;
> }
>
> +int upgrade_repository_format(struct repository *r, int target_version)
> +{
> + const char *gitdir = get_git_dir();Show 6 quoted lines
> + struct strbuf sb = STRBUF_INIT;
> + struct strbuf err = STRBUF_INIT;
> + struct strbuf repo_version = STRBUF_INIT;
> + struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
> +
> + strbuf_git_common_path(&sb, r, "/config");
nit: can leave out the '/' to avoid a double-/.
Show 10 quoted lines
> + read_repository_format(&repo_fmt, sb.buf);
> + strbuf_release(&sb);
> +
> + if (repo_fmt.version >= target_version)
> + return 0;
> +
> + repo_fmt.version = target_version;
> +
> + if (verify_repository_format(&repo_fmt, &err) < 0) {
> + warning("Unable to upgrade repository format to %d: %s",Same nit about capitalization.
> + target_version, err.buf);
whitespace nit: this would typically use a tab, to line up with the paren on the previous line.
Show 6 quoted lines
> + strbuf_release(&err);
> + return -1;
> + }
> +
> + strbuf_addf(&repo_version, "%d", target_version);
> + git_config_set("core.repositoryformatversion", repo_version.buf);Ah, I think I misled you: the config_set API hasn't learned to take a struct repository yet, so we should hardcode the_repository in this function instead of taking a "struct repository" parameter.
Show 26 quoted lines
> + strbuf_release(&repo_version);
> + return 1;
> +}
> +
> static void init_repository_format(struct repository_format *format)
> {
> const struct repository_format fresh = REPOSITORY_FORMAT_INIT;
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index a3988bd4b8..71270d3a53 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -30,6 +30,27 @@ test_expect_success 'extensions.partialclone without filter' '
> git -C client fetch origin
> '
>
> +test_expect_success 'convert shallow clone to partial clone' '
> + rm -fr server client &&
> + test_create_repo server &&
> + test_commit -C server my_commit 1 &&
> + test_commit -C server my_commit2 1 &&
> + git clone --depth=1 "file://$(pwd)/server" client &&
> + git -C client fetch --unshallow --filter="blob:none" &&
> + test_cmp_config -C client true remote.origin.promisor &&
> + test_cmp_config -C client blob:none remote.origin.partialclonefilter &&
> + test_cmp_config -C client 1 core.repositoryformatversion
> +'nit: Missing blank line.
Is there a different check this test could perform to check the user-facing behavior instead of how the configuration is encoded?
Show 13 quoted lines
> +test_expect_success 'convert shallow clone to partial clone must fail with invalid extension' '
> + rm -fr server client &&
> + test_create_repo server &&
> + test_commit -C server my_commit 1 &&
> + test_commit -C server my_commit2 1 &&
> + git clone --depth=1 "file://$(pwd)/server" client &&
> + git -C client config extensions.sandwidth true &&
> + test_must_fail git -C client fetch --unshallow --filter="blob:none"
> +'
> +
> test_expect_success 'missing reflog object, but promised by a commit, passes fsck' '
> rm -rf repo &&
> test_create_repo repo &&
With whatever subset of the mentioned changes makes sense,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
[1] https://lore.kernel.org/git/20200312230931.GF120942@google.com/.
Show changes to diff +11 −10
diff --git i/list-objects-filter-options.c w/list-objects-filter-options.c
index 6d62b60eaca..ce9193c3885 100644
--- i/list-objects-filter-options.c
+++ w/list-objects-filter-options.c
@@ -326,8 +326,8 @@ void partial_clone_register(
/* Check if it is already registered */
if (!promisor_remote_find(remote)) {
- if (upgrade_repository_format(the_repository, 1) < 0)
- die(_("Unable to upgrade repository format to support partial clone"));
+ if (upgrade_repository_format(1) < 0)
+ die(_("cannot enable partial clone support"));
/* Add promisor config for the remote */
cfg_name = xstrfmt("remote.%s.promisor", remote);
diff --git i/repository.h w/repository.h
index f301f6f4562..14574c6e627 100644
--- i/repository.h
+++ w/repository.h
@@ -197,9 +197,10 @@ void repo_update_index_if_able(struct repository *, struct lock_file *);
void prepare_repo_settings(struct repository *r);
/*
- * Return 1 if upgrade repository format to target_version succeeded,
- * 0 if no upgrade is necessary.
+ * Upgrade the repository format to target_version.
+ * Returns 1 on success, 0 if no upgrade was necessary, and -1 after
+ * printing a diagnostic on error.
*/
-int upgrade_repository_format(struct repository *r, int target_version);
+int upgrade_repository_format(int target_version);
#endif /* REPOSITORY_H */
diff --git i/setup.c w/setup.c
index 84da976e077..d1f0aff7d30 100644
--- i/setup.c
+++ w/setup.c
@@ -538,15 +538,14 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
return 0;
}
-int upgrade_repository_format(struct repository *r, int target_version)
+int upgrade_repository_format(int target_version)
{
- const char *gitdir = get_git_dir();
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
struct strbuf repo_version = STRBUF_INIT;
struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
- strbuf_git_common_path(&sb, r, "/config");
+ strbuf_git_common_path(&sb, the_repository, "config");
read_repository_format(&repo_fmt, sb.buf);
strbuf_release(&sb);
@@ -556,8 +555,8 @@ int upgrade_repository_format(struct repository *r, int target_version)
repo_fmt.version = target_version;
if (verify_repository_format(&repo_fmt, &err) < 0) {
- warning("Unable to upgrade repository format to %d: %s",
- target_version, err.buf);
+ warning("cannot upgrade repository format to %d: %s",
+ target_version, err.buf);
strbuf_release(&err);
return -1;
}
diff --git i/t/t0410-partial-clone.sh w/t/t0410-partial-clone.sh
index 71270d3a539..d580488330f 100755
--- i/t/t0410-partial-clone.sh
+++ w/t/t0410-partial-clone.sh
@@ -41,6 +41,7 @@ test_expect_success 'convert shallow clone to partial clone' '
test_cmp_config -C client blob:none remote.origin.partialclonefilter &&
test_cmp_config -C client 1 core.repositoryformatversion
'
+
test_expect_success 'convert shallow clone to partial clone must fail with invalid extension' '
rm -fr server client &&
test_create_repo server &&
Xin Li
