diff --git a/src/builtin_set.cpp b/src/builtin_set.cpp index f70d105f6..8e2766706 100644 --- a/src/builtin_set.cpp +++ b/src/builtin_set.cpp @@ -45,12 +45,8 @@ static int is_path_variable(const wchar_t *env) { return contains(path_variables /// Call env_set. If this is a path variable, e.g. PATH, validate the elements. On error, print a /// description of the problem to stderr. -static int my_env_set(const wchar_t *key, const wcstring_list_t &val, int scope, +static int my_env_set(const wchar_t *key, const wcstring_list_t &list, int scope, io_streams_t &streams) { - size_t i; - int retcode = 0; - const wchar_t *val_str = NULL; - if (is_path_variable(key)) { // Fix for https://github.com/fish-shell/fish-shell/issues/199 . Return success if any path // setting succeeds. @@ -70,8 +66,8 @@ static int my_env_set(const wchar_t *key, const wcstring_list_t &val, int scope, if (!existing_variable.missing_or_empty()) tokenize_variable_array(existing_variable, existing_values); - for (i = 0; i < val.size(); i++) { - const wcstring &dir = val.at(i); + for (size_t i = 0; i < list.size(); i++) { + const wcstring &dir = list.at(i); if (!string_prefixes_string(L"/", dir) || contains(existing_values, dir)) { any_success = true; continue; @@ -109,44 +105,38 @@ static int my_env_set(const wchar_t *key, const wcstring_list_t &val, int scope, // Fail at setting the path if we tried to set it to something non-empty, but it wound up // empty. - if (!val.empty() && !any_success) { - return 1; + if (!list.empty() && !any_success) { + return STATUS_CMD_ERROR; } } - wcstring sb; - if (!val.empty()) { - for (i = 0; i < val.size(); i++) { - sb.append(val[i]); - if (i < val.size() - 1) { - sb.append(ARRAY_SEP_STR); - } - } - val_str = sb.c_str(); - } - - switch (env_set(key, val_str, scope | ENV_USER)) { + // We don't check `val->empty()` because an array var with a single empty string will be + // "empty". A truly empty array var is set to the special value `ENV_NULL`. + auto val = list_to_array_val(list); + int retcode = env_set(key, *val == ENV_NULL ? NULL : val->c_str(), scope | ENV_USER); + switch (retcode) { case ENV_OK: { + retcode = STATUS_CMD_OK; break; } case ENV_PERM: { streams.err.append_format(_(L"%ls: Tried to change the read-only variable '%ls'\n"), L"set", key); - retcode = 1; + retcode = STATUS_CMD_ERROR; break; } case ENV_SCOPE: { streams.err.append_format( _(L"%ls: Tried to set the special variable '%ls' with the wrong scope\n"), L"set", key); - retcode = 1; + retcode = STATUS_CMD_ERROR; break; } case ENV_INVALID: { streams.err.append_format( _(L"%ls: Tried to set the special variable '%ls' to an invalid value\n"), L"set", key); - retcode = 1; + retcode = STATUS_CMD_ERROR; break; } default: { diff --git a/src/common.cpp b/src/common.cpp index a14507cfa..d0dea1c20 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -1633,19 +1633,6 @@ int common_get_width() { return get_current_winsize().ws_col; } int common_get_height() { return get_current_winsize().ws_row; } -void tokenize_variable_array(const wcstring &val, std::vector &out) { - size_t pos = 0, end = val.size(); - while (pos <= end) { - size_t next_pos = val.find(ARRAY_SEP, pos); - if (next_pos == wcstring::npos) { - next_pos = end; - } - out.resize(out.size() + 1); - out.back().assign(val, pos, next_pos - pos); - pos = next_pos + 1; // skip the separator, or skip past the end - } -} - bool string_prefixes_string(const wchar_t *proposed_prefix, const wcstring &value) { size_t prefix_size = wcslen(proposed_prefix); return prefix_size <= value.size() && value.compare(0, prefix_size, proposed_prefix) == 0; diff --git a/src/common.h b/src/common.h index 49e8fe30c..58e449b7d 100644 --- a/src/common.h +++ b/src/common.h @@ -12,9 +12,6 @@ #include #include #include -#ifdef HAVE_SYS_IOCTL_H -#include -#endif #include #include @@ -738,12 +735,6 @@ void common_handle_winch(int signal); /// Write the given paragraph of output, redoing linebreaks to fit the current screen. wcstring reformat_for_screen(const wcstring &msg); -/// Tokenize the specified string into the specified wcstring_list_t. -/// -/// \param val the input string. The contents of this string is not changed. -/// \param out the list in which to place the elements. -void tokenize_variable_array(const wcstring &val, wcstring_list_t &out); - /// Make sure the specified direcotry exists. If needed, try to create it and any currently not /// existing parent directories. /// diff --git a/src/env.cpp b/src/env.cpp index c228cf4c9..cb9cec7e3 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -961,7 +961,7 @@ int env_set(const wcstring &key, const wchar_t *val, env_mode_flags_t var_mode) return ENV_INVALID; } - // Zero element arrays are internaly not coded as null but as this placeholder string. + // Zero element arrays are internally not coded as an empty string but this placeholder string. if (!val) { val = ENV_NULL; //!OCLINT(parameter reassignment) } @@ -1522,10 +1522,55 @@ const wchar_t *const env_vars_snapshot_t::highlighting_keys[] = {L"PATH", L"CDPA const wchar_t *const env_vars_snapshot_t::completing_keys[] = {L"PATH", L"CDPATH", NULL}; -wcstring *list_to_array_val(const wcstring_list_t &list) { - return new wcstring(); +// The next set of functions convert between a flat string and an actual list of strings using +// the encoding employed by fish internally for "arrays". + +std::unique_ptr list_to_array_val(const wcstring_list_t &list) { + auto val = std::unique_ptr(new wcstring()); + + if (list.size() == 0) { + // Zero element arrays are internally encoded as this placeholder string. + val->append(ENV_NULL); + } else { + for (auto it : list) { + if (!val->empty()) val->push_back(ARRAY_SEP); + val->append(it); + } + } + + return val; } -wcstring *list_to_array_val(const wchar_t **list) { - return new wcstring(); +std::unique_ptr list_to_array_val(const wchar_t **list) { + auto val = std::unique_ptr(new wcstring()); + + if (!*list) { + // Zero element arrays are internally encoded as this placeholder string. + val->append(ENV_NULL); + } else { + for (auto it = list; *it; it++) { + if (it != list) val->push_back(ARRAY_SEP); + val->append(*it); + } + } + + return val; +} + +void tokenize_variable_array(const wcstring &val, wcstring_list_t &out) { + out.clear(); // ensure the output var is empty -- this will normally be a no-op + + // Zero element arrays are internally encoded as this placeholder string. + if (val == ENV_NULL) return; + + size_t pos = 0, end = val.size(); + while (pos <= end) { + size_t next_pos = val.find(ARRAY_SEP, pos); + if (next_pos == wcstring::npos) { + next_pos = end; + } + out.resize(out.size() + 1); + out.back().assign(val, pos, next_pos - pos); + pos = next_pos + 1; // skip the separator, or skip past the end + } } diff --git a/src/env.h b/src/env.h index 485180c01..7160e893d 100644 --- a/src/env.h +++ b/src/env.h @@ -16,7 +16,7 @@ extern bool curses_initialized; /// Character for separating two array elements. We use 30, i.e. the ascii record separator since /// that seems logical. -#define ARRAY_SEP ((wchar_t)(0x1e)) +#define ARRAY_SEP (wchar_t)0x1e /// String containing the character for separating two array elements. #define ARRAY_SEP_STR L"\x1e" @@ -212,6 +212,12 @@ extern bool term_has_xn; // does the terminal have the "eat_newline_glitch" bool term_supports_setting_title(); /// Returns the fish internal representation for an array of strings. -wcstring *list_to_array_val(const wcstring_list_t &list); -wcstring *list_to_array_val(const wchar_t **list); +std::unique_ptr list_to_array_val(const wcstring_list_t &list); +std::unique_ptr list_to_array_val(const wchar_t **list); + +/// Tokenize the specified string into the specified wcstring_list_t. +/// +/// \param val the input string. The contents of this string is not changed. +/// \param out the list in which to place the elements. +void tokenize_variable_array(const wcstring &val, wcstring_list_t &out); #endif diff --git a/src/fish.cpp b/src/fish.cpp index 260d6144f..f78439df2 100644 --- a/src/fish.cpp +++ b/src/fish.cpp @@ -424,19 +424,12 @@ int main(int argc, char **argv) { // OK to not do this atomically since we cannot have gone multithreaded yet. set_cloexec(fd); - if (*(argv + my_optind)) { - wcstring sb; - char **ptr; - int i; - for (i = 1, ptr = argv + my_optind; *ptr; i++, ptr++) { - if (i != 1) sb.append(ARRAY_SEP_STR); - sb.append(str2wcstring(*ptr)); - } - - env_set(L"argv", sb.c_str(), 0); - } else { - env_set(L"argv", NULL, 0); + wcstring_list_t list; + for (char **ptr = argv + my_optind; *ptr; ptr++) { + list.push_back(str2wcstring(*ptr)); } + auto val = list_to_array_val(list); + env_set(L"argv", *val == ENV_NULL ? NULL : val->c_str(), 0); const wcstring rel_filename = str2wcstring(file); diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index b3e83c95c..065656280 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -940,6 +940,57 @@ static void test_parse_util_cmdsubst_extent() { } } +/// Verify the behavior of the `list_to_aray_val()` family of functions. +static void test_list_to_array() { + auto list = wcstring_list_t(); + auto val = list_to_array_val(list); + if (*val != ENV_NULL) { + err(L"test_list_to_array failed on line %lu", __LINE__); + } + + list.push_back(L"abc"); + val = list_to_array_val(list); + if (*val != list[0]) { + err(L"test_list_to_array failed on line %lu", __LINE__); + } + + list.push_back(L"def"); + val = list_to_array_val(list); + if (*val != L"abc" ARRAY_SEP_STR L"def") { + err(L"test_list_to_array failed on line %lu", __LINE__); + } + + list.push_back(L"ghi"); + val = list_to_array_val(list); + if (*val != L"abc" ARRAY_SEP_STR L"def" ARRAY_SEP_STR L"ghi") { + err(L"test_list_to_array failed on line %lu", __LINE__); + } + + const wchar_t *array1[] = {NULL}; + val = list_to_array_val(array1); + if (*val != ENV_NULL) { + err(L"test_list_to_array failed on line %lu", __LINE__); + } + + const wchar_t *array2[] = {L"abc", NULL}; + val = list_to_array_val(array2); + if (wcscmp(val->c_str(), L"abc") != 0) { + err(L"test_list_to_array failed on line %lu", __LINE__); + } + + const wchar_t *array3[] = {L"abc", L"def", NULL}; + val = list_to_array_val(array3); + if (wcscmp(val->c_str(), L"abc" ARRAY_SEP_STR L"def") != 0) { + err(L"test_list_to_array failed on line %lu", __LINE__); + } + + const wchar_t *array4[] = {L"abc", L"def", L"ghi", NULL}; + val = list_to_array_val(array4); + if (wcscmp(val->c_str(), L"abc" ARRAY_SEP_STR L"def" ARRAY_SEP_STR L"ghi") != 0) { + err(L"test_list_to_array failed on line %lu", __LINE__); + } +} + static struct wcsfilecmp_test { const wchar_t *str1; const wchar_t *str2; @@ -997,6 +1048,7 @@ static void test_wcsfilecmp() { static void test_utility_functions() { say(L"Testing utility functions"); + test_list_to_array(); test_wcsfilecmp(); test_parse_util_cmdsubst_extent(); } diff --git a/src/history.h b/src/history.h index ca8b6316a..68381f1a4 100644 --- a/src/history.h +++ b/src/history.h @@ -258,8 +258,8 @@ class history_t { // Incorporates the history of other shells into this history. void incorporate_external_changes(); - // Gets all the history into a string with ARRAY_SEP_STR. This is intended for the $history - // environment variable. This may be long! + // Gets all the history into a string. This is intended for the $history environment variable. + // This may be long! void get_string_representation(wcstring *result, const wcstring &separator); // Sets the valid file paths for the history item with the given identifier. diff --git a/src/path.cpp b/src/path.cpp index 9d6866054..fcb383474 100644 --- a/src/path.cpp +++ b/src/path.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -56,7 +57,7 @@ static bool path_get_path_core(const wcstring &cmd, wcstring *out_path, // for the fish programs. Possibly with a duplicate dir if PREFIX is empty, "/", "/usr" or // "/usr/". If the PREFIX duplicates /bin or /usr/bin that is harmless other than a trivial // amount of time testing a path we've already tested. - bin_path = L"/bin" ARRAY_SEP_STR L"/usr/bin" ARRAY_SEP_STR PREFIX L"/bin"; + bin_path = *list_to_array_val(wcstring_list_t({L"/bin", L"/usr/bin", PREFIX L"/bin"})); } std::vector pathsv;