change how redirections are formatted

Modify `fish_indent` to emit redirections without a space before the target of the redirection; e.g., "2>&1" rather than "2>& 1" as the former is clearer to humans. Fixes #2899
fish-shell · Apr 6, 2016 · 4ff8e6e · 4ff8e6e
1 parent 8e8b5a6
commit 4ff8e6e
Show file tree

Hide file tree

Showing 6 changed files with 136 additions and 30 deletions.
diff --git a/doc_src/fish_indent.txt b/doc_src/fish_indent.txt
@@ -11,6 +11,8 @@ fish_indent [OPTIONS]
 
 The following options are available:
 
+- `-d` or `--dump` dumps information about the parsed fish commands to stderr
+
 - `-i` or `--no-indent` do not indent commands; only reformat to one job per line
 
 - `-v` or `--version` displays the current fish version and then exits

diff --git a/src/fish_indent.cpp b/src/fish_indent.cpp
@@ -15,10 +15,7 @@ along with this program; if not, write to the Free Software
 Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
 */
 
-/** \file fish_indent.cpp
-  The fish_indent proegram.
-*/
-
+// The fish_indent proegram.
 #include "config.h"
 
 #include <getopt.h>
@@ -45,10 +42,11 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
 
 #define SPACES_PER_INDENT 4
 
-/* An indent_t represents an abstract indent depth. 2 means we are in a doubly-nested block, etc. */
+// An indent_t represents an abstract indent depth. 2 means we are in a doubly-nested block, etc.
 typedef unsigned int indent_t;
+static bool dump_parse_tree = false;
 
-/* Read the entire contents of a file into the specified string */
+// Read the entire contents of a file into the specified string.
 static wcstring read_file(FILE *f)
 {
     wcstring result;
@@ -69,10 +67,12 @@ static wcstring read_file(FILE *f)
     return result;
 }
 
-/* Append whitespace as necessary. If we have a newline, append the appropriate indent. Otherwise, append a space. */
-static void append_whitespace(indent_t node_indent, bool do_indent, bool has_new_line, wcstring *out_result)
+// Append whitespace as necessary. If we have a newline, append the appropriate indent. Otherwise,
+// append a space.
+static void append_whitespace(indent_t node_indent, bool do_indent, bool has_new_line,
+        wcstring *out_result)
 {
-    if (! has_new_line)
+    if (!has_new_line)
     {
         out_result->push_back(L' ');
     }
@@ -82,26 +82,62 @@ static void append_whitespace(indent_t node_indent, bool do_indent, bool has_new
     }
 }
 
