end-4 / dots-hyprland

Modern, feature-rich and accessible desktop configuration.
https://end-4.github.io/dots-hyprland-wiki/en/
GNU General Public License v3.0
3.16k stars 204 forks source link

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

Closed H0mire closed 1 month ago

H0mire commented 2 months ago

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 commented 2 months ago

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:

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.

H0mire commented 2 months ago

Thank you for your feedback.

H0mire commented 2 months ago

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

Thanks!

clsty commented 2 months ago

The git clone part has some major problems.

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 commented 2 months ago

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

H0mire commented 2 months ago

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

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

end-4 commented 2 months ago

@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 commented 1 month ago

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

clsty commented 1 month ago

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