Conversation
Without the separator, commands like `typeset -x -a` are misinterpreted as trying to set the attribute `-a`, which is invalid. Adding `--` clarifies that `-a` is the name of the variable/function.
There was a problem hiding this comment.
Pull Request Overview
This pull request enhances the safety of shell built-in command output by adding -- separators when printing variable or function names that start with a hyphen. This prevents names from being misinterpreted as command options when the output is re-parsed by the shell.
- Added logic to detect variable/function names starting with
-and prepend--separator in output - Updated
export,readonly, andtypesetbuilt-ins to handle this case for scalars, arrays, and functions - Added comprehensive test coverage for the new behavior across multiple built-in commands
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| variable.c | Core implementation of -- separator logic for variable and function printing |
| tests/typeset-y.tst | Test cases for typeset built-in with names starting with - |
| tests/shift-p.tst | Test case for shift built-in with -- separator |
| tests/return-p.tst | Test case for return built-in with -- separator |
| tests/readonly-y.tst | Test cases for readonly built-in with names starting with - |
| tests/readonly-p.tst | Test case for readonly built-in with -- separator |
| tests/export-y.tst | Test case for export built-in with names starting with - |
| tests/export-p.tst | Test case for export built-in with -- separator |
| tests/exit-p.tst | Test case for exit built-in with -- separator |
| tests/eval-p.tst | Test case for eval built-in with -- separator |
| NEWS.ja | Japanese documentation of the new feature |
| NEWS | English documentation of the new feature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| wchar_t *quotedvalue = | ||
| (var->v_value != NULL) ? quote_as_word(var->v_value) : NULL; | ||
| const char *separator = (name[0] == L'-') ? "-- " : ""; |
There was a problem hiding this comment.
[nitpick] The separator logic is duplicated across three functions (print_scalar, print_array, print_function). Consider extracting this into a helper function like get_name_separator(const wchar_t *name) to reduce code duplication and improve maintainability.
This pull request improves the safety and correctness of variable and function name handling in shell built-ins (
export,readonly, andtypeset). The main enhancement is that when printing variables or functions whose names start with a hyphen (-), the output now includes a--separator before the name. This change ensures that the output can be safely re-parsed by the shell, preventing names from being misinterpreted as options. The update is documented in both English and Japanese release notes, implemented in the codebase, and covered by new and updated tests.Built-in command output improvements
variable.cso that when printing variables or functions whose names begin with-, a--separator is printed before the name forexport,readonly, andtypesetbuilt-ins. This applies to both scalar and array variables as well as function names, ensuring safe re-parsing by the shell. [1] [2] [3] [4] [5] [6]Documentation updates
NEWSandNEWS.jafiles to document the new behavior for handling variable and function names starting with-in relevant built-ins. [1] [2]Test coverage additions
tests/export-p.tst,tests/export-y.tst,tests/readonly-p.tst,tests/readonly-y.tst,tests/typeset-y.tst, and others to verify the correct output of the--separator for variable and function names beginning with-. These tests cover scalar variables, array variables, and function names across the affected built-ins. [1] [2] [3] [4] [5]Related command and operand handling
eval,exit,return,shift) to ensure proper handling of the--separator before operands, further safeguarding against misinterpretation of values starting with-. [1] [2] [3] [4]