Skip to content

add $pipestatus support#5632

Closed
zabereer wants to merge 13 commits into
fish-shell:masterfrom
zabereer:pipestatus
Closed

add $pipestatus support#5632
zabereer wants to merge 13 commits into
fish-shell:masterfrom
zabereer:pipestatus

Conversation

@zabereer
Copy link
Copy Markdown
Contributor

@zabereer zabereer commented Feb 9, 2019

Add $pipestatus support

Add $pipestatus variable similar to other shells. It is handy for status lines.
I frequently build software piping make output into grep and other commands, ending it with ;exit $pipestatus[1] so final status is that of make and not of grep etc.

Fixes issue #2039

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md

I will add CHANGELOG.md and update pull request. I love fish and have recently switched to it!!!

Comment thread src/env.cpp
Comment thread src/env.cpp Outdated
Comment thread src/proc.cpp
@faho
Copy link
Copy Markdown
Member

faho commented Feb 9, 2019

One big thing that #2039 left open was how logical operators are handled.

E.g.

! true

would in bash have a $? of 1 and a $PIPESTATUS of 0.

How does this handle that?

@faho faho added this to the fish 3.1.0 milestone Feb 9, 2019
@zabereer
Copy link
Copy Markdown
Contributor Author

zabereer commented Feb 9, 2019

Oh yes, I completely forgot about that. As it stands it behaves like bash and zsh.
! true; echo $pipestatus gives 0.

@faho
Copy link
Copy Markdown
Member

faho commented Feb 9, 2019

! true; echo $pipestatus gives 0.

Personally, that seems entirely counter-intuitive, but it's what bash and zsh do, so there's precedent.

I'd like to have that hashed out and nailed down before merging anything, because changing it would be tough.

@fish-shell/contributors Any opinions?

@zabereer
Copy link
Copy Markdown
Contributor Author

zabereer commented Feb 9, 2019

Yes completely agree. It is a tough one. It will either be confusing to other shell (zsh/bash) users switching to fish, or to new shell users that start out on fish.

@faho
Copy link
Copy Markdown
Member

faho commented Feb 9, 2019

Okay, I can't find any further specification in the bash manual or the zsh manual

In fact the sum total documentation for both seems to be

pipestatus
An array containing the exit statuses returned by all commands in the last pipeline.

PIPESTATUS
An array variable (see Arrays below) containing a list of exit status values from the processes in the most-recently-executed fore‐ground pipeline (which may contain only a single command).

Most questions about PIPESTATUS on stackoverflow also don't reference this fact.

The only answer I can find is https://stackoverflow.com/a/36447447/3150338, which explains:

! a | b | c is interpreted as !{a | b | c;} by the shell

and recommends using subshells.

The (approximate) equivalent for that in fish would be begin and end, so

begin; not a; end | b | c

So... we obviously don't have a problem with deviating from other shells. I'd argue that we should do so if there is a cause. E.g. if something is a common problem (like word splitting).

In this case it seems like it's not a common problem (or I'd have found more on stackoverflow), and I'm not entirely sure it is a problem.

Because the ! a | b | c behavior makes a certain amount of sense. The negation doesn't apply to the first command. $? reflects the status of the last command, so it might need to be applied there, if anything. (It's not, ! true | true | true has a PIPESTATUS of 0 0 0)

It's just quite a surprise in the ! a case.

So, as far as I'm concerned, applying logical operations to the first command doesn't make any sense, applying it to the last might.

Or we could keep it like bash and zsh apparently do and explain it in the docs.

@mqudsi
Copy link
Copy Markdown
Contributor

mqudsi commented Feb 9, 2019

Because the ! a | b | c behavior makes a certain amount of sense.

Only if you look at the most basic examples. It's less obvious here: ! true && true | false has less clear order of precedence for the operators. and and or were simpler in that regard since they required the termination of an expression with a literal ;, but && and || hide that.

