Revert "SHL-133 Shell command "! ls /tmp" fails"

This reverts commit 497f60533e.

src/main/java/org/springframework/shell/commands/OsCommands.java

@@ -21,7 +21,6 @@ import java.util.logging.Logger;
 import org.springframework.shell.core.CommandMarker;
 import org.springframework.shell.core.annotation.CliCommand;
 import org.springframework.shell.core.annotation.CliOption;
-import org.springframework.shell.core.annotation.PassThroughOptions;
 import org.springframework.shell.support.logging.HandlerUtils;
 import org.springframework.stereotype.Component;
 
@@ -40,7 +39,6 @@ public class OsCommands implements CommandMarker {
     private OsOperations osOperations = new OsOperationsImpl();
 
     @CliCommand(value = "!", help = "Allows execution of operating system (OS) commands")
-    @PassThroughOptions
     public void command(
             @CliOption(key = { "", "command" }, mandatory = false, specifiedDefaultValue = "", unspecifiedDefaultValue = "", help = "The command to execute") final String command) {
 

src/main/java/org/springframework/shell/commands/VersionCommands.java

@@ -1,4 +1,4 @@
-	 	package org.springframework.shell.commands;
+package org.springframework.shell.commands;
 
 import org.springframework.beans.BeansException;
 import org.springframework.context.ApplicationContext;

src/main/java/org/springframework/shell/core/SimpleParser.java

@@ -27,7 +27,6 @@ import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Map.Entry;
 import java.util.Set;
 import java.util.SortedSet;
 import java.util.TreeSet;
@@ -36,7 +35,6 @@ import java.util.logging.Logger;
 import org.springframework.shell.core.annotation.CliAvailabilityIndicator;
 import org.springframework.shell.core.annotation.CliCommand;
 import org.springframework.shell.core.annotation.CliOption;
-import org.springframework.shell.core.annotation.PassThroughOptions;
 import org.springframework.shell.event.ParseResult;
 import org.springframework.shell.support.logging.HandlerUtils;
 import org.springframework.shell.support.util.ExceptionUtils;
@@ -159,15 +157,10 @@ public class SimpleParser implements Parser {
 
 			// Attempt to parse
 			Map<String, String> options = null;
-			
-			// Hint to the parser that options will not be of the form --key=value, but should attempt to parse and
-			// pass through the command option 'as is'
-			PassThroughOptions passThroughOptions = methodTarget.getMethod()
-					.getAnnotation(PassThroughOptions.class);
 			try {
-				options = new Tokenizer(methodTarget.getRemainingBuffer(),
+				options = new Tokenizer(methodTarget.getRemainingBuffer()).getTokens();
-						false, passThroughOptions != null).getTokens();
+			}
-			} catch (IllegalArgumentException e) {
+			catch (IllegalArgumentException e) {
 				LOGGER.warning(ExceptionUtils.extractRootCause(e).getMessage());
 				return null;
 			}

src/main/java/org/springframework/shell/core/Tokenizer.java

@@ -28,16 +28,14 @@ import org.springframework.util.Assert;
  * Properly treats double quotes (") as option delimiters.
  * 
  * <p>
- * Expects option names to be preceded by a double dash. We call this an
+ * Expects option names to be preceded by a double dash. We call this an "option marker".
- * "option marker".
  * 
  * <p>
  * Treats spaces as the default option tokenizer.
  * 
  * <p>
- * Any token without an option marker is considered the default. The default is
+ * Any token without an option marker is considered the default. The default is returned in the Map as an element with
- * returned in the Map as an element with an empty string key (""). There can
+ * an empty string key (""). There can only be a single default.
- * only be a single default.
  * 
  * @author Eric Bottard
  * @since 1.1
@@ -56,14 +54,7 @@ public class Tokenizer {
 	private boolean allowUnbalancedLastQuotedValue;
 
 	/**
-	 * When processing 'raw' command options, allow them to be aggregated
+	 * Used to indicate that the last value was indeed half enclosed in quotes. Useful so that parser can re-add it.
-	 * together
-	 */
-	private boolean allowMultipleEmptyOptions;
-
-	/**
-	 * Used to indicate that the last value was indeed half enclosed in quotes.
-	 * Useful so that parser can re-add it.
 	 */
 	private boolean openingQuotesHaveNotBeenClosed;
 
@@ -76,14 +67,8 @@ public class Tokenizer {
 	}
 
 	public Tokenizer(String text, boolean allowUnbalancedLastQuotedValue) {
-		this(text, allowUnbalancedLastQuotedValue, false);
-	}
-
-	public Tokenizer(String text, boolean allowUnbalancedLastQuotedValue,
-			boolean allowMultipleEmptyOptions) {
 		this.buffer = text.toCharArray();
 		this.allowUnbalancedLastQuotedValue = allowUnbalancedLastQuotedValue;
-		this.allowMultipleEmptyOptions = allowMultipleEmptyOptions;
 		tokenize();
 	}
 
@@ -107,8 +92,7 @@ public class Tokenizer {
 	}
 
 	/**
-	 * Return true if the remaining buffer matches the given String (return
+	 * Return true if the remaining buffer matches the given String (return false if there is not enough input).
-	 * false if there is not enough input).
 	 */
 	private boolean lookAhead(char... toMatch) {
 		for (int i = 0; i < toMatch.length; i++) {
@@ -121,14 +105,14 @@ public class Tokenizer {
 	}
 
 	/**
-	 * Consume either {@code --key[=value]} or just {@code value}, eating extra
+	 * Consume either {@code --key[=value]} or just {@code value}, eating extra spaces.
-	 * spaces.
 	 */
 	private void eatKeyValuePair() {
 		if (lookAhead('-', '-')) {
 			pos += 2;
 			eatKeyEqualsValue();
-		} else {
+		}
+		else {
 			String value = eatValue();
 			store("", value);
 		}
@@ -136,18 +120,9 @@ public class Tokenizer {
 	}
 
 	private void store(String key, String value) {
-		if (this.allowMultipleEmptyOptions) {
+		if (result.put(key, value) != null) {
-			//collect up all the options and reconstruct the full 'string' by appending
+			throw new IllegalArgumentException("You cannot specify option '" + key
-			//each parsed option together with a space.
+					+ "' more than once in a single command");
-			if (result.containsKey(key)) {
-				String originalValue = result.get(key);
-				result.put(key, originalValue + " " + value);
-			} else {
-				result.put(key, value);
-			}
-		} else if (result.put(key, value) != null) {
-			throw new IllegalArgumentException("You cannot specify option '"
-					+ key + "' more than once in a single command");
 		}
 	}
 
@@ -161,8 +136,7 @@ public class Tokenizer {
 			endDelimiter = '"';
 			pos++;
 		}
-		// So that it can be retrieved later (if this is actually the last
+		// So that it can be retrieved later (if this is actually the last value)
-		// value)
 		lastValueDelimiter = endDelimiter;
 		lastValueStartOffset = pos;
 		while (pos < buffer.length && buffer[pos] != endDelimiter) {
@@ -174,20 +148,18 @@ public class Tokenizer {
 			sb.append(buffer[pos]);
 			pos++;
 		}
-		// When here, we either ran out of input, or encountered our delim, or
+		// When here, we either ran out of input, or encountered our delim, or both
-		// both
 		// Fail, unless we allow an unfinished quoted string to be reported
 		if (endDelimiter == '"' && // we're using quotes
 				pos == buffer.length && // we ran of input
 				(buffer[pos - 1] != '"' || // quotes are not properly closed
-				sb.length() == 0)) { // BUT it's ok if consumed nothing (pos-1
+				sb.length() == 0)) { // BUT it's ok if consumed nothing (pos-1 is *opening* quote then)
-										// is *opening* quote then)
 			if (allowUnbalancedLastQuotedValue) {
 				openingQuotesHaveNotBeenClosed = true;
 				return sb.toString();
-			} else {
+			}
-				throw new IllegalArgumentException(
+			else {
-						"Cannot have an unbalanced number of quotation marks");
+				throw new IllegalArgumentException("Cannot have an unbalanced number of quotation marks");
 			}
 		}
 		// Eat our delim
@@ -196,36 +168,30 @@ public class Tokenizer {
 	}
 
 	/**
-	 * Return the offset at which the last value seen started (NOT including any
+	 * Return the offset at which the last value seen started (NOT including any delimiter).
-	 * delimiter).
 	 */
 	public int getLastValueStartOffset() {
-		Assert.isTrue(lastValueStartOffset >= 0,
+		Assert.isTrue(lastValueStartOffset >= 0, "lastValueStartOffset has not been set yet");
-				"lastValueStartOffset has not been set yet");
 		return lastValueStartOffset;
 	}
 
 	/**
-	 * Return the delimiter (space or quotes) that was (or is being) used for
+	 * Return the delimiter (space or quotes) that was (or is being) used for the last value.
-	 * the last value.
 	 */
 	public char getLastValueDelimiter() {
-		Assert.isTrue(lastValueDelimiter != 0,
+		Assert.isTrue(lastValueDelimiter != 0, "lastValueDelimiter has not been set yet");
-				"lastValueDelimiter has not been set yet");
 		return lastValueDelimiter;
 	}
 
 	/**
-	 * Return whether the last value was meant to be enclosed in quotes, but the
+	 * Return whether the last value was meant to be enclosed in quotes, but the closing quote has not been typed yet.
-	 * closing quote has not been typed yet.
 	 */
 	public boolean openingQuotesHaveNotBeenClosed() {
 		return openingQuotesHaveNotBeenClosed;
 	}
 
 	/**
-	 * Return true if we know for sure that the last value has been typed in
+	 * Return true if we know for sure that the last value has been typed in full.
-	 * full.
 	 */
 	public boolean lastValueIsComplete() {
 		// If using quotes as delim and they're not closed, we know for sure.
@@ -234,12 +200,10 @@ public class Tokenizer {
 	}
 
 	/**
-	 * Apply delimiter escaping to the given string, using the actual delimiter
+	 * Apply delimiter escaping to the given string, using the actual delimiter that was used for the last value.
-	 * that was used for the last value.
 	 */
 	public String escape(String value) {
-		return value.replace("" + lastValueDelimiter, "" + ESCAPE_CHAR
+		return value.replace("" + lastValueDelimiter, "" + ESCAPE_CHAR + lastValueDelimiter);
-				+ lastValueDelimiter);
 	}
 
 	/**
@@ -260,11 +224,11 @@ public class Tokenizer {
 			lastValueDelimiter = ' ';
 			lastValueStartOffset = pos;
 			value = "";
-		} else {
+		}
+		else {
 			value = eatValue();
 		}
-		// Don't store the ""="" that would result from having a pending " --"
+		// Don't store the ""="" that would result from having a pending " --" at the end
-		// at the end
 		if (key.equals("") && value.equals("")) {
 			return;
 		}

src/main/java/org/springframework/shell/core/annotation/PassThroughOptions.java

@@ -1,45 +0,0 @@
-/*
- * Copyright 2011-2012 the original author or authors.
- * 
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- * 
- *      http://www.apache.org/licenses/LICENSE-2.0
- * 
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.springframework.shell.core.annotation;
-
-import java.lang.annotation.Documented;
-import java.lang.annotation.ElementType;
-import java.lang.annotation.Inherited;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
-
-
-/**
- * A marker annotation on a method that indicates if the options for this command 
- * should not be parsed by the shell.
-
- *
- * <p>
- * An example of when to use this is when passing arguments through to the the underying
- * Operating System Shell, e.g using the command " ! ls /tmp" *
- * <p>
- *
- * @author Mark Pollack
- * @since 1.1
- */
-@Inherited
-@Documented
-@Retention(RetentionPolicy.RUNTIME)
-@Target(ElementType.METHOD)
-public @interface PassThroughOptions {
-
-}

src/test/java/org/springframework/shell/commands/BuiltInCommandTests.java

@@ -15,8 +15,6 @@
  */
 package org.springframework.shell.commands;
 
-import static org.junit.Assert.assertTrue;
-
 import java.text.DateFormat;
 import java.text.ParseException;
 import java.util.Date;
@@ -45,18 +43,4 @@ public class BuiltInCommandTests extends AbstractShellIntegrationTest {
 		Date now = new Date();
 		MatcherAssert.assertThat(now, DateMatchers.within(5, TimeUnit.SECONDS, result));		
 	}
-	
-	@Test
-	public void OsCommandTest() throws ParseException {
-		
-		String osName = System.getProperty("os.name").toLowerCase(Locale.US);
-		String pathSep = System.getProperty("path.separator");
-		boolean isUnix = pathSep.equals(":") && osName.endsWith("x");
-		if (isUnix) {
-			CommandResult cr = getShell().executeCommand("! ls /tmp");
-			assertTrue(cr.isSuccess());
-		}
-		
-	}
-	
 }