On 01/01/2019 16:24, Harald van Dijk wrote:
On 01/01/2019 14:27, Martijn Dekker wrote:
On dash and on gwsh, the 'fc -s' command, that re-executes a command
from the history, executes the command repeatedly in an infinite loop.
It should execute it just once.
This is caused by the block with the XXX comment in src/histedit.c:
if (displayhist && hist) {
/*
* XXX what about recursive and
* relative histnums.
*/
history(hist, &he, H_ENTER, s);
}
This changes the position in the history list and also clobbers he, so
the if (he.num == last) check afterwards always returns false, and if
the check was supposed to return false, the next history(hist, &he,
direction) would not do what was intended.
The immediate problem can be fixed either by dropping support for the
non-standard and undocumented fc -s first last and immediately breaking
out of the loop, or by restoring the position (restoring he at the same
time).
Keeping fc -s first last working is pretty much not an option without
modifying it to generate a full list of commands to execute first.
Otherwise, the comment
/*
* At end? (if we were to lose last, we'd sure be
* messed up).
*/
is sure to hit: executing the new commands may clear out previous
history entries, including last. fc -s first does not have that problem
since it only takes a single command -- except that it should enter the
command into the history before executing it, not after it, at which
point you can get that problem after all. However...
But there are more problems: H_ENTER adds a new command to the history.
bash and ksh do not do this:
echo hello
fc -s 1
fc -l
will show (bash, but ksh is similar)
hello
1 echo hello
2 echo hello
That is, the echo hello command appears twice in the history, but the fc
-s 1 command does not appear at all. This is what POSIX probably[*]
requires as well.
...modifying fc to use H_REPLACE nicely avoids that problem. Since no
new history entry is created, no old entry can be cleared out.
By the time fc is called, the fc line is already in the history, so to
get the bash/ksh behaviour, the current command would need to be
overwritten. The documentation of libedit does not show any option which
can overwrite entries already created. It documents H_ADD and H_APPEND,
which can append, and has an undocumented H_REPLACE used by the equally
undocumented replace_history_entry() function in <editline/readline.h>.
I do not know yet if it makes sense to start using this.
Having thought about it, I think it does make sense, and have looked at
how to use H_REPLACE. It takes a char * and a void *, where the char *
specifies the new string for the currently selected history entry, and
the void * is application-specific data. Since no application-specific
data is ever used, it can simply be hardcoded to (void *) NULL, making
sure to use a cast because history() is a variadic function.
Since H_REPLACE operates on the currently selected history entry, it
requires H_FIRST to re-select the most-recent history entry.
A corner case is when one command line consists of multiple fc commands:
echo abc
echo def
fc -s 1; fc -s 2
Here, bash and ksh let the last fc -s command "win": after this, the
history is
echo abc
echo def
echo def
but the output is
abc
def
abc
def
Using H_REPLACE replicates that.
Also, when neither -s nor -l is specified and an editor is invoked, the
new command is supposed to be entered into the history as well. That too
is not implemented.
This can be implemented by modifying the parser to allow creating
history entries for files pushed back later. Currently, the check is
limited to stdin:
if (parsefile->fd == 0 && hist && something)
but keeping track of whether the history should be saved for each file,
and then turning it on when fc without -s/-l is used, lets this work.
Other bugs:
- fc without arguments is perfectly valid and should not raise an error.
- The extension to allow negative numeric arguments without -- should
work on BSD but does not work on glibc, because optind is set to 0
rather than 1.
- Commands containing blank lines are entered into the history without
the blank lines. Entirely blank commands should not be entered in the
history, but
echo "hello
world"
should not enter the history as
echo "hello
world"
since that has a different effect.
- fc -s's old=new option should not be accepted and ignored when -s is
not specified. old=new is supposed to be interpreted as a search string
in that case.
- When an out-of-range number is specified for last, it should not be
resolved using simply H_FIRST, as that is the command containing the
current fc invocation.[*]
What I have implemented does not apply to dash cleanly due to other
changes I have made, but could be easily adapted if desired. The fc
command is only useful in interactive shells, and dash is not used much
as an interactive shell, so I do not know how much interest there is in
letting this work correctly.
Cheers,
Harald van Dijk
[*] At least, unless it is desired to make that invocation accessible,
but in that case it should also be printed by default with fc -l. Some
other shells do that, but bash doesn't, the way it's implemented in ksh
appears to be broken, and I don't personally think it makes sense.
