Re: [PATCH] repack: add config to skip updating server info

[Taylor Blau <me@xxxxxxxxxxxx>]

On Fri, Mar 11, 2022 at 12:09:30PM +0100, Patrick Steinhardt wrote:
> diff --git a/Documentation/config/repack.txt b/Documentation/config/repack.txt
> index 9c413e177e..22bfc26afc 100644
> --- a/Documentation/config/repack.txt
> +++ b/Documentation/config/repack.txt
> @@ -25,3 +25,6 @@ repack.writeBitmaps::
>  	space and extra time spent on the initial repack.  This has
>  	no effect if multiple packfiles are created.
>  	Defaults to true on bare repos, false otherwise.
> +
> +repack.updateServerInfo::
> +	If set to false, git-repack will not run git-update-server-info.

Can you clarify here what the default value of this config variable is,
and how it interacts with repack's `-n` flag? E.g., something along the
lines of:

    repack.updateServerInfo::
        If set to false, linkgit:git-repack[1] will not run
        linkgit:git-update-serve-info[1]. Defaults to true. Can be
        overridden when true by the `-n` option of
        linkgit:git-repack[1].

Perhaps a little verbose, but I think it leaves less ambiguity about
what this new configuration variable is for.

> diff --git a/builtin/repack.c b/builtin/repack.c
> index da1e364a75..3baa993da2 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -22,6 +22,7 @@ static int delta_base_offset = 1;
>  static int pack_kept_objects = -1;
>  static int write_bitmaps = -1;
>  static int use_delta_islands;
> +static int no_update_server_info = 0;

Not the fault of this patch, but I wonder if this would be less
confusing if we stored `update_server_info` instead of
`no_update_server_info`. If you have time, I think it may be worth a
preparatory patch at the beginning to swap the two.

> +test_expect_success 'updates server info by default' '
> +	git init repo &&
> +	test_when_finished "rm -rf repo" &&
> +	test_commit -C repo message &&
> +	test_path_is_missing repo/.git/objects/info/packs &&
> +	test_path_is_missing repo/.git/info/refs &&
> +	git -C repo repack &&
> +	test_path_is_file repo/.git/objects/info/packs &&
> +	test_path_is_file repo/.git/info/refs
> +'

I wonder if this and the below tests might be cleaned up with a pair of
helper functions, perhaps:

    test_server_info_present () {
      test_path_is_file .git/objects/info/packs &&
      test_path_is_file .git/info/refs
    }

    test_server_info_missing () {
      test_path_is_missing .git/objects/info/packs &&
      test_path_is_missing .git/info/refs
    }

t7700 has a mix of styles, but it may shorten some of the lines to use a
subshell that is changed into the repo directory, e.g., the test above
would become:

    test_expect_success 'updates server info by default' '
      git init repo &&
      test_when_finished "rm -fr repo" &&
      (
        test_commit message &&
        test_server_info_missing &&
        git repack &&
        test_server_info_present
      )
    '

which reads a little more easily to me. It would be nice to avoid
creating the sub-repos at all, perhaps by removing these files
ourselves in between tests.

> +test_expect_success '-n skips updating server info' '
> +test_expect_success 'repack.updateServerInfo=true updates server info' '
> +test_expect_success 'repack.updateServerInfo=false skips updating server info' '
> +test_expect_success '-n overrides repack.updateServerInfo=true' '

Great, these four and the above together cover all of the cases I think
we'd be interested in.

Thanks,
Taylor