The core problem is that the fish doesn't have an elegant grouping operator. As a side effect of fish's adoption of undecorated parentheses as subshell operators, there's no clean way of explicitly specifying (or, even better, overriding) the order of operations at the prompt without using functions, if statements, temporary variables, etc. (Not that sh/bash's use of parentheses is actually clean, it makes it look like it's providing order-of-operations grouping but in reality it's just spawning a subshell that evaluates to the status of the inner contents (rather than executing their output as $(something) subshells do.)

That said, I would lose literally zero sleep over deviating from other shells with our implementation. $PIPESTATUS is pretty arcane even for pro shell scripting, and we've not had too many complaints about not having it at all until now. Someone using fish for scripting getting this involved in the minutia will inevitably have run into other compatibilities between sh and fish by the point they get to $pipestatus, so it's not like it's coming out of the blue.


However, more to the point, I think any $pipestatus implementation for fish should not ship without an accompanying conditional piping story. ba(sh) has set -o pipefail, I think fish can do better and provide something more fine-grained/localized than a global shell flag, e.g. something like && | (along the lines of our 2>| syntax), which is actually timely because of the improvements to io buffering that @ridiculousfish recently committed (since the output would need to be buffered until the status code has been determined - but now that I think about it, I'm not sure if the posix version guarantees the next command in the pipeline won't be executed if an upstream status is non-zero or merely that the script will terminate when an upstream status can be resolved and returns non-zero).

@faho
Copy link
Copy Markdown
Member

faho commented Feb 9, 2019

That said, I would lose literally zero sleep over deviating from other shells with our implementation

Sure, but should we? Is there enough of a point in doing so?

I'm not sure if the posix version guarantees the next command in the pipeline won't be executed

Bash's man page says:

If set, the return value of a pipeline is the value of the last (rightmost) command to exit with a non-zero status, or zero if all commands in the pipeline exit successfully. This option is disabled by default.

So it seems to be just about the status.

Which means it can be emulated with something like string match -v 0 -- $pipestatus.

merely that the script will terminate when an upstream status can be resolved and returns non-zero

Pipefail doesn't, errexit does. And errexit is terrible, because a non-zero return is not a universal indication of an exceptional error. (The "unofficial strict mode" is set -euo pipefail, IIRC)

See e.g. de32665 and https://mywiki.wooledge.org/BashFAQ/105.

However, more to the point, I think any $pipestatus implementation for fish should not ship without an accompanying conditional piping story.

Why not? I mean it's nice to have, but we've gotten by without $pipestatus, $pipestatus is useful on its own and there's no backwards-compatibility implications here, so it would be harmless to ship $pipestatus without that stuff.

@floam
Copy link
Copy Markdown
Member

floam commented Feb 9, 2019

The begin; ! true; end | false; thing is confusing for sure, it's not worth following bourne shell there if it falls out from that. I seem to recall that elvish shell has a really good story when it comes to pipelines. We might want to look there for inspiration rather than bash/zsh/ksh.

@faho
Copy link
Copy Markdown
Member

faho commented Feb 9, 2019

Okay, as it stands I have no idea how I'd explain this:

$ ! true | false; echo $pipestatus = $status
0 1 = 0
$ begin; ! true; end | false; echo $pipestatus = $status
0 1 = 1

The first I can explain with "the negation is applied to the entire pipeline", but the second does not make any sense. Even if I wrap it:

function nottrue; not true; return $status; end
nottrue | false; echo $pipestatus = $status

Also $pipestatus seems to be cleared by the prompt:

$ nottrue | false
$ echo $pipestatus = $status

prints "0 = 1".

@zabereer: IIRC, there's an idea of "apply_exit_status" that the prompt code sets to false. This should also affect $pipestatus.

@lilyball
Copy link
Copy Markdown
Contributor

lilyball commented Feb 9, 2019

Having not affect $pipestatus just doesn't make any sense, since not applies to the entire pipeline rather than a single command. $pipestatus reflects the status of each command, and $status the status of the entire pipeline. So I think we should match Bash here and have not false | false; echo $pipestatus = $status print 1 1 = 0. Besides making more sense to me, this also has the practical benefit where I can write code like

