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.

Sign up for GitHub

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: allow user to specify ssh or https access to an alternate site … #22

Closed
wants to merge 1 commit into from
Closed

feat: allow user to specify ssh or https access to an alternate site … #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions libexec/basher-_clone
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,26 @@

set -e

if [ "$#" -ne 1 ]; then
if [ "$#" -lt 1 ]; then
basher-help _clone
exit 1
fi

package="$1"
package=$1
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just keep "$1" here since users may give malformed input.

Copy link
Author

Choose a reason for hiding this comment

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

Assignments in bash don't do word splitting on variable expansions, quotes are only ever necessary if you use literal whitespace.

http://mywiki.wooledge.org/WordSplitting#Notes

Copy link
Contributor

Choose a reason for hiding this comment

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

@binaryphile I prefer always quoting variable unless requiring token splitting and wildcard expansion. Because "users may give malformed input", e.g. I want to install someone/program8, but I mistyped 8 to *, basher install 'someone/program*' and it happens there is a directory someone/program-sth, then basher will install program-sth. without any error. But if $1 is quoted, basher will try to install someone/program* and fails with some meaningful error message.

Copy link
Author

@binaryphile binaryphile Aug 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Wildcard expansion is not performed either for assignments.

e.g.:

> var_one=*
> declare -p var_one
declare -- var_one="*"

"declare -p" shows you the statement which generates the current contents of the variable. As you can see, "*" was not expanded prior to assignment, otherwise the contents of var_one would have been the directory listing of the current directory.

So if $1 contained a pattern, it would not be expanded as you were worried about:

> func_one() {
> local var_two=$1
> declare -p var_two
> }
> func_one "*" # bash strips these quotes before sending to the function as $1
declare -- var_two="*"

As you can see, no expansion due to assignment.

In any case, it's fine to quote anyway, and I used to, it's just not my style any more. Do as you like.

--edit: just to be clear, I'm not disagreeing with quoting variables when using them, absolutely you should! I've just learned that it's not strictly necessary with assignments so I leave them off there. You can also leave them off the subject of case statements and inside [[ ]] tests. In general, you should use shellcheck with vim and it will tell you when quoting is important, which is nice.


if [ -z "$package" ]; then
basher-help _clone
exit 1
fi

site=${2:-github.com}
Copy link
Contributor

Choose a reason for hiding this comment

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

What about "${2:-github.com}"?

Copy link
Author

@binaryphile binaryphile Aug 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't that what's there? If you mean without "site=", then that would try to run "github.com" as a command, not set it as the default value.

--edit: Oh I see, you mean the quotes. See my comment above.

proto=https

[[ $site == *@* ]] && {
IFS=@ read -r login site <<< "$site"
proto=ssh
}

IFS=/ read -r user name <<< "$package"

if [ -z "$user" ]; then
Expand All @@ -34,4 +42,6 @@ if [ -e "$BASHER_PACKAGES_PATH/$package" ]; then
exit 0
fi

git clone --depth=1 --recursive "https://github.com/$package.git" "${BASHER_PACKAGES_PATH}/$package"
[[ $proto == "ssh" ]] && repo=$login${login:+@}$site: || repo=$proto://$site/

git clone --depth=1 --recursive "$repo$package".git "${BASHER_PACKAGES_PATH}/$package"
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not handle name collision, e.g. github.com/sameuser/samerepo and bitbucket.org/sameuser/samerepo.

Copy link
Author

@binaryphile binaryphile Aug 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I don't see that as a valid use case. Presumably they are the same project if they have the same name, and you wouldn't clone two separate copies in that case, instead you would clone from one and then add the other as a new remote.

Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably they are the same project if they have the same name

In most cases, but it is possible they are different, e.g. when both sameuser and samerepo are short cool names.

Copy link
Author

Choose a reason for hiding this comment

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

As you wish.

4 changes: 3 additions & 1 deletion libexec/basher-install
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ if [ -z "$package" ]; then
exit 1
fi

[[ $package == *:* ]] && IFS=: read -r site package <<< "$package"

IFS=/ read -r user name <<< "$package"

if [ -z "$user" ]; then
Expand All @@ -28,7 +30,7 @@ if [ -z "$name" ]; then
exit 1
fi

basher-_clone "$package"
basher-_clone "$package" "$site"
basher-_deps "$package"
basher-_link-bins "$package"
basher-_link-man "$package"
Expand Down