helpers.sh: Robustify functions and don't exit 1 #985
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ermo
added
Bug
silkeh
previously approved these changes
Dec 6, 2023
|
boring I say let the users shell die that's fun! |
**Summary** Shellcheck recklessly introduced a bug that will make functions `exit 1` which causes the shell to exit, if the function in question doesn not complete successfully. This commit fixes this "small" oversight. In addition, this commit also makes the `gotopkg` and `goroot` functions more robust and yields hopefully useful error messages as necessary. **Test Plan** ``` ermo@solbox:~ $ goroot fatal: not a git repository (or any parent up to mount point /) Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set). If you're not somewhere in the solus packages git dir, use gotosoluspkgs instead. ermo@solbox:~/ $ gotosoluspkgs ermo@solbox:~/repos/getsolus/solpkgs $ cd ermo@solbox:~ $ gotopkg No package specified, going to the root of the solus packages git dir instead. ermo@solbox:~/repos/getsolus/solpkgs $ cd ermo@solbox:~ $ gotopkg grub2 ermo@solbox:~/repos/getsolus/solpkgs/packages/g/grub2 $ ``` Signed-off-by: Rune Morling <[email protected]>
ermo
force-pushed
the
fix-helpers.sh-script-killing-the-shell
branch
from
c1b4509 to
fc811fb
Compare
ermo
changed the title
sheepman4267
requested changes
Dec 7, 2023
There was a problem hiding this comment.
I see a couple issues here:
whatusesandwhatprovidesnow push you to the root of the repo, even if you're somewhere inside the repo already. That feels like it would be very, very frustrating.- Talking of that, do
whatusesandwhatprovidesneed to change the directory at all? I think not. Maybe we should:- Move these functions into a separate script, so they don't affect the user's shell at all, or
- Store the current directory, then change back when we've finished running our code? The former makes more sense to me, but I'm not great at shell scripts.
- Before, all functions other than
gotosoluspkgswould work just fine on a separate clone of the repo (as in, not the one which this script is sourced from). I find that useful. Maybe I'm just bad at git, but I like being able to have multiple clones, so I can (for example) have multiple branches checked out at once, and do comparisons between them manually. Havinggotopkgalways force me into my "main" clone of the repo would be a nuisance.
|
I agree with @sheepman4267 in his review. I would use |
TraceyC77
requested changes
Dec 7, 2023
| } | ||
|
|
||
| # What provides a lib | ||
| function whatprovides() { | ||
| gotosoluspkgs |
There was a problem hiding this comment.
I agree with @sheepman4267 that forcing the shell into a certain directory when it didn't before is disruptive to the user. This is not necessary if you use git -C.
Generally, if I need code to figure something out, I try to do it in a way that doesn't affect ther user environment if I can.
| @@ -1,30 +1,49 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| # shellcheck wants to add '|| exit 1' willy nilly, so let's | |||
There was a problem hiding this comment.
This needs to be reworded. Any comment starting with #shellcheck is interpreted by shellcheck as a directive.
| } | ||
|
|
||
| # Goes to the root directory of the git repository | ||
| function goroot() { | ||
| cd "$(git rev-parse --show-toplevel)" || exit 1 | ||
| local pkgroot="$(git rev-parse --show-toplevel)" |
There was a problem hiding this comment.
In helpers.sh line 21:
local pkgroot="$(git rev-parse --show-toplevel)"
^-----^ SC2155 (warning): Declare and assign separately to avoid masking return values.
|
Sounds like a (way) smarter/better approach is needed here, so closing. The good thing is that the shell will still exit 1, so will make interested parties more likely to come up with a set of fixes with nice properties. Best of luck. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Shellcheck recklessly introduced a bug that will make functions
exit 1which causes the shell to exit, if the function in question doesn not complete successfully.This commit fixes this "small" oversight.
In addition, this commit also makes the
gotopkgandgorootfunctions more robust and yields hopefully useful error messages as necessary.Test Plan