if not foo | bar | baz
  # inspect $pipestatus to see the statuses of foo, bar, and baz
end

Regarding conditional piping, pipefail doesn't prevent latter commands in the pipeline from starting because, at least AIUI, other shells start all commands simultaneously (and deal with shellscript in the pipeline by forking the shell). Fish is rather special in that putting fishscript into a pipeline buffers output at that point and delays execution of the latter commands. I don't think we should add an explicit "conditional pipeline" operator because the delayed-launch behavior is undesired behavior but something that a "conditional pipeline" would require. That said, I can see why it might be useful in some niche cases, and if you want this, it should be discussed in a separate issue.


$ begin; ! true; end | false; echo $pipestatus = $status
0 1 = 1

This definitely seems wrong. $pipestatus should contain the statuses of the two commands begin; ! true; end and false, which both should be 1.

I also agree that the prompt should not reset $pipestatus.

@faho
Copy link
Copy Markdown
Member

faho commented Feb 9, 2019

Having not affect $pipestatus just doesn't make any sense, since not applies to the entire pipeline rather than a single command.

My idea was to apply not to the last command, since that's effectively what $status does.

However, not crushes the nice int down to a boolean and in doing so loses information:

function returns5
    return 5
end

not return5;  echo $status # prints 0 obviously

Since the not is explicitly there, the user always has enough information to apply it to $pipestatus as well, if they want.

It's a bit weird because it could be both not a | b and a | not b, but I'd imagine that in practice nobody is going to hit that. And that's weird already:

true | not false | true

has a $status of 1. Because the not applies to the entire pipeline, i.e. the last true.

So I'd be alright with just doing what bash does, as long as the documentation is better than theirs.

@mqudsi
Copy link
Copy Markdown
Contributor

mqudsi commented Feb 10, 2019

Honestly, it makes more sense to me to have $pipestatus correspond to, literally, the status at the pipe. i.e. foo | bar should result in a $pipestatus with only one entry, corresponding to "$status if there were no pipe after foo, and $status remains available to inspect the status of the last command. Then you don't need to worry about the negation.

@lilyball
Copy link
Copy Markdown
Contributor

@mqudsi That doesn't scale past a single pipeline, such as foo | bar | baz.

@mqudsi
Copy link
Copy Markdown
Contributor

mqudsi commented Feb 10, 2019

@lilyball I think I'm missing something, because I'm envisioning $pipestatus after foo | bar | baz expanding to (exit status of foo) (exit status of bar) (three commands, two pipes, two elements in $pipestatus)

@lilyball
Copy link
Copy Markdown
Contributor

lilyball commented Feb 10, 2019

Ok so you're actually proposing just the same $pipestatus but missing the last element? And so running a command with no pipe at all, e.g. foo, would cause $pipestatus to have no entries?

I see no benefit to throwing away this value. Do note that the value you're proposing to throw away is not in fact the same thing as $status, because as we've already determined in a not foo | bar pipe, $pipestatus[-1] should contain the exit value of bar, whereas $status contains the negation.

@ridiculousfish
Copy link
Copy Markdown
Member

I initially merged this as ec29020 but then saw there was pending discussion, so I reverted as 1701e2c

@ridiculousfish
Copy link
Copy Markdown
Member

Really happy to see this PR, thanks for doing this!

As faho points out fish does not have per-process negation (despite the fact that the syntax looks supported). I think the way to think about $pipestatus is as the exit status of each individual process (which cannot be negated), while $status is the exit status of the entire job (which can be negated). This is simple to understand and explain, and matches bash.

@zabereer
Copy link
Copy Markdown
Contributor Author

@lilyball I completely agree with you, that is how I would expect $pipestatus to work. Initially I thought it should deviate from bash and zsh but after reading your and @faho responses I am now thinking it actually makes sense to follow bash and zsh in this regard (with better documentation).
@faho thank you I appreciate it I will address that, I removed some code I thought was superfluous which would have preserved $pipestatus to prompt.

