Skip to content

Update qinstall.sh: store_config(): Change for more efficency, and no…#38

Open
luca1234567a wants to merge 1 commit into
qnap-dev:masterfrom
luca1234567a:patch-1
Open

Update qinstall.sh: store_config(): Change for more efficency, and no…#38
luca1234567a wants to merge 1 commit into
qnap-dev:masterfrom
luca1234567a:patch-1

Conversation

@luca1234567a
Copy link
Copy Markdown

@luca1234567a luca1234567a commented Dec 19, 2024

store_config(): Change for more efficency, and not necessary operation. Change order of if.
Prevent non necessary execution of current_md5sum=$($CMD_MD5SUM "$file" 2>/dev/null | $CMD_CUT -d' ' -f1) if $file non exist.
Place first if [ -z "$orig_md5sum" ]; then because if $orig_md5sum is empty the if [ "$orig_md5sum" = "$current_md5sum" ] || [ "$new_md5sum" = "$current_md5sum" ]; then allways was false.

@edhongcy
Copy link
Copy Markdown
Collaborator

Thanks for working on this. I think the intent makes sense, but I would not merge this version as-is for two reasons:

  • This is no longer only an efficiency change. Reordering the conditions changes behavior in store_config(). In the old flow, if the current file already matched orig_md5sum or new_md5sum, the script would keep using the new file path without moving it aside. After this change, when orig_md5sum is empty and the file exists, the code now enters the -z "$orig_md5sum" branch first and moves the file to ${file}.qdkorig, even in cases where the current file already matches the new package content. That looks like a functional change, not just an optimization.

  • git diff --check also reports a whitespace issue on the new elif line (space before tab in indent).

Suggested direction:

  • Keep the optimization of avoiding md5sum when the file does not exist.
  • But preserve the original decision order for existing files, so the current_md5sum comparison still happens before the -z "$orig_md5sum" backup path.
  • Please also fix the whitespace issue before merge.

So from my side, this PR needs one more revision before it is safe to merge.

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.

2 participants