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

feat(scoop-(un)hold): Support scoop (un)hold scoop #5089

Merged
merged 34 commits into from
Aug 11, 2022

Conversation

yi-Xu-0100
Copy link
Contributor

@yi-Xu-0100 yi-Xu-0100 commented Aug 7, 2022

Description

Support scoop (un)hold scoop.
Allow to disable scoop itself updates for one day.

Motivation and Context

The scoop is a app in scoop, but it can not to be hold.
Now it works. 😁

How Has This Been Tested?

Checklist:

  • I have read the Contributing Guide.
  • I have ensured that I am targeting the develop branch.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.
  • I have added an entry in the CHANGELOG.

Allow to disable Scoop itself updates.
This configuration have the same function with 'scoop (un)hold scoop'.
@niheaven
Copy link
Member

niheaven commented Aug 7, 2022

I don't think scoop itself should be hold (not updated) since we keep fixing bugs in it, and this shouldn't be fixed IMO.

@yi-Xu-0100
Copy link
Contributor Author

@niheaven It is only an option both for user and contributor. Scoop always updates with remote without asking, and it may lose the local changes.🤔

For me, I change the content of scoop for doing some test locally, but the scoop always updates as the remote and loss my change content, I think I can hold the scoop to test the local feature, and enable update quickly if new version published. 😥

@niheaven
Copy link
Member

niheaven commented Aug 7, 2022

Yes, I've the same trouble as yours, and I think a better way is auto stashing the uncommited changes and noticing users that they have these changes and scoop has stashed them.

Would you like to make this feature and submit PR about that?

'Cos indeed a config option to HOLD update of scoop is so easy to be forgotten...

@yi-Xu-0100
Copy link
Contributor Author

yi-Xu-0100 commented Aug 7, 2022

@niheaven

