Re: [PATCH] setup: warn about un-enabled extensions

[Derrick Stolee <stolee@gmail.com>]

On 7/15/2020 1:01 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> The current one on the table is NOT
>> <20200714040616.GA2208896@google.com> but the two patches
>>
>> <1b26d9710a7ffaca0bad1f4e1c1729f501ed1559.1594690017.git.gitgitgadget@gmail.com>
>> <e11e973c6fff6a523da090f7294234902e65a9d0.1594690017.git.gitgitgadget@gmail.com>
>>
>> For example it special cases only the worktreeconfig and nothing
>> else, even though I suspect that other configuration variables were
>> also honored by mistake.

Sorry for the delay. I had your previous diff applied and was
playing around with the two "known breakages".

The diff below _does_ fail on t0410-partial-clone.sh, but it's
because you do change the behavior. Here is my diff hunk for that
test:

diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 463dc3a8be..fc8da56528 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -42,7 +42,7 @@ test_expect_success 'convert shallow clone to partial clone' '
        test_cmp_config -C client 1 core.repositoryformatversion
 '
 
-test_expect_success 'convert shallow clone to partial clone must fail with any extension' '
+test_expect_success 'convert shallow clone to partial clone succeeds with grandfathered extension' '
        rm -fr server client &&
        test_create_repo server &&
        test_commit -C server my_commit 1 &&
@@ -50,7 +50,20 @@ test_expect_success 'convert shallow clone to partial clone must fail with any e
        git clone --depth=1 "file://$(pwd)/server" client &&
        test_cmp_config -C client 0 core.repositoryformatversion &&
        git -C client config extensions.partialclone origin &&
-       test_must_fail git -C client fetch --unshallow --filter="blob:none"
+       git -C client fetch --unshallow --filter="blob:none"
+'
+
+test_expect_success 'convert shallow clone to partial clone must fail with unknown 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 &&
+       test_cmp_config -C client 0 core.repositoryformatversion &&
+       git -C client config extensions.unknownExtension true &&
+       git -C client config extensions.partialclone origin &&
+       test_must_fail git -C client fetch --unshallow --filter="blob:none" 2>err &&
+       test_i18ngrep "unable to upgrade repository format from 0 to 1" err
 '
 
 test_expect_success 'missing reflog object, but promised by a commit, passes fsck' '


> The attached may be a less ambitious and less risky update for the
> upcoming release.  It is to be applied on top of the two-patch
> series from Derrick, and just marks the other "known and honored
> back then by mistake" extensions as OK to be there for upgrading.
> 
> Thoughts?  If people are happy with that, then we could apply and
> cut an -rc1 with it.  Or if we are OK with the "just special case
> worktreeconfig; other extensions may have the same issue but we
> haven't heard actual complaints so we will leave them untouched",
> then -rc1 can be done with just those two patches.

The "special case wortreeConfig" of my submission was maybe based
on an incorrect assumption that this is the only place where Git
itself set an extension.* config without _also_ updating the
core.repositoryFormatVersion config.

The error I made in my v1 is to warn on ALL extensions, not just
the ones that Git knows about. This new approach is a good middle
ground between my v1 and v2.

Of course, the _other_ option is to revert xl/upgrade-repo-format
from v2.28.0 and take our time resolving this issue during the
2.29 cycle. I'm not sure how disruptive that action would be.

> Now I do need to shift my attention to other topics in flight.

I appreciate you spending so much time on this! It's a tough
time to be noticing such a complicated situation that is not
easily testable from a single Git version, but across versions.

> 
>  setup.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/setup.c b/setup.c
> index 65270440a9..a072c76d05 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -456,27 +456,32 @@ static int check_repo_format(const char *var, const char *value, void *vdata)
>  		data->version = git_config_int(var, value);
>  	else if (skip_prefix(var, "extensions.", &ext)) {
>  		/*
> -		 * record any known extensions here; otherwise,
> -		 * we fall through to recording it as unknown, and
> -		 * check_repository_format will complain
> +		 * Grandfather extensions that were known in 2.27 and
> +		 * were honored by mistake even in v0 repositories; it
> +		 * shoudn't be an error to upgrade v0 to v1 with them
> +		 * in the repository, as they couldn't have been used
> +		 * for incompatible purposes by the end user.
>  		 */
> -		int is_unallowed_extension = 1;
> +		int unallowed_in_v0 = 1;
>  
> -		if (!strcmp(ext, "noop"))
> -			;
> -		else if (!strcmp(ext, "preciousobjects"))
> +		if (!strcmp(ext, "noop")) {
> +			unallowed_in_v0 = 0;
> +		} else if (!strcmp(ext, "preciousobjects")) {
>  			data->precious_objects = git_config_bool(var, value);
> -		else if (!strcmp(ext, "partialclone")) {
> +			unallowed_in_v0 = 0;
> +		} else if (!strcmp(ext, "partialclone")) {
>  			if (!value)
>  				return config_error_nonbool(var);
>  			data->partial_clone = xstrdup(value);
> +			unallowed_in_v0 = 0;
>  		} else if (!strcmp(ext, "worktreeconfig")) {
>  			data->worktree_config = git_config_bool(var, value);
> -			is_unallowed_extension = 0;

Your previous diff had this comment, which I thought to be
helpful: 

+		/*
+		 * Extensions are added by more "} else if (...) {"
+		 * lines here, but do NOT mark them as allowed in v0
+		 * by copy-pasting without thinking.
+		 */

> -		} else
> +			unallowed_in_v0 = 0;
> +		} else {
>  			string_list_append(&data->unknown_extensions, ext);
> +		}
>  
> -		data->has_unallowed_extensions |= is_unallowed_extension;
> +		data->has_unallowed_extensions |= unallowed_in_v0;
>  	}
>  
>  	return read_worktree_config(var, value, vdata);

Thanks,
-Stolee