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

Closed
wants to merge 1 commit into from

Conversation

binaryphile
Copy link

@binaryphile binaryphile commented Aug 14, 2016

…such as bitbucket

Only affects the install command, all others don't need to change.

Defaults to the same syntax as before, goes to github https if not otherwise specified.

Extended syntax complies with git ssh access syntax ([email protected]:user/name), uses colon to separate server from repo path. Https access is like so:

bitbucket.org:user/name

Ssh access is triggered by the presence of the @ sign like so:

[email protected]:user/name

Does not work for specifying dependencies in package.sh since : is already used as a separator. I've got no plans for fixing this.

Cheers,

Ted

@binaryphile binaryphile deleted the site branch August 15, 2016 03:36
@binaryphile binaryphile restored the site branch August 15, 2016 03:37
@binaryphile binaryphile reopened this Aug 15, 2016
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

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.

@weakish
Copy link
Contributor

weakish commented Aug 28, 2016

Does not work for specifying dependencies in package.sh since : is already used as a separator. I've got no plans for fixing this.

@binaryphile What if change : to / in prefix? bitbucket.org/user/name and [email protected]/user/name

@binaryphile
Copy link
Author

binaryphile commented Aug 28, 2016

re: the change to / in prefix...it's been a little while since I made the pull request so I don't remember clearly but I do believe I tried that. I don't remember whether I ran into a technical issue or simply disliked the negative user experience of using an unexpected syntax for the ssh protocol. I think it was the bad user experience that swayed me, and the fact that I didn't have a use case where I needed the syntax to work for my dependencies.

I would prefer the : syntax but I certainly wouldn't blame you if you didn't want to change the existing dependency syntax.

I maintain my own fork of basher so it doesn't bother me either way. I'll probably stick with : there.

@juanibiapina
Copy link
Member

This is a great discussion. I'll take a look at the pull request in the near future and think about how to integrate this multi source functionality to the whole concept. The tests are not passing though.

@binaryphile What's different about your fork? maybe we could bring in some of the changes.

@binaryphile
Copy link
Author

I do active development with my own packages that are under basher's cellar directory, so I have two changes. The first is the one in this pull request, to allow access to private repos via ssh. The second is to do full histories by not using "--depth=1" in the clone command. I don't have any kind of configuration to specify it, I just edited it out of the command, so I haven't turned that into something suitable for a pull request. I'm probably not likely to any time soon either since it's such a simple tweak for me.

@juanibiapina
Copy link
Member

@binaryphile I added an option in #24 to allow full clones, since I also do local package development.

@juanibiapina
Copy link
Member

@binaryphile I have implemented this on issues #21 and #25 , see if you like it.

@juanibiapina
Copy link
Member

Closing because it has been a while.

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.

3 participants