Re: [PATCH] maintenance: fix incorrect `maintenance.repo` path with bare repository

From: Derrick Stolee <stolee@gmail.com>
Date: Feb 23, 2021, 12:25 UTC
Message-ID: <5653230a-20c2-321f-7ea0-93ce59858532@gmail.com>
In-Reply-To: <20210223073107.40675-1-sunshine@sunshineco.com>

On 2/23/2021 2:31 AM, Eric Sunshine wrote:

Show 18 quoted lines

> The periodic maintenance tasks configured by `git maintenance start`
> invoke `git for-each-repo` to run `git maintenance run` on each path
> specified by the multi-value global configuration variable
> `maintenance.repo`. Because `git for-each-repo` will likely be run
> outside of the repositories which require periodic maintenance, it is
> mandatory that the repository paths specified by `maintenance.repo` are
> absolute.
> 
> Unfortunately, however, `git maintenance register` does nothing to
> ensure that the paths it assigns to `maintenance.repo` are indeed
> absolute, and may in fact -- especially in the case of a bare repository
> -- assign a relative path to `maintenance.repo` instead. Fix this
> problem by converting all paths to absolute before assigning them to
> `maintenance.repo`.
> 
> While at it, also fix `git maintenance unregister` to convert paths to
> absolute, as well, in order to ensure that it can correctly remove from
> `maintenance.repo` a path assigned via `git maintenance register`.

Thanks for the report and the fix!

Show 10 quoted lines

> +static char *get_maintpath(void)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	const char *p = the_repository->worktree ?
> +		the_repository->worktree : the_repository->gitdir;
> +
> +	strbuf_realpath(&sb, p, 1);
> +	return strbuf_detach(&sb, NULL);
> +}
> +

This looks right.

Show 7 quoted lines

>  static int maintenance_register(void)
>  {
> +	int rc;
>  	char *config_value;
>  	struct child_process config_set = CHILD_PROCESS_INIT;
>  	struct child_process config_get = CHILD_PROCESS_INIT;
> +	char *maintpath = get_maintpath();

I'm sorry that this diff looks more complicated than seems necessary, but your creation of the "done:" label is important for cleaning up the string. Thanks.

Show 8 quoted lines

> +test_expect_success 'register and unregister bare repo' '
> +	test_when_finished "git config --global --unset-all maintenance.repo || :" &&
> +	test_might_fail git config --global --unset-all maintenance.repo &&
> +	git init --bare barerepo &&
> +	(
> +		cd barerepo &&
> +		git maintenance register &&
> +		git config --get --global --fixed-value maintenance.repo "$(pwd)" &&

I'm concerned about this test passing on Windows, but if the CI build is happy, then I'm happy.

> +		git maintenance unregister &&
> +		test_must_fail git config --global --get-all maintenance.repo
> +	)
> +'

Thanks!

Reviewed-by: Derrick Stolee <dstolee@microsoft.com>