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>, phillip.wood@dunelm.org.uk
Cc: git@vger.kernel.org, "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: Mon, 25 Mar 2024 16:39:23 +0000	[thread overview]
Message-ID: <cd7bc865-4a01-46f6-8d68-24f02fdabcb9@gmail.com> (raw)
In-Reply-To: <ZgE3mHsPfKbuQLQP@framework>

Hi Max

On 25/03/2024 08:36, Max Gautier wrote:
> On Sun, Mar 24, 2024 at 03:45:45PM +0000, Phillip Wood wrote:
>> 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.
> 
> In that case, should we hook the cleanup (in it's final form) in more
> place ? `git maintenance register` for instance ?

I'm not sure if that is needed if we leave the code to delete the unit 
files in place.

Best Wishes

Phillip

>>>> +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;
>> }
> 
> Thanks, this is exactly what I needed, I looked at the strbuf API but
> couldn't find this somehow.
> 