Implement alias/unalias builtin

[emlun]

Hello and happy Hacktoberfest! Here's a stab at implementing the alias/unalias builtin, along with a necessary bug fix (see 921fbeb611792d96b52b530663bfe1a74e4ac073 and 7d0511f262c2f485a790aeb6bb4970ffb75bb236) and some basic integration tests. I also extracted a debug_println! macro to make the assertions for those tests a bit cleaner.

What do you think?

[emlun]

@ashpil Have you had time to look at this?

[ashpil]

@ashpil Have you had time to look at this?

Oh wow this is amazing! I somehow completely missed this PR, thanks for pinging me again! Taking a look now.

[ashpil]

Currently panics if such a sequence of commands is run:

$ alias nothing=
$ nothing

If you do the same thing in dash, it just does nothing.

Edit: might be good to put a test case for this in along with the fix

[ashpil]

Thanks for the debug macro! Can you do convert this and this to debug_println, too?

[ashpil]

Looking at this, I think to me it might make more sense to do the parsing of the aliased command in the alias builtin itself, and have the alias structure be a map of names->Cmds. Then we can avoid parsing each time it is called. What are your thoughts on this?

[ashpil]

@emlun With the above comments, I think this PR should be good. Let me know if my comments make sense to you.

And, thanks again for implementing this! It looks great.

[emlun]

Thanks for the review! I'll try to fix these things up in the next couple of days.

[ashpil]

Thanks for the review! I'll try to fix these things up in the next couple of days.

Great! Make sure you rebase against master, as I just did a small commit, and run cargo clippy before pushing - it caught a couple things.

[ashpil]

And you can check alias/unalias in the README! 🎉

[emlun]

Alright, I think I've addressed all your comments now.

Looking at this, I think to me it might make more sense to do the parsing of the aliased command in the alias builtin itself, and have the alias structure be a map of names->Cmds. Then we can avoid parsing each time it is called. What are your thoughts on this?

You mean parse the alias immediately when defined? Hmm... maybe? My first thought on that is that it could get weird because aliases can reference each other and include pipes/conjunctions, and you can also pass additional arguments after an alias. But maybe all you'd need to do is concatenate those additional arguments onto whatever's in the predefined Cmd in the aliases map? I'm not sure.

[ashpil]

You mean parse the alias immediately when defined? Hmm... maybe? My first thought on that is that it could get weird because aliases can reference each other and include pipes/conjunctions, and you can also pass additional arguments after an alias. But maybe all you'd need to do is concatenate those additional arguments onto whatever's in the predefined Cmd in the aliases map? I'm not sure.

Yeah, I think you could append the additional arguments into the predefined Cmd's arguments, and that might even be simpler than the current version of concating the two strings and parsing them together. Just parse the args as normal, then if the command matches an alias, append it to the alias args, and execute as normal. The pipes and redirections are all included as part of the Cmd struct, so I don't think that would be an issue. The aliases referencing each other shouldn't be an issue either. Might have to move some of it into the parser, though.

To me, it seems as if it would be a cleaner and more performant way of doing it. If you're not up to it, that's fine. We can just get this merged in, and I can take a look at it myself later.

[emlun]

I took a stab at it and quickly ran into a couple of nontrivial issues. :slightly_smiling_face: For starters you'll have to pass the whole Shell as a mutable parameter to builtins::alias since the Lexer and Parser constructors need it. That in itself isn't so bad, but then comes the issue of actually constructing a Cmd inside builtins::alias, as that would need access to some stdin/out/err handles. That probably can be done too, but it feels to me like we're piling up a few too many things to be passed into the builtins::alias function. What do you think?

[ashpil]

Passing in the Shell seems reasonable, after all, that's what builtins are for - commands that can't be run by an external program as they must affect the shell directly. Unless I'm misunderstanding something, wouldn't builtins::alias not need io handles? It should just be able to use the parser.get interface to get the parsed version of the command, which it can then store in the aliases map?

[emlun]

Hm... oh yeah, you're probably right. I figured I'd need to construct a Simple instance directly at some point, which has the io handles in its fields, but yeah, you can probably get away with the instances returned by the parser. I'll take another look!

[emlun]

Okay, here are the changes I ended up with to make that happen. What do you think, should I pull that in here?

[ashpil]

Okay, here are the changes I ended up with to make that happen. What do you think, should I pull that in here?

I think that's better! A couple things:

1. I think at the moment, when we propagate the env we lose the existing env. Meaning, if we first do alias test='SOME_ENV=hello ls', when the env propagation happens, the existing SOME_ENV value is lost. So instead of replacing the env, it should just add onto it.
2. We currently do not think about redirections at all. Meaning if we use our previously defined test alias with test > file.txt, the output is not redirected.
3. Pipelines seems to freeze if they're part of an alias. alias test='ls | cat'

These things make me think that there would be way too much code for this in the runner. I think it might be smarter to just move the current propagate_env, move_args, etc functions into the impl Cmd, so one can just call cmd.extend_env(env), cmd.extend_args(args), cmd.new_io(io), etc, which to me seems it would be a lot cleaner.

Something that's subjective and doesn't necessarily need to be implemented now: I think instead of Cmds having a to_commandline function, it makes more sense to implement the std::fmt::Display trait which will make it easier to use.

[emlun]

That all sounds like good points and good ideas, thanks! I'll look into that.