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

Improve the git-sed command #709

Merged
merged 3 commits into from May 3, 2018
Merged

Improve the git-sed command #709

merged 3 commits into from May 3, 2018

Conversation

pabs3
Copy link
Contributor

@pabs3 pabs3 commented Apr 20, 2018

Discover a separator when the / character is used in arguments.
Prevent running sed when the search pattern does not match any files.
Allow the flags to be passed as a third argument.

This is more in-line with how sed itself works.
@spacewander
Copy link
Collaborator

A little question:
What's the reason to allow the flag to be passed as the third option?

@pabs3
Copy link
Contributor Author

pabs3 commented Apr 20, 2018 via email

@spacewander
Copy link
Collaborator

@pabs3
I see.

bin/git-sed Outdated
sep=/
;;
esac
command="git grep -lz '$search' | xargs -0r sed -i 's$sep$search$sep$replacement$sep$flags'"
Copy link
Collaborator

Choose a reason for hiding this comment

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

-r option is a GNU extension. The xargs in OS X doesn't support it.
We have to remove it for the compatibility.

@@ -3,12 +3,12 @@ git-sed(1) -- replace patterns in git-controlled files

## SYNOPSIS

`git-sed` [ -c ] [ -f <flags> ] <search> <replacement>
`git-sed` [ -c ] [ -f <flags> ] <search> <replacement> [ <flags> ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also need to update the html and man page.
See https://github.com/tj/git-extras/blob/master/man/Readme.md

@pabs3
Copy link
Contributor Author

pabs3 commented Apr 21, 2018 via email

@spacewander
Copy link
Collaborator

I've updated my branch to add a mechanism to check if the xargs -r option exists and only use it when it does exist.

Does it cause different behaviors between different platforms?
Maybe we'd better check the output of git grep by ourselves?

@pabs3
Copy link
Contributor Author

pabs3 commented Apr 25, 2018 via email

@spacewander
Copy link
Collaborator

running git grep twice instead of once

Maybe we could store the result in a bash variable?

@pabs3
Copy link
Contributor Author

pabs3 commented Apr 25, 2018 via email

@spacewander
Copy link
Collaborator

@pabs3
OK.
Where is your latest change? I could not find it in this pull request.

@pabs3
Copy link
Contributor Author

pabs3 commented Apr 26, 2018 via email

@pabs3
Copy link
Contributor Author

pabs3 commented Apr 26, 2018 via email

bin/git-sed Outdated
sep=/
;;
esac
r=$(xargs -0r false < /dev/null > /dev/null 2>&1 && echo r)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could use -r instead of -0r here, since we only want to test for -r?

Prevents running sed when the search pattern does not match any files.

This results in sed printing an unnessecary warning:

$ git sed foo bar
sed: no input files
Prevents sed from returning an error for arguments containing filenames:

$ git sed src/foo.c src/bar.c
sed: -e expression tj#1, char 13: unknown option to `s'
@spacewander spacewander merged commit 4325238 into tj:master May 3, 2018
@spacewander
Copy link
Collaborator

@pabs3
Merged. Thank you!

@spacewander
Copy link
Collaborator

@pabs3
Will you submit another pull request to update the documentation?

@pabs3 pabs3 deleted the git-sed branch May 5, 2018 04:43
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

2 participants