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.

Sign up for GitHub

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

Jump to bottom

Fix gwip alias for some BSD flavors of xargs #2714

Closed
wants to merge 2 commits into from
Closed

Fix gwip alias for some BSD flavors of xargs #2714

wants to merge 2 commits into from

Conversation

ncanceill
Copy link
Contributor

Status: Ready (but testing cannot hurt)

Some BSD flavors of the xargs command (eg on Mac OSX) do not support the -r flag — which is actually a non-standard flag introduced by the GNU flavor of xargs. This breaks the gwip alias in the current git plugin. See #2692 and #2713.

The bug was introduced by a869ec9. The original intention was to take care of the possibility that git ls-files --deleted -z would not send anything in the pipe. However, the author @bdubertret failed to see that, in that case, only the GNU flavor of xarg fails.

This is because the GNU flavor does not follow the xargs standard, and fails on empty input instead of returning 0 silently: it requires the -r flag in order to adopt the standard behavior. Apparently, all BSD flavors abide by the standard, and thus need not the -r flag; some of them (eg FreeBSD) still silently support the flag for the sake of compatibility, but others (eg Mac OSX) do not and fail in case they see it. Hence this PR.

@ncanceill ncanceill mentioned this pull request Apr 11, 2014
Closed
@mcornella
Copy link
Member

In my system (debian jessie, xargs 4.4.2) your test makes xargs run indefinitely, until some input is passed (or ^D is pressed).
Also, &>/dev/null at the beginning spits &>/dev/null: command not found on some occasions.
This is how it is run, seen with zsh -xv:

XARGS_OPTS=
+~/.oh-my-zsh/plugins/git/git.plugin.zsh:154> XARGS_OPTS='' 
&>/dev/null xargs -r && XARGS_OPTS+="-r"
+~/.oh-my-zsh/plugins/git/git.plugin.zsh:155> xargs -r

What works for me:

echo | xargs -r &>/dev/null && XARGS_OPTS="-r"

I have tested it with a custom function xargs() that returns 1 and it works ok too, but it should be tested on a non-GNU xargs version.

Finally, (1) the first line could be removed since an undefined variable is empty, and (2) to prevent possible side effects we should unset XARGS_OPTS after the alias.

EDIT: the command not found thing had to do with one function provided by the debian package command-not-found, which is just horribly made. The command is parsed correctly, as far as I've tested, but it seems good practice to put the &>/dev/null thing last.

@ncanceill
Copy link
Contributor Author

Awesome feedback, will update soon, thanks.

mcornella
mcornella reviewed Apr 15, 2014
View reviewed changes
plugins/git/git.plugin.zsh

# these aliases commit and uncommit wip branches
# first though, check xargs flavor for -r flag
echo | xargs -r &>/dev/null && XARGS_OPTS+="-r"
Copy link
Member

Choose a reason for hiding this comment

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

Minor correction: XARGS_OPTS="-r" 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, if any other file sets that option there may be side-effects.

EDIT: OK, done.

@mcornella
Copy link
Member

This looks ready 👍

@ncanceill ncanceill mentioned this pull request May 7, 2014
Merged
5 tasks
@ncanceill
Copy link
Contributor Author

Obsoleted by #2790

@ncanceill ncanceill closed this May 7, 2014
@ncanceill ncanceill deleted the fix-gwip branch May 7, 2014 17:06
@mcornella mcornella mentioned this pull request May 20, 2014
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ncanceill @mcornella