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

[Max Gautier <mg@max.gautier.name>]

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 ?

> 
> > > +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.

-- 
Max Gautier