patch for TimeCommand

And here's full patch...

I think your patch gobbled up a little too much.

 JNode.ipr                                          |   15 +-
 JNode.iws                                          |  969 +++++++++++---------
 all/build.xml                                      |    2 +-

I have no idea what those two JNode files are, im guessing they are some IDE specific config files or something. Add them to .git/info/exclude if your IDE is rewriting them that heavily.

Also don't include that change to all/build.xml that alters the max memory. In order to revert files you have modified back to their original

git checkout HEAD [file]

will load the file back to its original state before editing. Again, to keep this change local to you, .git/info/exclude is your local .gitignore.

After some issues, here's link to GIT branch:

http://repo.or.cz/w/jnode-mirror.git?a=shortlog;h=refs/heads/TimeCommand

Im requesting that this be moved to a bjorne builtin. Although i haven't tested everything totally yet. Im guessing that this is not going to handle non-trivial command lines properly. Also, if this were a builtin, it should allow command completion to work properly, without some complex CommandArgument implementation.

Its still quite acceptable to implement this as a command. When bjorne is being used, the builtin will take precedence, and will be handled in a more appropriate manner.

Can you shed some light on the possiblity and complexity of doing this as a builtin steve?

Why should be this a Bjorne built-in?
The time command can be very useful in the default shell too.

Making 'time' a bjorne builtin won't help in getting completion to work for two reasons.

1. The bjorne builtins (at the moment) don't use org.jnode.shell.syntax.*. They parse their arguments the old fashioned way. I want to fix this, but ...
2. The real problem here is that 'time' would require the command argument parser (MuParser) to do a dynamic lookup of a second syntax ... based on 'time's 1st actual argument. Making 'time' a builtin does not solve this problem. If we want completion to happen, the lookup / recursion has to happen while parsing the shell input. The interpreter only discovers that 'time' is a built-in when it executes the parse tree. That's too late to be any use ... they user has already pressed ENTER.

My advice to you is to leave the 'time' completion problem alone. As I said before, it is perfectly acceptable for 'time' to do no completion. And if someone insists otherwise, challenge them to figure out how to do it cleanly!

I suggest that you implement TimeCommand with 1st argument a AliasArgument and the remaining arguments captured using a StringArgument. The TimeCommand.execute method should create a CommandLine object to hold the command name and argument, then use the current shell (see 'ShellUtils.getCurrentShell()') to 'getCommandInfo(alias)' and then 'invoke(commandLine, commandInfo, null, null)'. The last step will do normal command argument parsing, then run the command using the shell's current invoker.

(This is an unusual use-case, so I can't guarantee that the recipe will work perfectly. But it should do, and if it doesn't it's "a bug" ... IMO.)

RE: the builtin
I wasn't proposing that the time builtin complete against a syntax. The idea i had was that if 'time' was the first token found, bjorne would gobble it up and consider it more of a flag for itself, and continue to parse the rest of the tokens as if time wasn't there. These would allow completion to behave 'normally'.

RE: the command
That was the original approach taken. The other approach is to use shell.runCommand(String). But again, i dont think either method is going to be capable of non-trivial commands that may include pipelines or complex scripts. Who's to say that what time is being fed is even alias? This is an assumption that a time 'command' would have to make, but a time 'builtin' would not.

On linux you have /usr/bin/time and then there is the bash builtin time. /usr/bin/time is not capable of timing anything more than a single command. Which is going to be no different for our time command no matter which way you do it. But the builtin will allow a whole pipeline to be timed, and it should be implemented so as to not interfere with the shells completion.

Im not suggesting that the 'only' time available be the builtin style, there can be both. And in fact the 'command' time has a more expressive syntax with flags. The builtin does not, but the builtin is capable of handling complex command lines and scripts. The command can only handle aliases.

Hmmm. I've just realised that bjorne is not handling builtins the same way as 'bash does'.
For example consider this on bash:

   $  time ls
   $  export TIME=time
   $  $TIME ls

If you look carefully, you will see that the first command above uses the bash builtin, but the third line uses /usr/bin/time. In other words, bash appears to be resolving the command name for builtins before it expands variables, and resolving non-builtins after expansion. By contrast, bjorne does all command resolution (including builtins) after the expansion.

But even after I've fixed that in bjorne (assuming that it is non-POSIX), itt still don't helps. That is because bjorne completion happens in the parse phase, not the execution phase. (In fact, completion does not even do a normal parse ... it is a special parse mode in which the end-of-tokens marker triggers capture of possible completions, followed by backtracking!)

In order for this to work, I'd need to have the bjorne parser recognize the 'time' and other builtins as special. Now that I think about it, I probably need to do this anyway in order to get any completion for builtins.

So my point #2 is not entirely correct. But my point #1 still stands. Making 'time' a builtin right now won't make completion work. And even when it completion for builtins is implemented, more bjorne parser is work is needed to support the completion behaviour of 'time'. Anyway, I'm not intending to implement the bjorne changes (as described above) real soon.

So my recommendation stands. Please just make 'time' a regular command for now ... and move on to some other problem.

Commited to svn and doc page is up.

Closed manually.