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

effort: handling of $'s and \n's, plus make shellcheck happy #622

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nicolaiskogheim
Copy link
Collaborator

@nicolaiskogheim nicolaiskogheim commented Jan 27, 2017

This fixes an issue that was reported this week, with $'s in a filename breaking git log and take git effort with it. git effort now understands how to treat files with actual newlines in them as well.

Shellcheck had some complaints, and I agreed to all of them. Because of that, there is a lot of code to review, so please look at each commit individually, if that makes it easier.

While I were at it, I expanded upon the docs, and made some small rewordings. If there are many comments on the doc edits, I'll split that out in it's own PR.


Please help test this

  • macOS
  • Linux
  • BSD

You can use this repo as a test repo. It contains files with names that should produce an error if the issue isn't fixed.

if [ -z "$(head -c 1 <<<"$(echo "$path")")" ]
then
exit 0
fi
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like printf (that produces output to effort) sends a final empty line when we call effort. That's the reason for this check.

@spacewander
Copy link
Collaborator

Sorry for my missing of this.
Pass under Linux with bash 4.3.30.

@nicolaiskogheim
Could we merge the pr now?

@nicolaiskogheim
Copy link
Collaborator Author

I'll fire up a FreeBSD box tomorrow and run the script just to be sure.

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.

2 participants