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

Disable tty pass through to talisman binary in pre-push hook mode #246

Merged
merged 2 commits into from Sep 8, 2020

Conversation

svishwanath-tw
Copy link
Collaborator

The git refs sent via stdin to hook script are lost with exec </dev/tty

@dineshba
Copy link
Collaborator

dineshba commented Sep 7, 2020

Hope you have done manual testing

@svishwanath-tw svishwanath-tw merged commit d01b683 into thoughtworks:master Sep 8, 2020
Copy link
Contributor

@derwent-m derwent-m left a comment

Choose a reason for hiding this comment

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

Noticed a bug caused by this merge.

Comment on lines -82 to +85
[[ $(toLower "${TALISMAN_INTERACTIVE}") == "true" ]] && INTERACTIVE="-i"
if [ $(toLower "${TALISMAN_INTERACTIVE}") == "true" ]; then
INTERACTIVE="-i"
[[ "${HOOKNAME}" == "pre-commit" ]] && exec < /dev/tty || echo_warning "talisman pre-push hook cannot be invoked in interactive mode currently"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey there, just noticed that since you've change the [[ ... ]] to if [ ... ], $(toLower "${TALISMAN_INTERACTIVE}") now needs to be quoted, otherwise you will get

.git/hooks/bin/pre-commit: line 82: [: ==: unary operator expected

if $TALISMAN_INTERACTIVE is not set.

Easy mistake to make :P

if [ "$(toLower "$TALISMAN_INTERACTIVE")" == "true" ]; then

does the trick!

derwent-m pushed a commit to derwent-m/talisman that referenced this pull request Sep 8, 2020
@svishwanath-tw svishwanath-tw deleted the pre-push-fix branch September 8, 2020 09:53
svishwanath-tw pushed a commit that referenced this pull request Sep 8, 2020
…249)

* [Resolves #170] Add pre-commit option in install.sh

* [Issue #247] update VERSION and SHAs to v1.8.0

* [Issue #247] download talisman_hook_script.bash along with talisman into .git/hooks/bin

* [Issue #247] 🚨 Fix shellcheck warnings

* [Issue #247] 📝 update doc, single project install

* 📝 fix inaccuracy in documentation

* 🐛 Handle bug introduced in #246

Co-authored-by: derwentx <derwentx@gmail.com>
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.

None yet

3 participants