-static void prettify_node_recursive(const wcstring &source, const parse_node_tree_t &tree, node_offset_t node_idx, indent_t node_indent, parse_token_type_t parent_type, bool *has_new_line, wcstring *out_result, bool do_indent)
+// Dump a parse tree node in a form helpful to someone debugging the behavior of this program.
+static void dump_node(indent_t node_indent, const parse_node_t &node, const wcstring &source)
+{
+    int nextc_idx = node.source_start + node.source_length;
+    wchar_t prevc = node.source_start > 0 ? source[node.source_start - 1] : L' ';
+    wchar_t nextc = nextc_idx < source.size() ? source[nextc_idx] : L' ';
+    wchar_t prevc_str[4] = {prevc, 0, 0, 0};
+    wchar_t nextc_str[4] = {nextc, 0, 0, 0};
+    if (prevc < L' ')
+    {
+        prevc_str[0] = L'\\';
+        prevc_str[1] = L'c';
+        prevc_str[2] = prevc + '@';
+    }
+    if (nextc < L' ')
+    {
+        nextc_str[0] = L'\\';
+        nextc_str[1] = L'c';
+        nextc_str[2] = nextc + '@';
+    }
+    fwprintf(stderr, L"{off %4d, len %4d, indent %2u, %ls} [%ls|%ls|%ls]\n",
+            node.source_start, node.source_length, node_indent,
+            parser_token_types[node.type].c_str(), prevc_str, source.substr(node.source_start,
+            node.source_length).c_str(), nextc_str);
+}
+
+static void prettify_node_recursive(const wcstring &source, const parse_node_tree_t &tree,
+        node_offset_t node_idx, indent_t node_indent, parse_token_type_t parent_type,
+        bool *has_new_line, wcstring *out_result, bool do_indent)
 {
     const parse_node_t &node = tree.at(node_idx);
     const parse_token_type_t node_type = node.type;
-
-    /* Increment the indent if we are either a root job_list, or root case_item_list, or in an if or while header (#1665) */
-    const bool is_root_job_list = (node_type == symbol_job_list && parent_type != symbol_job_list);
-    const bool is_root_case_item_list = (node_type == symbol_case_item_list && parent_type != symbol_case_item_list);
-    const bool is_if_while_header = ((node_type == symbol_job || node_type == symbol_andor_job_list) &&
-                                    (parent_type == symbol_if_clause || parent_type == symbol_while_header));
-    if (is_root_job_list || is_root_case_item_list || is_if_while_header)
+    const parse_token_type_t prev_node_type = node_idx > 0 ?
+        tree.at(node_idx - 1).type : token_type_invalid;
+
+    // Increment the indent if we are either a root job_list, or root case_item_list, or in an if or
+    // while header (#1665).
+    const bool is_root_job_list = node_type == symbol_job_list && parent_type != symbol_job_list;
+    const bool is_root_case_list = node_type == symbol_case_item_list &&
+        parent_type != symbol_case_item_list;
+    const bool is_if_while_header = \
+            (node_type == symbol_job || node_type == symbol_andor_job_list) &&
+            (parent_type == symbol_if_clause || parent_type == symbol_while_header);
+
+    if (is_root_job_list || is_root_case_list || is_if_while_header)
     {
         node_indent += 1;
     }
 
-    /* Handle comments, which come before the text */
-    if (node.has_comments())
+    if (dump_parse_tree) dump_node(node_indent, node, source);
+
+    if (node.has_comments())  // handle comments, which come before the text
     {
-        const parse_node_tree_t::parse_node_list_t comment_nodes = tree.comment_nodes_for_node(node);
-        for (size_t i=0; i < comment_nodes.size(); i++)
+        const parse_node_tree_t::parse_node_list_t comment_nodes = (
+                tree.comment_nodes_for_node(node));
+        for (size_t i = 0; i < comment_nodes.size(); i++)
         {
             const parse_node_t &comment_node = *comment_nodes.at(i);
             append_whitespace(node_indent, do_indent, *has_new_line, out_result);
@@ -111,26 +147,30 @@ static void prettify_node_recursive(const wcstring &source, const parse_node_tre
 
     if (node_type == parse_token_type_end)
     {
-        /* Newline */
         out_result->push_back(L'\n');
         *has_new_line = true;
     }
-    else if ((node_type >= FIRST_PARSE_TOKEN_TYPE && node_type <= LAST_PARSE_TOKEN_TYPE) || node_type == parse_special_type_parse_error)
+    else if ((node_type >= FIRST_PARSE_TOKEN_TYPE && node_type <= LAST_PARSE_TOKEN_TYPE) ||
+            node_type == parse_special_type_parse_error)
     {
         if (node.has_source())
         {
-            /* Some type representing a particular token */
-            append_whitespace(node_indent, do_indent, *has_new_line, out_result);
+            // Some type representing a particular token.
+            if (prev_node_type != parse_token_type_redirection)
+            {
+                append_whitespace(node_indent, do_indent, *has_new_line, out_result);
+            }
             out_result->append(source, node.source_start, node.source_length);
             *has_new_line = false;
         }
     }
 
-    /* Recurse to all our children */
+    // Recurse to all our children.
     for (node_offset_t idx = 0; idx < node.child_count; idx++)
     {
-        /* Note we pass our type to our child, which becomes its parent node type */
-        prettify_node_recursive(source, tree, node.child_start + idx, node_indent, node_type, has_new_line, out_result, do_indent);
+        // Note we pass our type to our child, which becomes its parent node type.
+        prettify_node_recursive(source, tree, node.child_start + idx, node_indent, node_type,
+                has_new_line, out_result, do_indent);
     }
 }
 
@@ -297,9 +337,10 @@ int main(int argc, char *argv[])
     } output_type = output_type_plain_text;
     bool do_indent = true;
 
-    const char *short_opts = "+hvi";
+    const char *short_opts = "+dhvi";
     const struct option long_opts[] =
     {
+        { "dump", no_argument, NULL, 'd' },
         { "no-indent", no_argument, NULL, 'i' },
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'v' },
@@ -319,6 +360,12 @@ int main(int argc, char *argv[])
                 exit_without_destructors(127);
             }
 
+            case 'd':
+            {
+                dump_parse_tree = true;
+                break;
+            }
+
             case 'h':
             {
                 print_help("fish_indent", 1);

diff --git a/src/parse_constants.h b/src/parse_constants.h
@@ -11,6 +11,8 @@
 #define PARSE_ASSERT(a) assert(a)
 #define PARSER_DIE() do { fprintf(stderr, "Parser dying!\n"); exit_without_destructors(-1); } while (0)
 
+// IMPORTANT: If the following enum is modified you must update the corresponding parser_token_types
+// array in parse_tree.cpp.
 enum parse_token_type_t
 {
     token_type_invalid,
@@ -41,7 +43,7 @@ enum parse_token_type_t
     symbol_plain_statement,
     symbol_arguments_or_redirections_list,
     symbol_argument_or_redirection,
-    
+
     symbol_andor_job_list,
 
     symbol_argument_list,
@@ -80,6 +82,8 @@ enum parse_token_type_t
     FIRST_PARSE_TOKEN_TYPE = parse_token_type_string,
     LAST_PARSE_TOKEN_TYPE = parse_token_type_end
 } __packed;
+// Array of strings corresponding to the enums above instantiated in parse_tree.cpp.
+extern wcstring parser_token_types[];
 
 /* These must be maintained in sorted order (except for none, which isn't a keyword). This enables us to do binary search. */
 enum parse_keyword_t

diff --git a/src/parse_tree.cpp b/src/parse_tree.cpp
@@ -16,6 +16,49 @@
 #include <vector>
 #include <algorithm>
 
+// This array provides strings for each symbol in enum parse_token_type_t in parse_constants.h.
+wcstring parser_token_types[] = {
+    L"token_type_invalid",
+    L"symbol_job_list",
+    L"symbol_job",
+    L"symbol_job_continuation",
+    L"symbol_statement",
+    L"symbol_block_statement",
+    L"symbol_block_header",
+    L"symbol_for_header",
+    L"symbol_while_header",
+    L"symbol_begin_header",
+    L"symbol_function_header",
+    L"symbol_if_statement",
+    L"symbol_if_clause",
+    L"symbol_else_clause",
+    L"symbol_else_continuation",
+    L"symbol_switch_statement",
+    L"symbol_case_item_list",
+    L"symbol_case_item",
+    L"symbol_boolean_statement",
+    L"symbol_decorated_statement",
+    L"symbol_plain_statement",
+    L"symbol_arguments_or_redirections_list",
+    L"symbol_argument_or_redirection",
+    L"symbol_andor_job_list",
+    L"symbol_argument_list",
+    L"symbol_freestanding_argument_list",
+    L"symbol_argument",
+    L"symbol_redirection",
+    L"symbol_optional_background",
+    L"symbol_end_command",
+    L"parse_token_type_string",
+    L"parse_token_type_pipe",
+    L"parse_token_type_redirection",
+    L"parse_token_type_background",
+    L"parse_token_type_end",
+    L"parse_token_type_terminate",
+    L"parse_special_type_parse_error",
+    L"parse_special_type_tokenizer_error",
+    L"parse_special_type_comment",
+    };
+
 using namespace parse_productions;
 
 static bool production_is_empty(const production_t *production)

diff --git a/tests/indent.in b/tests/indent.in
@@ -83,3 +83,9 @@ echo -n '
 if begin ; false; end; echo hi ; end
 while begin ; false; end; echo hi ; end
 ' | ../test/root/bin/fish_indent
+
+echo \nTest redir formatting
+# issue 2899
+echo -n '
+echo < stdin >>appended yes 2>&1 no > stdout maybe 2>&    4 | cat 2>| cat
+' | ../test/root/bin/fish_indent
diff --git a/tests/indent.out b/tests/indent.out
@@ -91,3 +91,7 @@ while begin
     end
     echo hi
 end
+
+Test redir formatting
+
+echo <stdin >>appended yes 2>&1 no >stdout maybe 2>&4 | cat 2>| cat