@zabereer
Copy link
Copy Markdown
Contributor Author

@ridiculousfish Thank you, that is very encouraging.

@zabereer
Copy link
Copy Markdown
Contributor Author

Last commit preserves $pipestatus to prompt. Will tackle negate next.

@ridiculousfish
Copy link
Copy Markdown
Member

ridiculousfish commented Feb 13, 2019

Will tackle negate next

What's are you planning for negate? If we like the bash / zsh behavior then $pipestatus should ignore job negation, i.e. there's nothing to do.

@floam
Copy link
Copy Markdown
Member

floam commented Feb 13, 2019

After adding this, we should soup-up at least one of the included prompts to display the pipeline status in a cool way, maybe iff. things aren't 0.

@lilyball
Copy link
Copy Markdown
Contributor

My old Bash prompt displays pipe statuses. It displays the status area in red if the pipeline failed and grey if the pipeline succeeded. If there's a pipeline, it displays the individual command statuses. If $? disagrees with the final pipeline command, it indicates this with parens. Here's a screenshot that demonstrates the various states it has:

screen shot 2019-02-13 at 2 25 43 pm

Here's my entire prompt command. I don't remember what the commented-out code is for, I wrote this many years ago:

__prompt_command() {
	local status=$? pipestatus=("${PIPESTATUS[@]}")
	local prompt=""

	prompt+='\[\e[1;32m\]\u\[\e[m\]@\[\e[1;36m\]\h\[\e[m\]:\[\e[1;31m\]'

	prompt+='$(__prompt_path)'

	prompt+='\[\e[0;33m\]'
	prompt+='$(__git_ps1 " (%s)")'
	prompt+='\[\e[m\]'

	local promptstatus=
	if (( status > 255 )); then
		# status > 255 indicates a special return from bash, such as
		# a syntax error. It appears that this type of return does not
		# modify PIPESTATUS.
		promptstatus=$status
	else
		for x in "${pipestatus[@]}"; do
			if (( x )); then
				promptstatus=${pipestatus[*]}
				break
			fi
		done
		if (( status != pipestatus[${#pipestatus[@]}-1] )); then
			promptstatus+="${promptstatus:+ }($status)"
			#if [[ -n $promptstatus ]]; then
				#promptstatus+=" ($status)"
			#elif (( status )); then
				#promptstatus=$status
			#fi
		fi
	fi
	if [[ -n $promptstatus ]]; then
		local color=31
		if (( status == 0 )); then color=2; fi
		prompt+=" \\[\\e[${color}m\\][$promptstatus]\\[\\e[m\\]"
	fi
	prompt+='> '

	PS1=$prompt
}

I very much like the way this displays statues and would suggest doing something similar if we want to bundle this with a Fish prompt. I'd suggest wrapping up this logic in a prebaked fish_prompt_status command except for the fact that it needs to save the $status and $pipestatus variables at the beginning of the prompt command.


Looking at that code, it has special handling for exit codes greater than 255 because Bash 3.x doesn't modify $PIPESTATUS in that case¹. Experimentally it looks like Fish sets $status = 127 in the syntax error case; this is a valid command exit status so we can't use it to detect a syntax error. We should make sure that we do in fact update $pipestatus in the case of a syntax error (the fact that Bash doesn't is IMO a bug).

¹Testing right now, neither does Bash 4, but Bash 4 also doesn't use $? > 255 anymore, so it's even worse, which is rather unfortunate.

@floam
Copy link
Copy Markdown
Member

floam commented Feb 14, 2019

Here's what elvish looks like after part of the pipe fails:

@floam
Copy link
Copy Markdown
Member

floam commented Feb 14, 2019

Actually, that's an old screenshot of mine. That's not how it looks today. It looks like this:

image

They reprint the command line with the exceptional part underlined.

Seems to me something like carats pointing up at the previous commandline would be neat.

@zabereer
Copy link
Copy Markdown
Contributor Author

Will tackle negate next

What's are you planning for negate? If we like the bash / zsh behavior then $pipestatus should ignore job negation, i.e. there's nothing to do.

@ridiculousfish, sorry I should have been more clear. The job level negation works like bash/zsh. However when one does:

begin; ! true; end;  echo $pipestatus : $status

it deviates from zsh and bash by returning 0 : 1 instead of 1 : 1 as @faho and @lilyball pointed out.

@zabereer
Copy link
Copy Markdown
Contributor Author

zabereer commented Feb 17, 2019

Using blocks in pipes seem to work. When not using a pipe the results are as follows (similar to bash but not zsh):
fish:

~ $ begin; ! false; end; echo $pipestatus : $status
1 : 0
~ $ ! false; echo $pipestatus : $status
1 : 0

bash:

~ $ { ! false; }; echo ${PIPESTATUS[@]} : $?
1 : 0
~ $ ! false; echo ${PIPESTATUS[@]} : $?
1 : 0

zsh:

~ $ { ! false }; echo $pipestatus : $?
0 : 0
~ $ ! false; echo $pipestatus : $?
1 : 0

I have left the tests for tying this down commented out as I am torn between what bash does and what zsh does regarding this.

@zabereer
Copy link
Copy Markdown
Contributor Author

Committed the tests I mentioned above. $pipestatus behaves like bash's. It also behaves like zsh's except for list statement without any pipes as I mentioned in previous post.
Also I discovered that fish allows the not or negate to be specified anywhere in the pipe whereas other shells consider that a syntax error. The following gives the same result in fish, but syntax error in bash and zsh:

~ $ ! true | false; echo $pipestatus : $status
0 1 : 0
~ $ true | ! false; echo $pipestatus : $status
0 1 : 0

@zabereer
Copy link
Copy Markdown
Contributor Author

I added a man page for pipestatus, it turned out longer than I had hoped (seems one can say a lot about it). I read in FORMATTING.md that it is undesirable to add new pages, so please suggest where to put documentation for this (bash and zsh is rather light on pipestatus documentation, so we can definitely do documentation better for fish).
So, as it stands fish $pipestatusbehaves like bash. It also behaves like zsh apart from the result of begin; ! false; !endwhere zsh is IMHO doing it correctly. It seems fish (and bash) drops the block surrounding the list/group command if it is a single command. So that parses to just ! false.
Please let me know if this is ok, or whether you would like me to address that and follow zsh rather than bash in this regard. (I documented this in the man page).

@zabereer
Copy link
Copy Markdown
Contributor Author

@lilyball that last commit should address what you said about setting $pipestatus on parse error.

@zabereer
Copy link
Copy Markdown
Contributor Author

@floam I will look into adding $pipestatus to one of the stock prompts - any preference which one? Maybe Classic + Status?
Regarding mapping values > 128 to signal names (like 130 -> "SIGINT"), any advice on how to do that would be very welcome. Currently I am thinking a fish script function to do that. Alternative could be done in builtin - but I think that is at the wrong level.

@zabereer
Copy link
Copy Markdown
Contributor Author

@faho are you happy with the changes I made to address your review?

@zanchey
Copy link
Copy Markdown
Member

zanchey commented Feb 21, 2019

Rather than a separate manpage, the documentation should go into the manual (probably in a new subsection after "the status variable").

@zabereer
Copy link
Copy Markdown
Contributor Author

@zanchey Sure I will move it.

@zabereer
Copy link
Copy Markdown
Contributor Author

I update the documentation as suggested, thank you.

@ridiculousfish
Copy link
Copy Markdown
Member

I'm merging this, I think it's been though plenty of discussion! If someone feels strongly about how not interacts feel free to open a new issue.

Squash-merged as 2c8abdf

@zabereer thank you for your great contribution and mighty patience!

@zanchey zanchey removed the RFC label Feb 25, 2019
@zabereer zabereer deleted the pipestatus branch February 26, 2019 05:51
@58bits 58bits mentioned this pull request Jul 18, 2019
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants