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

[Feature] Added Update-dots.sh script #473

Merged
merged 26 commits into from
May 16, 2024
Merged

Conversation

H0mire
Copy link
Contributor

@H0mire H0mire commented May 3, 2024

Automatically updates the Repository and the dots:
Replaces by default .config and .local files in the home directory.
Checks if user has modified files => Asking before replacing modified files.
You are able to exclude folders from updating. e.g. the "custom" folder
Implemented with Checksums.

@clsty
Copy link
Collaborator

clsty commented May 3, 2024

Generally this script choose the files under $base which does not have a exact same file with md5 hash under $home, and prompt whether to sync them, right?

I won't talk about the problem that you did not use set -e and either mkdir -p or equivalent, but the key point is that:

  1. All this can be done by using rsync with --checksum. You don't have to compare md5sum manually at all.
  2. This script only copies the modified files, or not. (Note that you did not ever consider the situation if the destination file does not exists.) It's even meaningless to continue the script after a "n" is given to "Do you want to keep these files untouched? [Y/n]" because it won't do anything anyway, just exit.

Though I guess it's possible to implement similar function for ./install.sh, such as using --no-overwrite. I'll work on it when I'm available.

Thanks for your contribution anyway.

Closing.


My apologies, I rechecked the logic and I've found that the position of git pull matters.

So, if a file is different with the destination before git pull, the script assume it's "modified", which means it may contain some useful changes from the user, so user should be allowed to skip such file after git pull and sync them.

The problem is that the script is not robust, such as:

  • This assumption about "modified" only holds true when "all files were synchronized the last time the user used install.sh, and no methods like git pull were used to update this repository after that till now."
    • Also there are some files which will be modified by .config/ags/scripts/color_generation/applycolor.sh, not by users.
    • You'd better let user to choose which ones of them, not only a [y/n] for all of them or none of them.
  • git pull may fail when a force push has happened in remote repo.
  • If something went wrong, you need a set -e at the beginning to make the script abort.
  • You did not consider the situation when the destination file does not exists.
  • You did not consider the situation when the destination folder structure does not exists.

And for the exclusion paths, you only considered .config/hypr/custom but .config/ags/user_options.js and .config/hypr/hyprland.conf should also be excluded. Note that they are files, not folders.

If you can solve these problem, I think it's OK to be merged.

@clsty clsty closed this May 3, 2024
@clsty clsty reopened this May 4, 2024
@H0mire
Copy link
Contributor Author

H0mire commented May 4, 2024

Thank you for your feedback.

  • Correct, i considered rsync, but as you know it would remove the user modified scripts, since the new ones have a different checksum and metadata.
  • I also considered the case that git pull has already been called. In that case, the scripts tells the user, that no file has been found that has been configured by the user, and asks if every file should be replaced.
    In order to make it independent there must be some cache that holds the checksums created with the install.sh script or the update-dots script. Should I implement that?
  • Ok, I will add the possibility to decide for each file independently. But i still will leave a -all option.
  • The rest will be fixed.

@H0mire
Copy link
Contributor Author

H0mire commented May 4, 2024

@clsty I updated the script. Could you please review the script again?

Thanks!

@clsty
Copy link
Collaborator

clsty commented May 4, 2024

The git clone part has some major problems.

  • I suggest to use a --depth=1 for git clone.
  • find "$temp_folder/$folder" -type f -print0 | while IFS= read -r -d '' file; do will give you a $file var which looks like ./cache/tmp.rhqpeLo6Zw/.config/ags/..., you surely don't want that ./cache/tmp.rhqpeLo6Zw thing, else $HOME/$file will be messed up.
  • It lacks logic to skip modified files.
  • After this part is done and temp directory removed, the script does not stop, but continues to copy files from main directory again.

Another problem is that you need to prompt user what this script is for, and give user chance to exit in case they run this by accident.


As for the rest part, after reading through it, it seems OK to me, but I don't have a suitable environment to test it, so I can't tell if there're more problems.

Nor have you tested it, right? Else I would not have found out the major problems above.

I suggest you to use it by yourself for a couple of days so you will be able to fix/improve some neglected things, if there is any.


@end-4 What do you think about this script?

@H0mire
Copy link
Contributor Author

H0mire commented May 4, 2024

Good job, great awareness. Makes sense. I tested it, but seems like the wrong way, lol. Thanks, working on it...

@H0mire
Copy link
Contributor Author

H0mire commented May 4, 2024

Ordered, delivered. Can you check? @end-4 and @clsty

Update:
Tested now by resetting to an earlier commit.
Tested Cases:

  • Git pull working, with and without checking files individually
  • Git pull not working, with and without checking files individually

@end-4
Copy link
Owner

end-4 commented May 4, 2024

@end-4 What do you think about this script?

@clsty I don't see any problem
Just gonna comment that the grammar/wording is good xd
Since this is a scripting-related PR I'll let you decide when to merge

@H0mire
Copy link
Contributor Author

H0mire commented May 16, 2024

@clsty It's been a while. Anything that must be improved? Encountered no error since.

@clsty
Copy link
Collaborator

clsty commented May 16, 2024

I can't find out any problem for now, so I'll merge it. Thank you for contribution.

@clsty clsty merged commit 187bd76 into end-4:main May 16, 2024
end-4 pushed a commit to Soliprem/dots-hyprland that referenced this pull request May 24, 2024
end-4 pushed a commit to Soliprem/dots-hyprland that referenced this pull request May 24, 2024
end-4 pushed a commit to Soliprem/dots-hyprland that referenced this pull request May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants