| Message ID | 20240322221327.12204-5-mg@max.gautier.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | maintenance: use packaged systemd units | expand |
On Fri, Mar 22, 2024, at 23:11, Max Gautier wrote: > Notes: > How should I refer to a commit which is part of the same patch series ? > The commit id will change so the message won't be correct anymore, right > ? It looks like a fair few say “in the previous commit” for the one just before this one and “in a previous commit” for some commit that was before this in the series but not the immediate previous one. I guess that’s okay, no?
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes: > On Fri, Mar 22, 2024, at 23:11, Max Gautier wrote: >> Notes: >> How should I refer to a commit which is part of the same patch series ? >> The commit id will change so the message won't be correct anymore, right >> ? > > It looks like a fair few say “in the previous commit” for the one just > before this one and “in a previous commit” for some commit that was > before this in the series but not the immediate previous one. I guess > that’s okay, no? I saw some series say "Earlier in the series, X has learned to do Y, so now we take advantage of that new feature", and found it quite readable.
On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote: > > +/* > + * 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'}; > + > + 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. -- >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);
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); >
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.
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. >
Le 25 mars 2024 17:39:23 GMT+01:00, Phillip Wood <phillip.wood123@gmail.com> a écrit : >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 But don't we want to remove it, eventually ?
diff --git a/builtin/gc.c b/builtin/gc.c index aaee91451a..99b158e481 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -2329,8 +2329,50 @@ static int systemd_set_units_state(int enable) return 0; } +/* + * 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'}; + + 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); + close(handle); + if (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); + } + } + + free(filename); +} + static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED) { + /* + * A previous version of Git wrote the units in the user configuration + * directory. Clean these up, if they exist. + * TODO: in the future (~2026 ?) remove this cleanup code + */ + systemd_delete_user_unit("git-maintenance@hourly.timer"); + systemd_delete_user_unit("git-maintenance@daily.timer"); + systemd_delete_user_unit("git-maintenance@weekly.timer"); + systemd_delete_user_unit("git-maintenance@.timer"); + systemd_delete_user_unit("git-maintenance@.service"); return systemd_set_units_state(run_maintenance); }
Before commit 976640edbb (maintenance: use packaged systemd units, 2024-03-21), we we're putting systemd unit files in $XDG_CONFIG_HOME ; these could mask those we are now distributing as part of git. Remove all the systemd units possibly created by previous version of git when running `git maintenance start/stop`. Avoid overwriting units we didn't write, by comparing the first line with the start of the comment we added to our unit files previously. Signed-off-by: Max Gautier <mg@max.gautier.name> --- Notes: How should I refer to a commit which is part of the same patch series ? The commit id will change so the message won't be correct anymore, right ? builtin/gc.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)