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

Fix tig-pick output redirection #832

Merged
merged 2 commits into from
May 27, 2018
Merged

Fix tig-pick output redirection #832

merged 2 commits into from
May 27, 2018

Conversation

odnoletkov
Copy link
Contributor

Current version doesn't work for my environment (bash bundled with macOS):

$ sh --version
GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin17)
Copyright (C) 2007 Free Software Foundation, Inc.

The result commit is printed to stderr instead of stdout. I can't pinpoint the
problem but other traits in the script suggest that it is intended for bash
anyway. And it works fine with explicit bash for me.

@odnoletkov
Copy link
Contributor Author

cc @kgraefe

@kgraefe
Copy link
Contributor

kgraefe commented May 25, 2018

I don't think I tested the script with anything else than Bash. I did use a syntax checker to remove Bashisms, but idk, maybe that wasn't complete?

I'd be fine with that PR.

@rolandwalker
Copy link
Contributor

I don't understand why tig-pick doesn't simply use STDOUT.

You are right that there are some non-POSIX idioms in it.

Have you tried keeping /bin/sh and changing the non-POSIX echo -n?

diff --git i/contrib/tig-pick w/contrib/tig-pick
index e4789f303..d06f4023c 100755
--- i/contrib/tig-pick
+++ w/contrib/tig-pick
@@ -40,7 +40,7 @@ stderr=$(tig "$@" 3>&2 2>&1 1>&3 3>&-) || {
 commit=$(echo "$stderr" | tail -n1)

 # Check return value for valid commit ID
-if ! echo -n "$commit" | grep -iqE '^[0-9a-f]{40}$'; then
+if ! printf '%s' "$commit" | grep -iqE '^[0-9a-f]{40}$'; then
        echo "$stderr" >&2
        exit 1
 fi

@odnoletkov
Copy link
Contributor Author

@rolandwalker I've tried it and it does fix the issue as well. Turns out original problem was indeed -n not interpreted as an argument in sh-compatibility mode of my bash. Thanks!

I think it's better to stick with sh then so I've updated the pull request.

@jonas
Copy link
Owner

jonas commented May 27, 2018

LGTM, thanks for the fix.

@jonas jonas merged commit d74e28a into jonas:master May 27, 2018
@kgraefe
Copy link
Contributor

kgraefe commented May 27, 2018

@odnoletkov I didn't know that bash had a sh-compatibility mode. Thanks for mentioning.

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.

4 participants