Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user

From: Phillip Wood <phillip.wood123@gmail.com>
To: Max Gautier <mg@max.gautier.name>, git@vger.kernel.org
Cc: "Lénaïc Huard" <lenaic@lhuard.fr>,
	"Derrick Stolee" <stolee@gmail.com>,
	"Patrick Steinhardt" <ps@pks.im>, "Jeff King" <peff@peff.net>,
	"Junio C Hamano" <gitster@pobox.com>
Subject: Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user
Date: Sun, 24 Mar 2024 15:45:45 +0000	[thread overview]
Message-ID: <1cce271f-c3df-432b-94ff-bf7b11c6924d@gmail.com> (raw)
In-Reply-To: <Zf636bsHs9woXRAS@framework>

Hi Max

On 23/03/2024 11:07, Max Gautier wrote:
> On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:
>>   
>> +/*
>> + * TODO: in the future (~2026 ?) remove this cleanup code
>> + */

That is rather optimistic - users only run "git maintenance start" once 
so any unit files that have been written in the past will exist well 
beyond 2026.

>> +static void systemd_delete_user_unit(char const *unit)
>> +{
>> +	char const	file_start_stale[] =	"# This file was created and is"
>> +						" maintained by Git.";
>> +	char		file_start_user[sizeof(file_start_stale)] = {'\0'};
>> +
>> +	char *filename = xdg_config_home_for("systemd/user", unit);
>> +	int handle = open(filename, O_RDONLY);
>> +
>> +	/*
>> +	 * Check this is actually our file and we're not removing a legitimate
>> +	 * user override.
>> +	 */
>> +	if (handle == -1 && !is_missing_file_error(errno))
>> +		warning(_("failed to delete '%s'"), filename);
>> +	else {
>> +		read(handle, file_start_user, sizeof(file_start_stale) - 1);
> 
> Actually that fails -Werror because I don't check read return.
> Alternative below (on top of this one), with one question:
> Are VLA using size_t const OK ? It's folded to a constant array by gcc
> but I don't know if that causes portability problem with other platforms
> ? I can always repeat the sizeof expr if it's a problematic construct.

I think it would be easier to use strbuf_read_file() instead - it is 
only a small file so there is not really any advantage in just reading 
the first line. Something like

static int systemd_delete_user_unit(const char* unit)
{
	int ret = 0;
	struct strbuf buf = STRBUF_INIT;
	char *filename = xdg_config_home_for("systemd/user", unit);

	if (strbuf_read_file(&buf, filename, 0) < 0) {
		if (errno != ENOENT)
			ret = error_errno(_("could not read '%s'", filename));
		goto out;
	}
	if (starts_with(buf.buf,
			"# This file was created and is maintained by git.\n") &&
	    unlink(filename))
		ret = error_errno(_("could not remove '%s', filename));

out:
	free(filename);
	strbuf_release(&buf);
	return ret;
}

Best Wishes

Phillip

> -- >8 --
> 
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 99b158e481..7fb25ea2b1 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -2332,11 +2332,14 @@ static int systemd_set_units_state(int enable)
>   /*
>    * TODO: in the future (~2026 ?) remove this cleanup code
>    */
> +
>   static void systemd_delete_user_unit(char const *unit)
>   {
>   	char const	file_start_stale[] =	"# This file was created and is"
>   						" maintained by Git.";
> -	char		file_start_user[sizeof(file_start_stale)] = {'\0'};
> +	size_t const	length = sizeof(file_start_stale);
> +	char		file_start_user[length] = {'\0'};
> +
>   
>   	char *filename = xdg_config_home_for("systemd/user", unit);
>   	int handle = open(filename, O_RDONLY);
> @@ -2348,14 +2351,14 @@ static void systemd_delete_user_unit(char const *unit)
>   	if (handle == -1 && !is_missing_file_error(errno))
>   		warning(_("failed to delete '%s'"), filename);
>   	else {
> -		read(handle, file_start_user, sizeof(file_start_stale) - 1);
> -		close(handle);
> -		if (strcmp(file_start_stale, file_start_user) == 0) {
> +		if (length - 1 == read(handle, file_start_user, length - 1) &&
> +				strcmp(file_start_stale, file_start_user) == 0) {
>   			if (unlink(filename) == 0)
>   				warning(_("deleted stale unit file '%s'"), filename);
>   			else if (!is_missing_file_error(errno))
>   				warning(_("failed to delete '%s'"), filename);
>   		}
> +		close(handle);
>   	}
>   
>   	free(filename);
> 