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

Fall back to 'tac' [sic] when 'rev' is not available (see comments) #39

Closed

Conversation

ernstki
Copy link
Contributor

@ernstki ernstki commented May 17, 2020

NOTE: tac is not a substitute for rev, and this PR was completely wrongheaded from the start. See this comment.

- should allow toolbelt to run in Git Bash on Windows

- closes nvie#29 (hopefully)
Copy link
Owner

@nvie nvie left a comment

Choose a reason for hiding this comment

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

One small request - what do you think? Very glad to get these PRs and upgrade support on non-Mac platforms :)

@@ -113,7 +113,9 @@ if [ -z "$commit" ]; then
fi
else
TAB=" " # literal tab char
git log -1 --name-status --pretty=format:"" "$commit" | cut -f2- | rev | cut -d"$TAB" -f1 | rev | make_relative | while read f; do
rev='rev'
type rev >/dev/null 2>&1 || rev='tac'
Copy link
Owner

Choose a reason for hiding this comment

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

Same request as in the other PR: let's not assume tac exists if rev does not, instead let's just do a test. In this case, you could make it a helper function:

reverse () {
  if type rev >/dev/null 2>&1; then
    rev
  elif type tac >/dev/null 2>&1; then
    tac
  else
    echo "Core utility 'rev' (nor 'tac') not found." >&2
    exit 2
  fi
}

And then call reverse in the pipe line, like

git log ... | reverse | cut -d"$TAB" -f1 | reverse | ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can totally do that. I just wasn't sure initially which side of the human-friendly-wrapper-functions-vs-lines-of-code question you would come down on, so I tried to keep the changes small.

Do you have strong opinions about line length? I would really like to break pipes over multiple lines at ~78 columns, but if it doesn't bother you, I won't let it bother me.

Copy link
Owner

Choose a reason for hiding this comment

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

I can totally do that. I just wasn't sure initially which side of the human-friendly-wrapper-functions-vs-lines-of-code question you would come down on, so I tried to keep the changes small.

Makes sense, it's a good default! In this case, the little extra helper function makes things easier to understand, so I'm ok with it :)

Do you have strong opinions about line length? I would really like to break pipes over multiple lines at ~78 columns, but if it doesn't bother you, I won't let it bother me.

That's fine in this case. This one was getting quite long in particular 👍

@ernstki
Copy link
Contributor Author

ernstki commented Jul 8, 2020

I had a serious misunderstanding about tac vs. rev; tac reverses linewise, re-ordering the lines in the file, but rev reverses character-wise, without changing the top-to-bottom order of the lines.

They are not drop-in replacements for one another like I mistakenly assumed they were without carefully reading the man pages.

I'm going to close this for now and reopen (with the other fixes you suggest already implemented) once I'm sure I'm doing the right thing here.

@ernstki ernstki closed this Jul 8, 2020
ernstki referenced this pull request in ernstki/git-toolbelt Sep 17, 2020
@ernstki ernstki changed the title Fall back to 'tac' when 'rev' is not available Fall back to 'tac' [sic] when 'rev' is not available (see comments) Sep 17, 2020
@ernstki ernstki deleted the 29-fall-back-to-tac-for-git-bash-windows branch September 17, 2020 15:08
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.

Support on Windows OS
2 participants