Implement builtin [ via builtin test

builtin.cpp

@@ -3992,6 +3992,7 @@ static int builtin_history(parser_t &parser, wchar_t **argv)
 static const builtin_data_t builtin_datas[]=
 {
     { 		L".",  &builtin_source, N_(L"Evaluate contents of file")   },
+    { 		L"[",  &builtin_test, N_(L"Test a condition")   },
     { 		L"and",  &builtin_generic, N_(L"Execute command if previous command suceeded")  },
     { 		L"begin",  &builtin_begin, N_(L"Create a block of code")   },
     { 		L"bg",  &builtin_bg, N_(L"Send job to background")   },

builtin_test.cpp

@@ -345,6 +345,7 @@ expression *test_parser::parse_combining_expression(unsigned int start, unsigned
             if (combiner != test_combine_and && combiner != test_combine_or)
             {
                 /* Not a combiner, we're done */
+                this->errors.insert(this->errors.begin(), format_string(L"Expected a combining operator like '-a' at index %u", idx));
                 break;
             }
             combiners.push_back(combiner);
@@ -561,16 +562,20 @@ expression *test_parser::parse_args(const wcstring_list_t &args, wcstring &err)
         break;
     }
 
-    if (! errored && result)
+    if (result)
     {
         /* It's also an error if there are any unused arguments. This is not detected by parse_expression() */
         assert(result->range.end <= args.size());
         if (result->range.end < args.size())
         {
-            append_format(err, L"test: unexpected argument at index %lu: '%ls'\n", (unsigned long)result->range.end, args.at(result->range.end).c_str());
+            if (err.empty())
+            {
+                append_format(err, L"test: unexpected argument at index %lu: '%ls'\n", (unsigned long)result->range.end, args.at(result->range.end).c_str());
+            }
+            errored = true;
+            
             delete result;
             result = NULL;
-            errored = true;
         }
     }
 
@@ -790,10 +795,31 @@ int builtin_test(parser_t &parser, wchar_t **argv)
     /* The first argument should be the name of the command ('test') */
     if (! argv[0])
         return BUILTIN_TEST_FAIL;
+    
+    /* Whether we are invoked with bracket '[' or not */
+    const bool is_bracket = ! wcscmp(argv[0], L"[");
 
     size_t argc = 0;
     while (argv[argc + 1])
         argc++;
+    
+    /* If we're bracket, the last argument ought to be ]; we ignore it. Note that argc is the number of arguments after the command name; thus argv[argc] is the last argument. */
+    if (is_bracket)
+    {
+        if (! wcscmp(argv[argc], L"]"))
+        {
+            /* Ignore the closing bracketp */
+            argc--;
+        }
+        else
+        {
+            builtin_show_error(L"[: the last argument must be ']'\n");
+            return BUILTIN_TEST_FAIL;
+        }
+
+    }
+    
+    /* Collect the arguments into a list */
     const wcstring_list_t args(argv + 1, argv + 1 + argc);
 
     if (argc == 0)

fish_tests.cpp

@@ -809,16 +809,21 @@ static void test_is_potential_path()
 
 /** Test the 'test' builtin */
 int builtin_test(parser_t &parser, wchar_t **argv);
-static bool run_test_test(int expected, wcstring_list_t &lst)
+static bool run_one_test_test(int expected, wcstring_list_t &lst, bool bracket)
 {
     parser_t parser(PARSER_TYPE_GENERAL, true);
     size_t i, count = lst.size();
-    wchar_t **argv = new wchar_t *[count+2];
-    argv[0] = (wchar_t *)L"test";
+    wchar_t **argv = new wchar_t *[count+3];
+    argv[0] = (wchar_t *)(bracket ? L"[" : L"test");
     for (i=0; i < count; i++)
     {
         argv[i+1] = (wchar_t *)lst.at(i).c_str();
     }
+    if (bracket)
+    {
+        argv[i+1] = (wchar_t *)L"]";
+        i++;
+    }
     argv[i+1] = NULL;
     int result = builtin_test(parser, argv);
     delete[] argv;
@@ -834,12 +839,33 @@ static bool run_test_test(int expected, const wcstring &str)
     copy(istream_iterator<wcstring, wchar_t, std::char_traits<wchar_t> >(iss),
          istream_iterator<wstring, wchar_t, std::char_traits<wchar_t> >(),
          back_inserter<vector<wcstring> >(lst));
-    return run_test_test(expected, lst);
+    
+    bool bracket = run_one_test_test(expected, lst, true);
+    bool nonbracket = run_one_test_test(expected, lst, false);
+    assert(bracket == nonbracket);
+    return nonbracket;
+}
+
+static void test_test_brackets()
+{
+    // Ensure [ knows it needs a ]
+    parser_t parser(PARSER_TYPE_GENERAL, true);
+    
+    const wchar_t *argv1[] = {L"[", L"foo", NULL};
+    assert(builtin_test(parser, (wchar_t **)argv1) != 0);
+    
+    const wchar_t *argv2[] = {L"[", L"foo", L"]", NULL};
+    assert(builtin_test(parser, (wchar_t **)argv2) == 0);
+    
+    const wchar_t *argv3[] = {L"[", L"foo", L"]", L"bar", NULL};
+    assert(builtin_test(parser, (wchar_t **)argv3) != 0);
+
 }
 
 static void test_test()
 {
     say(L"Testing test builtin");
+    test_test_brackets();
 
     assert(run_test_test(0, L"5 -ne 6"));
     assert(run_test_test(0, L"5 -eq 5"));
@@ -892,6 +918,10 @@ static void test_test()
     /* We didn't properly handle multiple "just strings" either */
     assert(run_test_test(0, L"foo"));
     assert(run_test_test(0, L"foo -a bar"));
+    
+    /* These should be errors */
+    assert(run_test_test(1, L"foo bar"));
+    assert(run_test_test(1, L"foo bar baz"));
 }
 
 /** Testing colors */