$SCOOP_HOLD = get_config 'SCOOP_HOLD' $true
if($SCOOP_HOLD) {
warn "'SCOOP_HOLD' has been setting to '`$true' and skip updating Scoop..."
warn "If you want to update Scoop itself, use 'scoop config SCOOP_HOLD `$false' or 'scoop unhold scoop' to enable it."
} else {

I use a warning notification when scoop used to update itself. It effects every time when users want to update scoop. 😁

The stash operation may helpful, but I may not deal with it good. If I make this feature, I will submit a PR to here. 🙌

But I also think the hold should be a choice for uses of scoop. The scoop is an app in scoop/apps, so I think it needs to obey the rule of an app. An app can be hold to disable update, not likes Windows. 😂

@r15ch13
Copy link
Member

r15ch13 commented Aug 7, 2022

Wouldn't it be better to move the bucket update code out of update_scoop() so there is no need for a big overarching if-statement? The function can then be returned early.

@niheaven
Copy link
Member

niheaven commented Aug 7, 2022

Just add autostash in #5091, and this could meet your need @yi-Xu-0100

@niheaven
Copy link
Member

niheaven commented Aug 7, 2022

Wouldn't it be better to move the bucket update code out of update_scoop() so there is no need for a big overarching if-statement? The function can then be returned early.

Good idea for the function separation and may be done later.

@yi-Xu-0100
Copy link
Contributor Author

@niheaven

#5091 is good for users who change a little and do things in the current branch. 👍

But for contributors, It always set the scoop to remote, we still need to check out our own branch and stash pop changes. It will make many steps for test. 🤔

@niheaven
Copy link
Member

niheaven commented Aug 7, 2022

No, you don't. It will stash uncommitted change on your working branch and then checkout 'develop' branch (the one in config file), all your works will not be lost.

@yi-Xu-0100
Copy link
Contributor Author

@niheaven I know that the #5091 will stash uncommitted change and will not lose my work. That is a good job! 👍

But we do not always work on the branch in which we config SCOOP_REPO and SCOOP_BRANCH in the config file.
New PR may create a new branch, and I do not want to set the config every time. 😥

It will update when I test in another branch, and I need to check out and get my work back every time. It can be simply used scoop hold scoop to avoid this happening. 🤔

@niheaven
Copy link
Member

niheaven commented Aug 7, 2022

Hmm, you may not understand my point. Which I mean is, you create a new branch and do some change, commit them, then do another changes, then you want to test it but a scoop update happened, the #5091 will stash all your uncommitted changes on the new branch and then the following code of scoop update will checkout develop.

All you do after such update are checkout your working branch and git stash pop, everything is back: your new branch, your new commit and your uncommitted changes.

Keep coding forward, and you never need to test something that invoke scoop update except for scoop update itself, right? For the latter case, change your SCOOP_REPO and SCOOP_BRANCH...

@r15ch13
Copy link
Member

r15ch13 commented Aug 7, 2022

Or develop stuff in a separate repo and use it from the editor or create a custom shim that points to the develop repo. 😄

@yi-Xu-0100
Copy link
Contributor Author

yi-Xu-0100 commented Aug 7, 2022

@niheaven @r15ch13

I know what all you say, I just think it needs to do many steps to test little things about update.

The another reason is that I think the user can choose to hold the scoop if the user did not have a fork repo.
It could be an option, right?

@niheaven
Copy link
Member

niheaven commented Aug 7, 2022

The another reason is that I think the user can choose to hold the scoop if the user did not have a fork repo.
It could be an option, right?

That's what I'm against, that one should never hold scoop core since there're ongoing bugfixes and one may omit them if scoop is held. It's something dangerous even if there may be enough notices or warnings.

@yi-Xu-0100
Copy link
Contributor Author

@niheaven All right, if you resolutely retain this opinion. Could we add a configuration of SCOOP_HOLD_DAYS instead of it?

Just like windows, it could hold update operation for days. It could be a choice for the minority.

@niheaven
Copy link
Member

niheaven commented Aug 7, 2022

@niheaven All right, if you resolutely retain this opinion. Could we add a configuration of SCOOP_HOLD_DAYS instead of it?

Just like windows, it could hold update operation for days. It could be a choice for the minority.

That's great, and please make it!

@yi-Xu-0100 yi-Xu-0100 changed the title feat(scoop-config): Add new configuration of SCOOP_HOLD feat(scoop-config): Add new configuration of SCOOP_HOLD_DAYS Aug 8, 2022
@yi-Xu-0100
Copy link
Contributor Author

@niheaven The new configuration of SCOOP_HOLD_DAYS has been done. 😁

libexec/scoop-config.ps1 Outdated Show resolved Hide resolved
libexec/scoop-hold.ps1 Outdated Show resolved Hide resolved
libexec/scoop-update.ps1 Outdated Show resolved Hide resolved
Copy link
Member

@niheaven niheaven left a comment

Choose a reason for hiding this comment

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

Change the option name to SCOOP_HOLD and remove the option after holding days.

libexec/scoop-config.ps1 Outdated Show resolved Hide resolved
libexec/scoop-hold.ps1 Outdated Show resolved Hide resolved
libexec/scoop-unhold.ps1 Outdated Show resolved Hide resolved
libexec/scoop-update.ps1 Outdated Show resolved Hide resolved
yi-Xu-0100 and others added 12 commits August 9, 2022 09:07
Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
@yi-Xu-0100
Copy link
Contributor Author

@niheaven @r15ch13 @rashil2000 I think this feature is ready to be merged, are there other comments?

@niheaven
Copy link
Member

niheaven commented Aug 9, 2022

I'm fine expect for the try-catch (if-else IMO).

Copy link
Member

@rashil2000 rashil2000 left a comment

Choose a reason for hiding this comment

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

Looks fine to me

lib/core.ps1 Outdated Show resolved Hide resolved
yi-Xu-0100 and others added 3 commits August 9, 2022 16:40
Co-authored-by: Rashil Gandhi <46838874+rashil2000@users.noreply.github.com>
@rashil2000 rashil2000 merged commit ec04dd0 into ScoopInstaller:develop Aug 11, 2022
@r15ch13
Copy link
Member

r15ch13 commented Aug 11, 2022

The update_until value still does exactly the opposite of it's name.

@niheaven
Copy link
Member

niheaven commented Aug 11, 2022

So update_after? Or stop_update_until, hold_update_until.

@r15ch13
Copy link
Member

r15ch13 commented Aug 11, 2022

hold_update_until sounds better. It also needs to be added to the description in scoop-config.ps1

@niheaven
Copy link
Member

But this could not be set via scoop config, just likes lastupdate, since it is POSIX and is not human writable, in most cases.

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.

4 participants