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

`history --delete` (fish script, not builtin) is broken by entries with newlines #31

Closed
ridiculousfish opened this issue Jun 5, 2012 · 5 comments
Assignees
Labels
bug
Milestone

Comments

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Jun 5, 2012

Run this:

foo\
bar

Then this:

history --delete --prefix foo

You get two items foo\ and bar; there should only be one.

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Jul 15, 2016

The following comment is from issue #3224 that I opened because I didn't find this issue when I did a superficial search.

The history --delete command is broken. If your history contains multiline commands each line is listed as an individual entry you can select for deletion. That's obviously bogus. It might be possible to fix that. However, I'm wondering if it wouldn't make more sense to simply change the UI to eliminate selecting individual entries for deletion. Just give the user the choice of deleting all or none of the matching entries.

Also, including the recently introduced -t or --with-time flag produces output that makes selecting an individual entry for deletion impossible. Yet another reason to remove that capability.

I noticed this while working on issue #3224.

@krader1961 krader1961 changed the title History function mishandles entries with newlines `history --delete` (fish script, not builtin) is broken by entries with newlines Jul 15, 2016
krader1961 added a commit that referenced this issue Jul 21, 2016
Kurtis Rader
This fixes several problems with how the builtin `history` command handles
arguments. It now complains and refuses to do anything if the user specifies
incompatible actions (e.g., `--search` and `--clear`). It also fixes a
regression introduced by previous changes with regard to invocations that
don't explicitly specify `--search` or a search term.

Enhances the history man page to clarify the behavior of various options.

This change is already far larger than I like so unit tests will be added
in a separate commit.

Fixes #3224.

Note: This fixes only a couple problems with the interactive `history
--delete` command in the `history` function. The main problem will be
dealt with via issue #31.
@c22
Copy link

@c22 c22 commented Jul 27, 2016

Running into this same bug when a command is in a begin/end block spanning multiple lines. I would expect slightly different behaviour to what @ridiculousfish talks about in the original report.

for example, if my history contains

begin
echo hello
echo world
end

history --prefix echo should return no results (as it does currently)
history --prefix begin should return (as it does currently)

begin
echo hello
echo world
end

(although it would possibly be even nicer if it indented output in the same way writing "begin[return]" at the interactive prompt does)

history --prefix begin --delete should allow me to delete the entire block (it currently does not)

history --contains echo --delete should also allow me to delete the entire block (it currently does not)

I don't think it would be consistent if "history --contains echo --delete" deleted only the lines in the block that contained echo, because then in the example above you'd be left with

begin
end

in your history and it wouldn't make sense.

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Oct 12, 2016

It seems like the only sensible solution is adding another flag to request that the history search output have newlines converted to \n and backslashes escaped (i.e., \ -> \\). Similar to, if not identical to, running each history item through string escape --no-quoted before writing it to stdout. The caller can then easily and safely unescape the search output back to its original form for display and other processing. Even if --with-time or -t was also used.

It is unlikely this will be implemented in time for the 2.4.0 release but I'm going to focus on fixing this any way. Fixing this will make a nice finale to the other history improvements that have been made in the past couple of months.

@faho
Copy link
Member

@faho faho commented Oct 12, 2016

@krader1961: Wouldn't using \0 as the separator work as well? That way you could loop through history entries with while read -z. At least I believe that char can't occur in the history.

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Oct 12, 2016

Wouldn't using \0 as the separator work as well?

Yes. I was worried about potentially having an embedded NUL in the history but that's impossible today and we should prohibit it in the future if we ever switch from "char *" style strings to C++ string objects in all the history code paths.

@zanchey zanchey modified the milestones: fish 2.4.0, fish-future Oct 28, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.