Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make a backup of the crontab file on edition and deletion #128

Merged
merged 1 commit into from
Dec 27, 2022

Conversation

iTrooz
Copy link
Contributor

@iTrooz iTrooz commented Dec 16, 2022

This will help users that mistakenly run crontab -r, by providing a way to restore the crontab file

A bit related to #12

@t8m
Copy link
Member

t8m commented Dec 20, 2022

This is a nice idea however I'd like to see it a little bit more general - i.e., backup the previous user's crontab any time a writing to a crontab happens. Also the backup file should be placed somewhere in the home directory - I assume $(XDG_CACHE_HOME)/crontab.backup (if XDG_CACHE_HOME is set) and $HOME/.cache/crontab.backup otherwise.

@iTrooz
Copy link
Contributor Author

iTrooz commented Dec 22, 2022

I think that should be good !

src/crontab.c Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
src/crontab.c Show resolved Hide resolved
@iTrooz iTrooz force-pushed the backup_on_deletion branch from 5eabf8c to 8589910 Compare December 22, 2022 16:14
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final nits. Plus we need the username from -u in the backup file name and also some documentation in the crontab(1) manpage.

src/crontab.c Outdated Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
Comment on lines +557 to +598
(void) fclose(crontab_file);
(void) fclose(backup_file);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the space between the cast and function name.

Copy link
Contributor Author

@iTrooz iTrooz Dec 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other instances of (void) have a space, for example :

(void) strcpy(Host, argv[optind]);

Are you sure ?

@iTrooz
Copy link
Contributor Author

iTrooz commented Dec 23, 2022

Sure! Sorry for all these basic mistakes, it's my first time contributing C code

(I know I still have to add documentation to the man, I'll do that when I get time)

@iTrooz iTrooz force-pushed the backup_on_deletion branch 2 times, most recently from f395088 to cfa03c4 Compare December 23, 2022 13:13
man/crontab.1 Outdated Show resolved Hide resolved
man/crontab.1 Outdated Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
src/crontab.c Outdated Show resolved Hide resolved
@iTrooz
Copy link
Contributor Author

iTrooz commented Dec 27, 2022

Do you want me to clean the commit tree with an interactive rebase, before integrating them in master ?

src/crontab.c Outdated Show resolved Hide resolved
@t8m
Copy link
Member

t8m commented Dec 27, 2022

Do you want me to clean the commit tree with an interactive rebase, before integrating them in master ?

Yes, please squash the commits to single one with some descriptive commit message.

@iTrooz iTrooz force-pushed the backup_on_deletion branch from 27f12d5 to 45a62cf Compare December 27, 2022 16:06
@iTrooz iTrooz changed the title Make a backup of the crontab file on deletion Make a backup of the crontab file on edition and deletion Dec 27, 2022
@t8m
Copy link
Member

t8m commented Dec 27, 2022

Perfect. Merging.

@t8m t8m merged commit 2ba7685 into cronie-crond:master Dec 27, 2022
@iTrooz iTrooz deleted the backup_on_deletion branch December 27, 2022 16:18
@iTrooz
Copy link
Contributor Author

iTrooz commented Dec 27, 2022

Thanks for being patient with me

@anthonyryan1
Copy link

This is failing on systems where $XDG_CACHE_HOME or $HOME/.cache doesn't exist yet.

Should the pull request to fix this create the .cache folder where it doesn't exist, or disable the backup when it doesn't exist?

I'm leaning towards creating .cache where missing, rather than failing.

@t8m
Copy link
Member

t8m commented Jan 23, 2024

This is failing on systems where $XDG_CACHE_HOME or $HOME/.cache doesn't exist yet.

Should the pull request to fix this create the .cache folder where it doesn't exist, or disable the backup when it doesn't exist?

I'm leaning towards creating .cache where missing, rather than failing.

This was reported as #173

Creating a .cache would be fine IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants