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

WIP: Allow user to prevent commands from being saved in history #512

Merged
merged 4 commits into from Aug 26, 2017

Conversation

Projects
None yet
2 participants
@BafDyce
Contributor

BafDyce commented Aug 23, 2017

Commands that should NOT be saved in the history can be defined via the
HISTORY_IGNORE environment variable (array). Currently, the following
options are supported:

  • "all": No commands will be saved until this option is removed again
  • "whitespace": ignores commands with a leading whitespace character
  • "no_such_command": ignores commands with exit status NO_SUCH_COMMAND
  • "regex:xxx": where xxx is a regex (https://doc.rust-lang.org/regex), ignores all commands that match this regex

Examples:

# echo @HISTORY_IGNORE

# let HISTORY_IGNORE = [ all ] # saved
# let HISTORY_IGNORE = [ whitespace ] # saved
#  true # ignored
#  let HISTORY_IGNORE = [  ] # saved
#  let HISTORY_IGNORE = [ whitespace ] # ignored
# history
echo @HISTORY_IGNORE
let HISTORY_IGNORE = [ all ] # saved
let HISTORY_IGNORE = [ whitespace ] # saved
 let HISTORY_IGNORE = [  ] # saved


# let HISTORY_IGNORE = [ no_such_command ]
# true # saved
#  true # saved
# false # saved
# trulse # ignored


# let HISTORY_IGNORE = [ 'regex:.*' ] # behaves like 'all'
# true # ignored
#  true # ignored
# false # ignored
# trulse # ignored

Solution:
I implemented it in a way that is open for future extensions, unlike bashs implementation ;)

Drawbacks
We need to keep the compiled regexes in a vector, so we have a small heap-usage there. However, currently there is nothing we can do about that.

TODOs:

  • unit & integration tests

Fixes:
#503
#502 (partly, see https://github.com/redox-os/ion/issues/503#issuecomment-323079748)

State:
WIP

Other:
Also open to discussion if you think I could implement something differently, etc.

@mmstick

This comment has been minimized.

Collaborator

mmstick commented Aug 23, 2017

I'll wait for the TODO's to give you some time to implement them and perfect this feature. I'm not sure why the regex's wouldn't work though, if you ensure that they're set with single quotes (which ignores all parsing rules). IE:

echo one two three | awk '{print $2}'
@mmstick

This comment has been minimized.

Collaborator

mmstick commented Aug 23, 2017

Also, maybe we should just make the HISTORY_IGNORE array a sort of facade, whereby a match to that key doesn't search our hashmap of arrays, but points directly to a structure which we can update?

@BafDyce

This comment has been minimized.

Contributor

BafDyce commented Aug 24, 2017

Uh, that's my mistake then. I always used double quotes. That probably explains the issues I had.

Is there already a variable that uses such a behavior in the code base? If not do you have a suggestion on how I could implement this (just a snippet of (pseudo) code would be enough)

Either way, I'll try to resolve the Todos asap but I cant promise much in terms of when it will be finished.

@mmstick

This comment has been minimized.

Collaborator

mmstick commented Aug 24, 2017

Check out get_var on master. The new SWD and MWD variables are doing that.

@BafDyce BafDyce changed the title from Allow user to prevent commands from being saved in history to WIP: Allow user to prevent commands from being saved in history Aug 25, 2017

@BafDyce

This comment has been minimized.

Contributor

BafDyce commented Aug 25, 2017

I could not use the same logic as it is used for the SWD and MWD variables. (These live just in the context of the Variables struct and we need to access the history).

I therefore went another way:
Add a new member to the Shell struct which contains the patterns (+ compiled regexes) that specify which commands should be ignored from the history. Whenever an array variable is asigned some value we check whether it was HISTORY_IGNORE that was changed. If yes, then we update the shells ignore patterns. Compilation of all regexes also happens only at this time.

Also, maybe we should just make the HISTORY_IGNORE array a sort of facade, whereby a match to that key doesn't search our hashmap of arrays, but points directly to a structure which we can update?

I didn't do that (HISTORY_IGNORE is still a normal array variable) so that one can still use it like an array in shell scripts, etc.

I'm not sure why the regex's wouldn't work though, if you ensure that they're set with single quotes (which ignores all parsing rules)

Some testing confirmed this! When using single quotes everything works as expected.

I'll also update the original post to reflect the new changes

@BafDyce

This comment has been minimized.

Contributor

BafDyce commented Aug 26, 2017

Rebased the branch (+ git push -f !) to resolve the merge conflicts caused by fab14ab. I also verified that the feature still works as expected.

@mmstick

This comment has been minimized.

Collaborator

mmstick commented Aug 26, 2017

Sorry about the big change. There shouldn't have been any conflicts between our work. Changes were largely within the assignment parsing, and changes outside of that were strictly to fix compiler errors due to the change in the API for assignment parsing. Assignment parsing should now be much more advanced, and I'll have all the type checking functionality completed later today. Maybe even implement array arithmetic while I'm at it. Something like this:

let a:int[] = [1 2 3 4]
let a += 1
echo @a
> 2 3 4 5
@BafDyce

This comment has been minimized.

Contributor

BafDyce commented Aug 26, 2017

No problem, it was just a small conflict which was resolved in about 5-10 minutes. Part of my PR adds a small check to the assignment logic, to detect when HISTORY_IGNORE is changed and to update the ignore patterns/regular expressions.

I welcome the changes to the assignment parsing API. I was positively surprised to see types coming into a scripting language. Opens up a few questions though:

  • Is it mandatory for each assigment to specify a type or is there some sort of default type (I haven't fully looked through the code yet)
  • Will casting be supported? For example, will this be possible in the future?
# let a:str = "123"
# let b:int = a
# let a += 4
# let b += 4
# echo $a $b
1234 127
@mmstick

This comment has been minimized.

Collaborator

mmstick commented Aug 26, 2017

If no type parameter is defined, then it's type is determined by the value, same as before. Technically, the type is referred to internally as the Any type.

let a = [one two three]
let a = "one"

It only checks to see if the type is an array, as that's all that really matters in deciding where we're going to assign this variable. Arrays of course are defined by whether or not they're wrapped within brackets. No brackets = string.

As for what you mentioned regarding casting, the type checking only happens during assignment, and that information is not carried on after assignment. So you can technically create an a:str and then later set a new variable with b:int using a's value.

If you attempt to use an assignment operator on a value whose parameter isn't defined, it will try to determine if that value can be parsed as a u64, followed by f64, and then otherwise error.

@BafDyce

This comment has been minimized.

Contributor

BafDyce commented Aug 26, 2017

Sounds neat :-)

it will try to determine if that value can be parsed as a u64, followed by f64, and then otherwise error.

Do I read this correctly that signed integers (i64) are not (yet) implemented/supported? Or are signed values covered by f64?

@mmstick

This comment has been minimized.

Collaborator

mmstick commented Aug 26, 2017

Forgot to think about it, but I'll do that. i64 -> f64.

@mmstick

This comment has been minimized.

Collaborator

mmstick commented Aug 26, 2017

Okay -- I have type checking completed, for both arrays and strings. And switched to i64 instead of u64. Boolean values will also be normalized to a standard value.

let a:bool = [1 0 1 1 0 0]
echo @a
> true false true true false false

BafDyce added some commits Aug 23, 2017

Allow user to prevent commands from being saved in history
Commands that should NOT be saved in the history, can be defined via the
@HISTORY_IGNORE environment variable (array). Currently, the following
options are supported:
- "all": No commands will be saved until this option is removed again
- "whitespace": ignores commands with a leading whitespace character
- "no_such_command": ignores commands with exit status NO_SUCH_COMMAND
- "regex:xxx": where xxx is a regex (https://doc.rust-lang.org/regex)
ignores all commands that match this regex
Parse HISTORY_IGNORE regexes only when changing the variable
Not on every executed command. This saves quite some precious CPU cycles
@BafDyce

This comment has been minimized.

Contributor

BafDyce commented Aug 26, 2017

Rebased again to include latest hotfix.

@mmstick

This comment has been minimized.

Collaborator

mmstick commented Aug 26, 2017

Do you want me to merge this now?

if let Err(err) = self.context.as_mut().unwrap().history.push(command.into()) {
let stderr = io::stderr();
let mut stderr = stderr.lock();
let _ = writeln!(stderr, "ion: {}", err);

This comment has been minimized.

@mmstick

mmstick Aug 26, 2017

Collaborator

You could use eprintln!() instead to make this simpler. Does the same thing.

// The length check is there to just ignore empty regex definitions
_ if pattern.starts_with(regex_prefix) && pattern.len() > regex_prefix.len() => {
flags |= IGNORE_BASED_ON_REGEX;
let regex_string = pattern[regex_prefix.len()..].to_owned();

This comment has been minimized.

@mmstick

mmstick Aug 26, 2017

Collaborator

Maybe only use to_owned() when pushing to the regexes vector?

This comment has been minimized.

@BafDyce

BafDyce Aug 26, 2017

Contributor

Actually, we don't need to_owned() at all here. It's a surviver of some earlier version and I forgot to remove it when refactoring.

if c.is_whitespace() {
return false;
}
}

This comment has been minimized.

@mmstick

mmstick Aug 26, 2017

Collaborator

Can rewrite this block as:

if command.bytes().next().map_or(false, |b| (b as char).is_whitespace()) {
    return false;
}

This comment has been minimized.

@BafDyce

BafDyce Aug 26, 2017

Contributor

Shouldn't we use command.chars() here?

We wouldn't need the cast and nothing breaks if the first character is some multi-byte unicode char.

This comment has been minimized.

@mmstick

mmstick Aug 26, 2017

Collaborator

Has higher performance to use bytes. Whitespace characters are never larger than a byte, so it's a bit more efficient.

if regex.is_match(command) && !command.contains("HISTORY_IGNORE") {
return false;
}
}

This comment has been minimized.

@mmstick

mmstick Aug 26, 2017

Collaborator
if regexes.any(|regex| regex.is_match(command) && !command.contains("HISTORY_IGNORE")) {
    return false;
}

This comment has been minimized.

@BafDyce

BafDyce Aug 26, 2017

Contributor

This looks so much cleaner! I really should get more familiar with closures and all the fancy functions that are available in the standard lib ;)

@BafDyce

This comment has been minimized.

Contributor

BafDyce commented Aug 26, 2017

Thanks for your feedback! I'll include the suggested changes.

Also, I've the documentation almost finished (added an entire page to the manual, just about the command history). So, I'd like to at least include the documentation into the PR.

@BafDyce

This comment has been minimized.

Contributor

BafDyce commented Aug 26, 2017

If you want, you can merge it now.

Unless you also want to have some unit & integration tests included. Decision is upon you ;)

@mmstick

This comment has been minimized.

Collaborator

mmstick commented Aug 26, 2017

You can add the unit & integration tests later. I'll go ahead and merge this.

@mmstick mmstick merged commit c264416 into redox-os:master Aug 26, 2017

@BafDyce

This comment has been minimized.

Contributor

BafDyce commented Aug 26, 2017

Maybe, we should have removed the "WIP:" from the title prior to the merge :D

@mmstick

This comment has been minimized.

Collaborator

mmstick commented Aug 26, 2017

As a heads up, I'm writing the documentation for method expansions at the moment.

@mmstick

This comment has been minimized.

Collaborator

mmstick commented Aug 26, 2017

Documentation for methods is complete. Now documenting the new assignment logic.

@mmstick

This comment has been minimized.

Collaborator

mmstick commented Aug 27, 2017

And that's now complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment