From a23894ca3769e263f22f853cee239f224456c456 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 20 Oct 2018 13:16:14 -0700 Subject: [PATCH 01/11] Simplify callback_data_t SET_EXPORT no longer makes sense; remove it. --- src/env.cpp | 29 ++++++++--------------------- src/env_universal_common.cpp | 17 ++++++++++------- src/env_universal_common.h | 16 +++++++++------- src/fish_tests.cpp | 9 +++------ 4 files changed, 30 insertions(+), 41 deletions(-) diff --git a/src/env.cpp b/src/env.cpp index 09c4f8305..19b799b3c 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -608,28 +608,16 @@ static void react_to_variable_change(const wchar_t *op, const wcstring &key) { /// Universal variable callback function. This function makes sure the proper events are triggered /// when an event occurs. -static void universal_callback(fish_message_type_t type, const wchar_t *name) { - const wchar_t *op; +static void universal_callback(const callback_data_t &cb) { + const wchar_t *op = cb.is_erase() ? L"ERASE" : L"SET"; - switch (type) { - case SET: - case SET_EXPORT: { - op = L"SET"; - break; - } - case ERASE: { - op = L"ERASE"; - break; - } - } - - react_to_variable_change(op, name); + react_to_variable_change(op, cb.key); vars_stack().mark_changed_exported(); - event_t ev = event_t::variable_event(name); + event_t ev = event_t::variable_event(cb.key); ev.arguments.push_back(L"VARIABLE"); ev.arguments.push_back(op); - ev.arguments.push_back(name); + ev.arguments.push_back(cb.key); event_fire(&ev); } @@ -720,10 +708,9 @@ void misc_init() { } } -static void env_universal_callbacks(callback_data_list_t &callbacks) { - for (size_t i = 0; i < callbacks.size(); i++) { - const callback_data_t &data = callbacks.at(i); - universal_callback(data.type, data.key.c_str()); +static void env_universal_callbacks(const callback_data_list_t &callbacks) { + for (const callback_data_t &cb : callbacks) { + universal_callback(cb); } } diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 2424f49dd..bfc5afcf9 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -74,6 +74,9 @@ /// Small note about not editing ~/.fishd manually. Inserted at the top of all .fishd files. #define SAVE_MSG "# This file contains fish universal variable definitions.\n" +/// The different types of messages found in the fishd file. +enum class uvar_message_type_t { set, set_export }; + static wcstring get_machine_identifier(); /// return a list of paths where the uvars file has been historically stored. @@ -148,7 +151,7 @@ static bool append_utf8(const wcstring &input, std::string *receiver, std::strin /// Creates a file entry like "SET fish_color_cwd:FF0". Appends the result to *result (as UTF8). /// Returns true on success. storage may be used for temporary storage, to avoid allocations. -static bool append_file_entry(fish_message_type_t type, const wcstring &key_in, +static bool append_file_entry(uvar_message_type_t type, const wcstring &key_in, const wcstring &val_in, std::string *result, std::string *storage) { assert(storage != NULL); assert(result != NULL); @@ -158,7 +161,7 @@ static bool append_file_entry(fish_message_type_t type, const wcstring &key_in, const size_t result_length_on_entry = result->size(); // Append header like "SET " - result->append(type == SET ? SET_MBS : SET_EXPORT_MBS); + result->append(type == uvar_message_type_t::set ? SET_MBS : SET_EXPORT_MBS); result->push_back(' '); // Append variable name like "fish_color_cwd". @@ -300,7 +303,7 @@ void env_universal_t::generate_callbacks(const var_table_t &new_vars, // If the value is not present in new_vars, it has been erased. if (new_vars.find(key) == new_vars.end()) { - callbacks.push_back(callback_data_t(ERASE, key, L"")); + callbacks.push_back(callback_data_t(key, none())); } } @@ -319,8 +322,7 @@ void env_universal_t::generate_callbacks(const var_table_t &new_vars, if (existing == this->vars.end() || existing->second.exports() != new_entry.exports() || existing->second != new_entry) { // Value has changed. - callbacks.push_back(callback_data_t(new_entry.exports() ? SET_EXPORT : SET, key, - new_entry.as_string())); + callbacks.push_back(callback_data_t(key, new_entry.as_string())); } } } @@ -407,8 +409,9 @@ bool env_universal_t::write_to_fd(int fd, const wcstring &path) { // variable; soldier on. const wcstring &key = iter->first; const env_var_t &var = iter->second; - append_file_entry(var.exports() ? SET_EXPORT : SET, key, encode_serialized(var.as_list()), - &contents, &storage); + append_file_entry( + var.exports() ? uvar_message_type_t::set_export : uvar_message_type_t::set, key, + encode_serialized(var.as_list()), &contents, &storage); // Go to next. ++iter; diff --git a/src/env_universal_common.h b/src/env_universal_common.h index 0f0f723d0..c597a031a 100644 --- a/src/env_universal_common.h +++ b/src/env_universal_common.h @@ -13,17 +13,19 @@ #include "env.h" #include "wutil.h" -/// The different types of messages found in the fishd file. -typedef enum { SET, SET_EXPORT, ERASE } fish_message_type_t; - /// Callback data, reflecting a change in universal variables. struct callback_data_t { - fish_message_type_t type; + // The name of the variable. wcstring key; - wcstring val; - callback_data_t(fish_message_type_t t, wcstring k, wcstring v) - : type(t), key(std::move(k)), val(std::move(v)) {} + // The value of the variable, or none if it is erased. + maybe_t val; + + /// Construct from a key and maybe a value. + callback_data_t(wcstring k, maybe_t v) : key(std::move(k)), val(std::move(v)) {} + + /// \return whether this callback represents an erased variable. + bool is_erase() const { return !val.has_value(); } }; typedef std::vector callback_data_list_t; diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 02b5c34a5..70d5938f9 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2915,15 +2915,12 @@ static void test_universal_callbacks() { // Should see exactly three changes. do_test(callbacks.size() == 3); - do_test(callbacks.at(0).type == SET); do_test(callbacks.at(0).key == L"alpha"); - do_test(callbacks.at(0).val == L"2"); - do_test(callbacks.at(1).type == SET_EXPORT); + do_test(callbacks.at(0).val == wcstring{L"2"}); do_test(callbacks.at(1).key == L"beta"); - do_test(callbacks.at(1).val == L"1"); - do_test(callbacks.at(2).type == ERASE); + do_test(callbacks.at(1).val == wcstring{L"1"}); do_test(callbacks.at(2).key == L"delta"); - do_test(callbacks.at(2).val == L""); + do_test(callbacks.at(2).val == none()); (void)system("rm -Rf test/fish_uvars_test/"); } From ce1463bde6311f953f09409a89a4feed2589c603 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 20 Oct 2018 13:44:10 -0700 Subject: [PATCH 02/11] Add line_iterator_t Adds support for splitting a collection into lines. --- src/common.h | 36 ++++++++++++++++++++++++++++++++++++ src/fish_tests.cpp | 17 +++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/src/common.h b/src/common.h index 633570d42..d540d0996 100644 --- a/src/common.h +++ b/src/common.h @@ -379,6 +379,42 @@ wcstring_list_t split_string(const wcstring &val, wchar_t sep); /// Join a list of strings by a separator character. wcstring join_strings(const wcstring_list_t &vals, wchar_t sep); +/// Support for iterating over a newline-separated string. +template +class line_iterator_t { + // Storage for each line. + Collection storage; + + // The collection we're iterating. Note we hold this by reference. + const Collection &coll; + + // The current location in the iteration. + typename Collection::const_iterator current; + +public: + /// Construct from a collection (presumably std::string or std::wcstring). + line_iterator_t(const Collection &coll) : coll(coll), current(coll.cbegin()) {} + + /// Access the storage in which the last line was stored. + const Collection &line() const { + return storage; + } + + /// Advances to the next line. \return true on success, false if we have exhausted the string. + bool next() { + if (current == coll.end()) + return false; + auto newline_or_end = std::find(current, coll.cend(), '\n'); + storage.assign(current, newline_or_end); + current = newline_or_end; + + // Skip the newline. + if (current != coll.cend()) + ++current; + return true; + } +}; + enum fuzzy_match_type_t { // We match the string exactly: FOOBAR matches FOOBAR. fuzzy_match_exact = 0, diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 70d5938f9..cc2dbeef7 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2809,6 +2809,22 @@ static void test_input() { } } +static void test_line_iterator() { + say(L"Testing line iterator"); + + std::string text1 = "Alpha\nBeta\nGamma\n\nDelta\n"; + std::vector lines1; + line_iterator_t iter1(text1); + while (iter1.next()) lines1.push_back(iter1.line()); + do_test((lines1 == std::vector{"Alpha", "Beta", "Gamma", "", "Delta"})); + + wcstring text2 = L"\n\nAlpha\nBeta\nGamma\n\nDelta"; + std::vector lines2; + line_iterator_t iter2(text2); + while (iter2.next()) lines2.push_back(iter2.line()); + do_test((lines2 == std::vector{L"", L"", L"Alpha", L"Beta", L"Gamma", L"", L"Delta"})); +} + #define UVARS_PER_THREAD 8 #define UVARS_TEST_PATH L"test/fish_uvars_test/varsfile.txt" @@ -4805,6 +4821,7 @@ int main(int argc, char **argv) { if (should_test_function("colors")) test_colors(); if (should_test_function("complete")) test_complete(); if (should_test_function("input")) test_input(); + if (should_test_function("line_iterator")) test_line_iterator(); if (should_test_function("universal")) test_universal(); if (should_test_function("universal")) test_universal_callbacks(); if (should_test_function("notifiers")) test_universal_notifiers(); From 11d523e61a33b87bf28989531d0f78d81917bbb7 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 20 Oct 2018 14:38:49 -0700 Subject: [PATCH 03/11] Build out support for multiple file formats in uvars This is in preparation for adjusting the file format to support path variables. --- src/common.cpp | 14 ++++++- src/common.h | 2 + src/env_universal_common.cpp | 71 ++++++++++++++++++++++++++++++++++-- src/env_universal_common.h | 16 +++++++- src/fish_tests.cpp | 22 +++++++++++ 5 files changed, 119 insertions(+), 6 deletions(-) diff --git a/src/common.cpp b/src/common.cpp index 10e2343f8..fac1556ce 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -1744,8 +1744,7 @@ int common_get_width() { return get_current_winsize().ws_col; } int common_get_height() { return get_current_winsize().ws_row; } 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; + return string_prefixes_string(proposed_prefix, value.c_str()); } bool string_prefixes_string(const wcstring &proposed_prefix, const wcstring &value) { @@ -1763,6 +1762,17 @@ bool string_prefixes_string(const wchar_t *proposed_prefix, const wchar_t *value return true; } +bool string_prefixes_string(const char *proposed_prefix, const std::string &value) { + return string_prefixes_string(proposed_prefix, value.c_str()); +} + +bool string_prefixes_string(const char *proposed_prefix, const char *value) { + for (size_t idx = 0; proposed_prefix[idx] != L'\0'; idx++) { + if (proposed_prefix[idx] != value[idx]) return false; + } + return true; +} + bool string_prefixes_string_case_insensitive(const wcstring &proposed_prefix, const wcstring &value) { size_t prefix_size = proposed_prefix.size(); diff --git a/src/common.h b/src/common.h index d540d0996..349974f02 100644 --- a/src/common.h +++ b/src/common.h @@ -342,6 +342,8 @@ std::string wcs2string(const wcstring &input); bool string_prefixes_string(const wcstring &proposed_prefix, const wcstring &value); bool string_prefixes_string(const wchar_t *proposed_prefix, const wcstring &value); bool string_prefixes_string(const wchar_t *proposed_prefix, const wchar_t *value); +bool string_prefixes_string(const char *proposed_prefix, const std::string &value); +bool string_prefixes_string(const char *proposed_prefix, const char *value); /// Test if a string is a suffix of another. bool string_suffixes_string(const wcstring &proposed_suffix, const wcstring &value); diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index bfc5afcf9..999b084cf 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -74,6 +74,9 @@ /// Small note about not editing ~/.fishd manually. Inserted at the top of all .fishd files. #define SAVE_MSG "# This file contains fish universal variable definitions.\n" +/// Version for fish 3.0 +#define UVARS_VERSION_3_0 "3.0" + /// The different types of messages found in the fishd file. enum class uvar_message_type_t { set, set_export }; @@ -764,7 +767,7 @@ var_table_t env_universal_t::read_message_internal(int fd) { // Process it if it's a newline (which is true if we are before the end of the buffer). if (cursor < bufflen && !line.empty()) { if (utf8_to_wchar(line.data(), line.size(), &wide_line, 0)) { - env_universal_t::parse_message_internal(wide_line, &result, &storage); + env_universal_t::parse_message_2x_internal(wide_line, &result, &storage); } line.clear(); } @@ -778,8 +781,70 @@ var_table_t env_universal_t::read_message_internal(int fd) { return result; } -/// Parse message msg/ -void env_universal_t::parse_message_internal(const wcstring &msgstr, var_table_t *vars, +/// \return the format corresponding to file contents \p s. +uvar_format_t env_universal_t::format_for_contents(const std::string &s) { + // Walk over leading comments, looking for one like '# version' + line_iterator_t iter{s}; + while (iter.next()) { + const std::string &line = iter.line(); + if (line.empty()) continue; + if (line.front() != L'#') { + // Exhausted leading comments. + break; + } + // Note scanf %s is max characters to write; add 1 for null terminator. + char versionbuf[64 + 1]; + if (sscanf(line.c_str(), "# VERSION: %64s", versionbuf) != 1) continue; + + // Try reading the version. + if (!strcmp(versionbuf, UVARS_VERSION_3_0)) { + return uvar_format_t::fish_3_0; + } else { + // Unknown future version. + return uvar_format_t::future; + } + } + // No version found, assume 2.x + return uvar_format_t::fish_2_x; +} + +void env_universal_t::populate_variables(const std::string &s, var_table_t *out_vars) { + // Decide on the format. + const uvar_format_t format = format_for_contents(s); + + line_iterator_t iter{s}; + wcstring wide_line; + wcstring storage; + while (iter.next()) { + const std::string &line = iter.line(); + // Skip empties and constants. + if (line.empty() || line.front() == L'#') continue; + + // Convert to UTF8. + wide_line.clear(); + if (!utf8_to_wchar(line.data(), line.size(), &wide_line, 0)) continue; + + switch (format) { + case uvar_format_t::fish_2_x: + env_universal_t::parse_message_2x_internal(wide_line, out_vars, &storage); + break; + case uvar_format_t::fish_3_0: + // For future formats, just try with the most recent one. + case uvar_format_t::future: + env_universal_t::parse_message_30_internal(wide_line, out_vars, &storage); + break; + } + } +} + +/// Parse message msg per fish 3.0 format. +void env_universal_t::parse_message_30_internal(const wcstring &msgstr, var_table_t *vars, + wcstring *storage) { + // TODO. +} + +/// Parse message msg per fish 2.x format. +void env_universal_t::parse_message_2x_internal(const wcstring &msgstr, var_table_t *vars, wcstring *storage) { const wchar_t *msg = msgstr.c_str(); diff --git a/src/env_universal_common.h b/src/env_universal_common.h index c597a031a..daebe7a2c 100644 --- a/src/env_universal_common.h +++ b/src/env_universal_common.h @@ -30,6 +30,10 @@ struct callback_data_t { typedef std::vector callback_data_list_t; +// List of fish universal variable formats. +// This is exposed for testing. +enum class uvar_format_t { fish_2_x, fish_3_0, future }; + bool get_hostname_identifier(wcstring &result); /// Class representing universal variables. class env_universal_t { @@ -68,7 +72,10 @@ class env_universal_t { // vars_to_acquire. void acquire_variables(var_table_t &vars_to_acquire); - static void parse_message_internal(const wcstring &msg, var_table_t *vars, wcstring *storage); + static void parse_message_2x_internal(const wcstring &msg, var_table_t *vars, + wcstring *storage); + static void parse_message_30_internal(const wcstring &msg, var_table_t *vars, + wcstring *storage); static var_table_t read_message_internal(int fd); public: @@ -95,6 +102,13 @@ class env_universal_t { /// Reads and writes variables at the correct path. Returns true if modified variables were /// written. bool sync(callback_data_list_t &callbacks); + + /// Populate a variable table \p out_vars from a \p s string. + /// This is exposed for testing only. + static void populate_variables(const std::string &s, var_table_t *out_vars); + + /// Guess a file format. Exposed for testing only. + static uvar_format_t format_for_contents(const std::string &s); }; /// The "universal notifier" is an object responsible for broadcasting and receiving universal diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index cc2dbeef7..ce1282a9a 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2940,6 +2940,27 @@ static void test_universal_callbacks() { (void)system("rm -Rf test/fish_uvars_test/"); } +static void test_universal_formats() { + say(L"Testing universal format detection"); + const struct { + const char *str; + uvar_format_t format; + } tests[] = { + {"# VERSION: 3.0", uvar_format_t::fish_3_0}, + {"# version: 3.0", uvar_format_t::fish_2_x}, + {"# blah blahVERSION: 3.0", uvar_format_t::fish_2_x}, + {"stuff\n# blah blahVERSION: 3.0", uvar_format_t::fish_2_x}, + {"# blah\n# VERSION: 3.0", uvar_format_t::fish_3_0}, + {"# blah\n#VERSION: 3.0", uvar_format_t::fish_3_0}, + {"# blah\n#VERSION:3.0", uvar_format_t::fish_3_0}, + {"# blah\n#VERSION:3.1", uvar_format_t::future}, + }; + for (const auto &test : tests) { + uvar_format_t format = env_universal_t::format_for_contents(test.str); + do_test(format == test.format); + } +} + bool poll_notifier(const std::unique_ptr ¬e) { bool result = false; if (note->usec_delay_between_polls() > 0) { @@ -4824,6 +4845,7 @@ int main(int argc, char **argv) { if (should_test_function("line_iterator")) test_line_iterator(); if (should_test_function("universal")) test_universal(); if (should_test_function("universal")) test_universal_callbacks(); + if (should_test_function("universal")) test_universal_formats(); if (should_test_function("notifiers")) test_universal_notifiers(); if (should_test_function("completion_insertions")) test_completion_insertions(); if (should_test_function("autosuggestion_ignores")) test_autosuggestion_ignores(); From d98874bd08a03826933c907dd0edc3f80bd6c45a Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 20 Oct 2018 15:55:46 -0700 Subject: [PATCH 04/11] Improve testability factoring of env_universal_t --- src/env.h | 4 +-- src/env_universal_common.cpp | 55 ++++++++++++++---------------------- src/env_universal_common.h | 3 ++ src/fish_tests.cpp | 39 +++++++++++++++++++++++++ 4 files changed, 65 insertions(+), 36 deletions(-) diff --git a/src/env.h b/src/env.h index 114a91cbe..30fb4ab0d 100644 --- a/src/env.h +++ b/src/env.h @@ -127,8 +127,8 @@ class env_var_t { env_var_t &operator=(const env_var_t &var) = default; env_var_t &operator=(env_var_t &&) = default; - bool operator==(const env_var_t &var) const { return vals == var.vals; } - bool operator!=(const env_var_t &var) const { return vals != var.vals; } + bool operator==(const env_var_t &rhs) const { return vals == rhs.vals && flags == rhs.flags; } + bool operator!=(const env_var_t &rhs) const { return ! (*this == rhs); } }; /// Gets the variable with the specified name, or none() if it does not exist. diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 999b084cf..d0c257c54 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -391,45 +391,32 @@ bool env_universal_t::load_from_path(const wcstring &path, callback_data_list_t return result; } +/// Serialize the contents to a string. +std::string env_universal_t::serialize_with_vars(const var_table_t &vars) { + std::string storage; + std::string contents = SAVE_MSG; + for (const auto &kv : vars) { + // Append the entry. Note that append_file_entry may fail, but that only affects one + // variable; soldier on. + const wcstring &key = kv.first; + const env_var_t &var = kv.second; + append_file_entry( + var.exports() ? uvar_message_type_t::set_export : uvar_message_type_t::set, key, + encode_serialized(var.as_list()), &contents, &storage); + } + return contents; +} + /// Writes our state to the fd. path is provided only for error reporting. bool env_universal_t::write_to_fd(int fd, const wcstring &path) { ASSERT_IS_LOCKED(lock); assert(fd >= 0); bool success = true; - - // Stuff we output to fd. - std::string contents; - - // Temporary storage. - std::string storage; - - // Write the save message. If this fails, we don't bother complaining. - write_loop(fd, SAVE_MSG, strlen(SAVE_MSG)); - - var_table_t::const_iterator iter = vars.begin(); - while (iter != vars.end()) { - // Append the entry. Note that append_file_entry may fail, but that only affects one - // variable; soldier on. - const wcstring &key = iter->first; - const env_var_t &var = iter->second; - append_file_entry( - var.exports() ? uvar_message_type_t::set_export : uvar_message_type_t::set, key, - encode_serialized(var.as_list()), &contents, &storage); - - // Go to next. - ++iter; - - // Flush if this is the last iteration or we exceed a page. - if (iter == vars.end() || contents.size() >= 4096) { - if (write_loop(fd, contents.data(), contents.size()) < 0) { - const char *error = strerror(errno); - debug(0, _(L"Unable to write to universal variables file '%ls': %s"), path.c_str(), - error); - success = false; - break; - } - contents.clear(); - } + std::string contents = serialize_with_vars(vars); + if (write_loop(fd, contents.data(), contents.size()) < 0) { + const char *error = strerror(errno); + debug(0, _(L"Unable to write to universal variables file '%ls': %s"), path.c_str(), error); + success = false; } // Since we just wrote out this file, it matches our internal state; pretend we read from it. diff --git a/src/env_universal_common.h b/src/env_universal_common.h index daebe7a2c..263eb297c 100644 --- a/src/env_universal_common.h +++ b/src/env_universal_common.h @@ -109,6 +109,9 @@ class env_universal_t { /// Guess a file format. Exposed for testing only. static uvar_format_t format_for_contents(const std::string &s); + + /// Serialize a variable list. Exposed for testing only. + static std::string serialize_with_vars(const var_table_t &vars); }; /// The "universal notifier" is an object responsible for broadcasting and receiving universal diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index ce1282a9a..7ccf73bdc 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2888,6 +2888,35 @@ static void test_universal() { (void)system("rm -Rf test/fish_uvars_test/"); } +static void test_universal_output() { + say(L"Testing universal variable output"); + var_table_t vars; + vars[L"varA"] = env_var_t(wcstring_list_t{L"ValA1", L"ValA2"}, 0); + vars[L"varB"] = env_var_t(wcstring_list_t{L"ValB1"}, env_var_t::flag_export); + std::string text = env_universal_t::serialize_with_vars(vars); + const char *expected = + "# This file contains fish universal variable definitions.\n" + "SET varA:ValA1\\x1eValA2\n" + "SET_EXPORT varB:ValB1\n"; + do_test(text == expected); +} + +static void test_universal_parsing() { + say(L"Testing universal variable parsing"); + const char *input = + "# This file contains fish universal variable definitions.\n" + "SET varA:ValA1\\x1eValA2\n" + "SET_EXPORT varB:ValB1\n"; + + var_table_t vars; + vars[L"varA"] = env_var_t(wcstring_list_t{L"ValA1", L"ValA2"}, 0); + vars[L"varB"] = env_var_t(wcstring_list_t{L"ValB1"}, env_var_t::flag_export); + + var_table_t parsed_vars; + env_universal_t::populate_variables(input, &parsed_vars); + do_test(vars == parsed_vars); +} + static bool callback_data_less_than(const callback_data_t &a, const callback_data_t &b) { return a.key < b.key; } @@ -4602,6 +4631,14 @@ static void test_timezone_env_vars() { static void test_env_vars() { test_timezone_env_vars(); // TODO: Add tests for the locale and ncurses vars. + + env_var_t v1 = {L"abc", env_var_t::flag_export}; + env_var_t v2 = {wcstring_list_t{L"abc"}, env_var_t::flag_export}; + env_var_t v3 = {wcstring_list_t{L"abc"}, 0}; + env_var_t v4 = {wcstring_list_t{L"abc", L"def"}, env_var_t::flag_export}; + do_test(v1 == v2 && ! (v1 != v2)); + do_test(v1 != v3 && ! (v1 == v3)); + do_test(v1 != v4 && ! (v1 == v4)); } static void test_illegal_command_exit_code() { @@ -4844,6 +4881,8 @@ int main(int argc, char **argv) { if (should_test_function("input")) test_input(); if (should_test_function("line_iterator")) test_line_iterator(); if (should_test_function("universal")) test_universal(); + if (should_test_function("universal")) test_universal_output(); + if (should_test_function("universal")) test_universal_parsing(); if (should_test_function("universal")) test_universal_callbacks(); if (should_test_function("universal")) test_universal_formats(); if (should_test_function("notifiers")) test_universal_notifiers(); From adc69f94da8c8ba4911edd11d6378c78895bb82f Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 20 Oct 2018 17:20:49 -0700 Subject: [PATCH 05/11] Support parsing the new universal variable format --- src/env_universal_common.cpp | 113 +++++++++++++++++++++++++---------- src/env_universal_common.h | 3 + src/fish_tests.cpp | 27 +++++++++ 3 files changed, 111 insertions(+), 32 deletions(-) diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index d0c257c54..9762b105f 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -77,6 +77,15 @@ /// Version for fish 3.0 #define UVARS_VERSION_3_0 "3.0" +// Fields used in fish 3.0 uvars +namespace fish3_uvars { +namespace { +constexpr const wchar_t *SETUVAR = L"SETUVAR"; +constexpr const wchar_t *EXPORT = L"--export"; +constexpr const wchar_t *PATH = L"--path"; +} // namespace +} // namespace fish3_uvars + /// The different types of messages found in the fishd file. enum class uvar_message_type_t { set, set_export }; @@ -108,12 +117,13 @@ static maybe_t default_vars_path() { } /// Test if the message msg contains the command cmd. -static bool match(const wchar_t *msg, const wchar_t *cmd) { +/// On success, updates the cursor to just past the command. +static bool match(const wchar_t **inout_cursor, const wchar_t *cmd) { + const wchar_t *cursor = *inout_cursor; size_t len = wcslen(cmd); - if (wcsncasecmp(msg, cmd, len) != 0) return false; - - if (msg[len] && msg[len] != L' ' && msg[len] != L'\t') return false; - + if (wcsncasecmp(cursor, cmd, len) != 0) return false; + if (cursor[len] && cursor[len] != L' ' && cursor[len] != L'\t') return false; + *inout_cursor = cursor + len; return true; } @@ -824,46 +834,85 @@ void env_universal_t::populate_variables(const std::string &s, var_table_t *out_ } } +static const wchar_t *skip_spaces(const wchar_t *str) { + while (*str == L' ' || *str == L'\t') str++; + return str; +} + +bool env_universal_t::populate_1_variable(const wchar_t *input, env_var_t::env_var_flags_t flags, + var_table_t *vars, wcstring *storage) { + const wchar_t *str = skip_spaces(input); + const wchar_t *colon = wcschr(str, L':'); + if (!colon) return false; + + // Parse out the value into storage, and decode it into a variable. + storage->clear(); + if (!unescape_string(colon + 1, storage, 0)) { + return false; + } + env_var_t var{decode_serialized(*storage), flags}; + + // Parse out the key and write into the map. + storage->assign(str, colon - str); + const wcstring &key = *storage; + (*vars)[key] = std::move(var); + return true; +} + /// Parse message msg per fish 3.0 format. void env_universal_t::parse_message_30_internal(const wcstring &msgstr, var_table_t *vars, wcstring *storage) { - // TODO. + namespace f3 = fish3_uvars; + const wchar_t *const msg = msgstr.c_str(); + if (msg[0] == L'#') return; + + const wchar_t *cursor = msg; + if (!match(&cursor, f3::SETUVAR)) { + debug(1, PARSE_ERR, msg); + return; + } + // Parse out flags. + env_var_t::env_var_flags_t flags = 0; + for (;;) { + cursor = skip_spaces(cursor); + if (*cursor != L'-') break; + if (match(&cursor, f3::EXPORT)) { + flags |= env_var_t::flag_export; + } else if (match(&cursor, f3::PATH)) { + flags |= env_var_t::flag_pathvar; + } else { + // Skip this unknown flag, for future proofing. + while (*cursor && *cursor != L' ' && *cursor != L'\t') cursor++; + } + } + + // Populate the variable with these flags. + if (!populate_1_variable(cursor, flags, vars, storage)) { + debug(1, PARSE_ERR, msg); + } } /// Parse message msg per fish 2.x format. void env_universal_t::parse_message_2x_internal(const wcstring &msgstr, var_table_t *vars, wcstring *storage) { - const wchar_t *msg = msgstr.c_str(); + const wchar_t *const msg = msgstr.c_str(); + const wchar_t *cursor = msg; // debug(3, L"parse_message( %ls );", msg); - if (msg[0] == L'#') return; + if (cursor[0] == L'#') return; - bool is_set_export = match(msg, SET_EXPORT_STR); - bool is_set = !is_set_export && match(msg, SET_STR); - if (is_set || is_set_export) { - const wchar_t *name, *tmp; - const bool exportv = is_set_export; - - name = msg + (exportv ? wcslen(SET_EXPORT_STR) : wcslen(SET_STR)); - while (name[0] == L'\t' || name[0] == L' ') name++; - - tmp = wcschr(name, L':'); - if (tmp) { - // Use 'storage' to hold our key to avoid allocations. - storage->assign(name, tmp - name); - const wcstring &key = *storage; - - wcstring val; - if (unescape_string(tmp + 1, &val, 0)) { - env_var_t &entry = (*vars)[key]; - entry.set_exports(exportv); - entry.set_vals(decode_serialized(val)); - } - } else { - debug(1, PARSE_ERR, msg); - } + env_var_t::env_var_flags_t flags = 0; + if (match(&cursor, SET_EXPORT_STR)) { + flags |= env_var_t::flag_export; + } else if (match(&cursor, SET_STR)) { + flags |= 0; } else { debug(1, PARSE_ERR, msg); + return; + } + + if (!populate_1_variable(cursor, flags, vars, storage)) { + debug(1, PARSE_ERR, msg); } } diff --git a/src/env_universal_common.h b/src/env_universal_common.h index 263eb297c..caa4f774d 100644 --- a/src/env_universal_common.h +++ b/src/env_universal_common.h @@ -72,6 +72,9 @@ class env_universal_t { // vars_to_acquire. void acquire_variables(var_table_t &vars_to_acquire); + static bool populate_1_variable(const wchar_t *str, env_var_t::env_var_flags_t flags, + var_table_t *vars, wcstring *storage); + static void parse_message_2x_internal(const wcstring &msg, var_table_t *vars, wcstring *storage); static void parse_message_30_internal(const wcstring &msg, var_table_t *vars, diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 7ccf73bdc..dfc9ede61 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2903,6 +2903,32 @@ static void test_universal_output() { static void test_universal_parsing() { say(L"Testing universal variable parsing"); + const char *input = + "# This file contains fish universal variable definitions.\n" + "# VERSION: 3.0\n" + "SETUVAR varA:ValA1\\x1eValA2\n" + "SETUVAR --export varB:ValB1\n" + "SETUVAR --nonsenseflag varC:ValC1\n" + "SETUVAR --export --path varD:ValD1\n" + "SETUVAR --path --path varE:ValE1\\x1eValE2\n"; + + const env_var_t::env_var_flags_t flag_export = env_var_t::flag_export; + const env_var_t::env_var_flags_t flag_pathvar = env_var_t::flag_pathvar; + + var_table_t vars; + vars[L"varA"] = env_var_t(wcstring_list_t{L"ValA1", L"ValA2"}, 0); + vars[L"varB"] = env_var_t(wcstring_list_t{L"ValB1"}, flag_export); + vars[L"varC"] = env_var_t(wcstring_list_t{L"ValC1"}, 0); + vars[L"varD"] = env_var_t(wcstring_list_t{L"ValD1"}, flag_export | flag_pathvar); + vars[L"varE"] = env_var_t(wcstring_list_t{L"ValE1", L"ValE2"}, flag_pathvar); + + var_table_t parsed_vars; + env_universal_t::populate_variables(input, &parsed_vars); + do_test(vars == parsed_vars); +} + +static void test_universal_parsing_legacy() { + say(L"Testing universal variable legacy parsing"); const char *input = "# This file contains fish universal variable definitions.\n" "SET varA:ValA1\\x1eValA2\n" @@ -4883,6 +4909,7 @@ int main(int argc, char **argv) { if (should_test_function("universal")) test_universal(); if (should_test_function("universal")) test_universal_output(); if (should_test_function("universal")) test_universal_parsing(); + if (should_test_function("universal")) test_universal_parsing_legacy(); if (should_test_function("universal")) test_universal_callbacks(); if (should_test_function("universal")) test_universal_formats(); if (should_test_function("notifiers")) test_universal_notifiers(); From fe485f24850e6139418ba73f460cf85104e3bc2d Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 20 Oct 2018 18:12:01 -0700 Subject: [PATCH 06/11] Adopt populate_varabless in universal variables --- src/env_universal_common.cpp | 53 ++++++++++++------------------------ 1 file changed, 17 insertions(+), 36 deletions(-) diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 9762b105f..6cce97c24 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -77,6 +77,9 @@ /// Version for fish 3.0 #define UVARS_VERSION_3_0 "3.0" +// Maximum file size we'll read. +static constexpr size_t k_max_read_size = 16 * 1024 * 1024; + // Fields used in fish 3.0 uvars namespace fish3_uvars { namespace { @@ -733,48 +736,26 @@ bool env_universal_t::sync(callback_data_list_t &callbacks) { var_table_t env_universal_t::read_message_internal(int fd) { var_table_t result; - // Temp value used to avoid repeated allocations. - wcstring storage; - - // The line we construct (and then parse). - std::string line; - wcstring wide_line; - for (;;) { - // Read into a buffer. Note this is NOT null-terminated! - char buffer[1024]; + // Read everything from the fd. Put a sane limit on it. + std::string contents; + while (contents.size() < k_max_read_size) { + char buffer[4096]; ssize_t amt = read_loop(fd, buffer, sizeof buffer); if (amt <= 0) { break; } - const size_t bufflen = (size_t)amt; - - // Walk over it by lines. The contents of an unterminated line will be left in 'line' for - // the next iteration. - ssize_t line_start = 0; - while (line_start < amt) { - // Run until we hit a newline. - size_t cursor = line_start; - while (cursor < bufflen && buffer[cursor] != '\n') { - cursor++; - } - - // Copy over what we read. - line.append(buffer + line_start, cursor - line_start); - - // Process it if it's a newline (which is true if we are before the end of the buffer). - if (cursor < bufflen && !line.empty()) { - if (utf8_to_wchar(line.data(), line.size(), &wide_line, 0)) { - env_universal_t::parse_message_2x_internal(wide_line, &result, &storage); - } - line.clear(); - } - - // Skip over the newline (or skip past the end). - line_start = cursor + 1; - } + contents.append(buffer, amt); } - // We make no effort to handle an unterminated last line. + // Handle overlong files. + if (contents.size() >= k_max_read_size) { + contents.resize(k_max_read_size); + // Back up to a newline. + size_t newline = contents.rfind('\n'); + contents.resize(newline == wcstring::npos ? 0 : newline); + } + + populate_variables(contents, &result); return result; } From 11c77abc8c9bd99f5bdc753e7deffb73b0e304b4 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 20 Oct 2018 18:19:30 -0700 Subject: [PATCH 07/11] Make universal variable matching case sensitive --- src/env_universal_common.cpp | 41 ++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 6cce97c24..30d57978a 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -56,17 +56,6 @@ #include #endif // Haiku -/// The set command. -#define SET_STR L"SET" - -/// The set_export command. -#define SET_EXPORT_STR L"SET_EXPORT" - -/// Non-wide version of the set command. -#define SET_MBS "SET" - -/// Non-wide version of the set_export command. -#define SET_EXPORT_MBS "SET_EXPORT" /// Error message. #define PARSE_ERR L"Unable to parse universal variable message: '%ls'" @@ -80,12 +69,20 @@ // Maximum file size we'll read. static constexpr size_t k_max_read_size = 16 * 1024 * 1024; +// Fields used in fish 2.x uvars. +namespace fish2x_uvars { +namespace { +constexpr const char *SET = "SET"; +constexpr const char *SET_EXPORT = "SET_EXPORT"; +} // namespace +} // namespace fish2x_uvars + // Fields used in fish 3.0 uvars namespace fish3_uvars { namespace { -constexpr const wchar_t *SETUVAR = L"SETUVAR"; -constexpr const wchar_t *EXPORT = L"--export"; -constexpr const wchar_t *PATH = L"--path"; +constexpr const char *SETUVAR = "SETUVAR"; +constexpr const char *EXPORT = "--export"; +constexpr const char *PATH = "--path"; } // namespace } // namespace fish3_uvars @@ -121,10 +118,12 @@ static maybe_t default_vars_path() { /// Test if the message msg contains the command cmd. /// On success, updates the cursor to just past the command. -static bool match(const wchar_t **inout_cursor, const wchar_t *cmd) { +static bool match(const wchar_t **inout_cursor, const char *cmd) { const wchar_t *cursor = *inout_cursor; - size_t len = wcslen(cmd); - if (wcsncasecmp(cursor, cmd, len) != 0) return false; + size_t len = strlen(cmd); + if (!std::equal(cmd, cmd + len, cursor)) { + return false; + } if (cursor[len] && cursor[len] != L' ' && cursor[len] != L'\t') return false; *inout_cursor = cursor + len; return true; @@ -171,13 +170,14 @@ static bool append_file_entry(uvar_message_type_t type, const wcstring &key_in, const wcstring &val_in, std::string *result, std::string *storage) { assert(storage != NULL); assert(result != NULL); + namespace f2x = fish2x_uvars; // Record the length on entry, in case we need to back up. bool success = true; const size_t result_length_on_entry = result->size(); // Append header like "SET " - result->append(type == uvar_message_type_t::set ? SET_MBS : SET_EXPORT_MBS); + result->append(type == uvar_message_type_t::set ? f2x::SET : f2x::SET_EXPORT); result->push_back(' '); // Append variable name like "fish_color_cwd". @@ -876,6 +876,7 @@ void env_universal_t::parse_message_30_internal(const wcstring &msgstr, var_tabl /// Parse message msg per fish 2.x format. void env_universal_t::parse_message_2x_internal(const wcstring &msgstr, var_table_t *vars, wcstring *storage) { + namespace f2x = fish2x_uvars; const wchar_t *const msg = msgstr.c_str(); const wchar_t *cursor = msg; @@ -883,9 +884,9 @@ void env_universal_t::parse_message_2x_internal(const wcstring &msgstr, var_tabl if (cursor[0] == L'#') return; env_var_t::env_var_flags_t flags = 0; - if (match(&cursor, SET_EXPORT_STR)) { + if (match(&cursor, f2x::SET_EXPORT)) { flags |= env_var_t::flag_export; - } else if (match(&cursor, SET_STR)) { + } else if (match(&cursor, f2x::SET)) { flags |= 0; } else { debug(1, PARSE_ERR, msg); From d18e2d970c1da037e7fa93975ffbe15f7979cf55 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 20 Oct 2018 18:21:05 -0700 Subject: [PATCH 08/11] Switch to new universal variable format Example line: SETUVAR --export foo:bar --- src/env_universal_common.cpp | 28 +++++++++++++++++++--------- src/fish_tests.cpp | 18 +++++++++++++++--- 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 30d57978a..c925e29d0 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -56,7 +56,6 @@ #include #endif // Haiku - /// Error message. #define PARSE_ERR L"Unable to parse universal variable message: '%ls'" @@ -166,20 +165,30 @@ static bool append_utf8(const wcstring &input, std::string *receiver, std::strin /// Creates a file entry like "SET fish_color_cwd:FF0". Appends the result to *result (as UTF8). /// Returns true on success. storage may be used for temporary storage, to avoid allocations. -static bool append_file_entry(uvar_message_type_t type, const wcstring &key_in, +static bool append_file_entry(env_var_t::env_var_flags_t flags, const wcstring &key_in, const wcstring &val_in, std::string *result, std::string *storage) { + namespace f3 = fish3_uvars; assert(storage != NULL); assert(result != NULL); - namespace f2x = fish2x_uvars; // Record the length on entry, in case we need to back up. bool success = true; const size_t result_length_on_entry = result->size(); - // Append header like "SET " - result->append(type == uvar_message_type_t::set ? f2x::SET : f2x::SET_EXPORT); + // Append SETVAR header. + result->append(f3::SETUVAR); result->push_back(' '); + // Append flags. + if (flags & env_var_t::flag_export) { + result->append(f3::EXPORT); + result->push_back(' '); + } + if (flags & env_var_t::flag_pathvar) { + result->append(f3::PATH); + result->push_back(' '); + } + // Append variable name like "fish_color_cwd". if (!valid_var_name(key_in)) { debug(0, L"Illegal variable name: '%ls'", key_in.c_str()); @@ -407,15 +416,16 @@ bool env_universal_t::load_from_path(const wcstring &path, callback_data_list_t /// Serialize the contents to a string. std::string env_universal_t::serialize_with_vars(const var_table_t &vars) { std::string storage; - std::string contents = SAVE_MSG; + std::string contents; + contents.append(SAVE_MSG); + contents.append("# VERSION: " UVARS_VERSION_3_0 "\n"); + for (const auto &kv : vars) { // Append the entry. Note that append_file_entry may fail, but that only affects one // variable; soldier on. const wcstring &key = kv.first; const env_var_t &var = kv.second; - append_file_entry( - var.exports() ? uvar_message_type_t::set_export : uvar_message_type_t::set, key, - encode_serialized(var.as_list()), &contents, &storage); + append_file_entry(var.get_flags(), key, encode_serialized(var.as_list()), &contents, &storage); } return contents; } diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index dfc9ede61..c591196db 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2890,14 +2890,26 @@ static void test_universal() { static void test_universal_output() { say(L"Testing universal variable output"); + + const env_var_t::env_var_flags_t flag_export = env_var_t::flag_export; + const env_var_t::env_var_flags_t flag_pathvar = env_var_t::flag_pathvar; + var_table_t vars; vars[L"varA"] = env_var_t(wcstring_list_t{L"ValA1", L"ValA2"}, 0); - vars[L"varB"] = env_var_t(wcstring_list_t{L"ValB1"}, env_var_t::flag_export); + vars[L"varB"] = env_var_t(wcstring_list_t{L"ValB1"}, flag_export); + vars[L"varC"] = env_var_t(wcstring_list_t{L"ValC1"}, 0); + vars[L"varD"] = env_var_t(wcstring_list_t{L"ValD1"}, flag_export | flag_pathvar); + vars[L"varE"] = env_var_t(wcstring_list_t{L"ValE1", L"ValE2"}, flag_pathvar); + std::string text = env_universal_t::serialize_with_vars(vars); const char *expected = "# This file contains fish universal variable definitions.\n" - "SET varA:ValA1\\x1eValA2\n" - "SET_EXPORT varB:ValB1\n"; + "# VERSION: 3.0\n" + "SETUVAR varA:ValA1\\x1eValA2\n" + "SETUVAR --export varB:ValB1\n" + "SETUVAR varC:ValC1\n" + "SETUVAR --export --path varD:ValD1\n" + "SETUVAR --path varE:ValE1\\x1eValE2\n"; do_test(text == expected); } From 7b5296817ac0a72ba540bddc58c161106f0ef929 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 21 Oct 2018 00:31:30 -0700 Subject: [PATCH 09/11] Teach universal variables to not overwrite future file formats --- src/env_universal_common.cpp | 68 ++++++++++++++++++++++-------------- src/env_universal_common.h | 14 ++++++-- src/fish_tests.cpp | 29 +++++++++++++++ 3 files changed, 83 insertions(+), 28 deletions(-) diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index c925e29d0..517a276ff 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -381,7 +381,14 @@ void env_universal_t::load_from_fd(int fd, callback_data_list_t &callbacks) { debug(5, L"universal log sync elided based on fstat()"); } else { // Read a variables table from the file. - var_table_t new_vars = this->read_message_internal(fd); + var_table_t new_vars; + uvar_format_t format = this->read_message_internal(fd, &new_vars); + + // Hacky: if the read format is in the future, avoid overwriting the file: never try to + // save. + if (format == uvar_format_t::future) { + ok_to_save = false; + } // Announce changes. this->generate_callbacks(new_vars, callbacks); @@ -659,8 +666,6 @@ bool env_universal_t::sync(callback_data_list_t &callbacks) { const wcstring directory = wdirname(vars_path); bool success = true; int vars_fd = -1; - int private_fd = -1; - wcstring private_file_path; debug(5, L"universal log performing full sync"); @@ -676,12 +681,30 @@ bool env_universal_t::sync(callback_data_list_t &callbacks) { this->load_from_fd(vars_fd, callbacks); } - // Open adjacent temporary file. - if (success) { - success = this->open_temporary_file(directory, &private_file_path, &private_fd); - if (!success) debug(5, L"universal log open_temporary_file() failed"); + if (success && ok_to_save) { + success = this->save(directory, vars_path); } + // Clean up. + if (vars_fd >= 0) { + close(vars_fd); + } + + return success; +} + +// Write our file contents. +// \return true on success, false on failure. +bool env_universal_t::save(const wcstring &directory, const wcstring &vars_path) { + assert(ok_to_save && "It's not OK to save"); + + int private_fd = -1; + wcstring private_file_path; + + // Open adjacent temporary file. + bool success = this->open_temporary_file(directory, &private_file_path, &private_fd); + if (!success) debug(5, L"universal log open_temporary_file() failed"); + // Write to it. if (success) { assert(private_fd >= 0); @@ -698,14 +721,14 @@ bool env_universal_t::sync(callback_data_list_t &callbacks) { if (fchmod(private_fd, sbuf.st_mode) == -1) debug(5, L"universal log fchmod() failed"); } -// Linux by default stores the mtime with low precision, low enough that updates that occur in quick -// succession may result in the same mtime (even the nanoseconds field). So manually set the mtime -// of the new file to a high-precision clock. Note that this is only necessary because Linux -// aggressively reuses inodes, causing the ABA problem; on other platforms we tend to notice the -// file has changed due to a different inode (or file size!) -// -// It's probably worth finding a simpler solution to this. The tests ran into this, but it's -// unlikely to affect users. + // Linux by default stores the mtime with low precision, low enough that updates that occur + // in quick succession may result in the same mtime (even the nanoseconds field). So + // manually set the mtime of the new file to a high-precision clock. Note that this is only + // necessary because Linux aggressively reuses inodes, causing the ABA problem; on other + // platforms we tend to notice the file has changed due to a different inode (or file size!) + // + // It's probably worth finding a simpler solution to this. The tests ran into this, but it's + // unlikely to affect users. #if HAVE_CLOCK_GETTIME && HAVE_FUTIMENS struct timespec times[2] = {}; times[0].tv_nsec = UTIME_OMIT; // don't change ctime @@ -725,27 +748,20 @@ bool env_universal_t::sync(callback_data_list_t &callbacks) { } // Clean up. - if (vars_fd >= 0) { - close(vars_fd); - } if (private_fd >= 0) { close(private_fd); } if (!private_file_path.empty()) { wunlink(private_file_path); } - if (success) { // All of our modified variables have now been written out. modified.clear(); } - return success; } -var_table_t env_universal_t::read_message_internal(int fd) { - var_table_t result; - +uvar_format_t env_universal_t::read_message_internal(int fd, var_table_t *vars) { // Read everything from the fd. Put a sane limit on it. std::string contents; while (contents.size() < k_max_read_size) { @@ -765,8 +781,7 @@ var_table_t env_universal_t::read_message_internal(int fd) { contents.resize(newline == wcstring::npos ? 0 : newline); } - populate_variables(contents, &result); - return result; + return populate_variables(contents, vars); } /// \return the format corresponding to file contents \p s. @@ -796,7 +811,7 @@ uvar_format_t env_universal_t::format_for_contents(const std::string &s) { return uvar_format_t::fish_2_x; } -void env_universal_t::populate_variables(const std::string &s, var_table_t *out_vars) { +uvar_format_t env_universal_t::populate_variables(const std::string &s, var_table_t *out_vars) { // Decide on the format. const uvar_format_t format = format_for_contents(s); @@ -823,6 +838,7 @@ void env_universal_t::populate_variables(const std::string &s, var_table_t *out_ break; } } + return format; } static const wchar_t *skip_spaces(const wchar_t *str) { diff --git a/src/env_universal_common.h b/src/env_universal_common.h index caa4f774d..7ea32e665 100644 --- a/src/env_universal_common.h +++ b/src/env_universal_common.h @@ -48,6 +48,10 @@ class env_universal_t { // Path that we save to. If empty, use the default. const wcstring explicit_vars_path; + // Whether it's OK to save. This may be set to false if we discover that a future version of + // fish wrote the uvars contents. + bool ok_to_save{true}; + mutable fish_mutex_t lock; bool load_from_path(const wcstring &path, callback_data_list_t &callbacks); void load_from_fd(int fd, callback_data_list_t &callbacks); @@ -79,7 +83,9 @@ class env_universal_t { wcstring *storage); static void parse_message_30_internal(const wcstring &msg, var_table_t *vars, wcstring *storage); - static var_table_t read_message_internal(int fd); + static uvar_format_t read_message_internal(int fd, var_table_t *vars); + + bool save(const wcstring &directory, const wcstring &vars_path); public: explicit env_universal_t(wcstring path); @@ -108,13 +114,17 @@ class env_universal_t { /// Populate a variable table \p out_vars from a \p s string. /// This is exposed for testing only. - static void populate_variables(const std::string &s, var_table_t *out_vars); + /// \return the format of the file that we read. + static uvar_format_t populate_variables(const std::string &s, var_table_t *out_vars); /// Guess a file format. Exposed for testing only. static uvar_format_t format_for_contents(const std::string &s); /// Serialize a variable list. Exposed for testing only. static std::string serialize_with_vars(const var_table_t &vars); + + /// Exposed for testing only. + bool is_ok_to_save() const { return ok_to_save; } }; /// The "universal notifier" is an object responsible for broadcasting and receiving universal diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index c591196db..ac5f58a31 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -3028,6 +3028,34 @@ static void test_universal_formats() { } } +static void test_universal_ok_to_save() { + // Ensure we don't try to save after reading from a newer fish. + say(L"Testing universal Ok to save"); + if (system("mkdir -p test/fish_uvars_test/")) err(L"mkdir failed"); + const char *contents = "# VERSION: 99999.99\n"; + FILE *fp = wfopen(UVARS_TEST_PATH, "w"); + assert(fp && "Failed to open UVARS_TEST_PATH for writing"); + fwrite(contents, strlen(contents), 1, fp); + fclose(fp); + + file_id_t before_id = file_id_for_path(UVARS_TEST_PATH); + do_test(before_id != kInvalidFileID && "UVARS_TEST_PATH should be readable"); + + callback_data_list_t cbs; + env_universal_t uvars(UVARS_TEST_PATH); + do_test(uvars.is_ok_to_save() && "Should be OK to save before sync"); + uvars.sync(cbs); + cbs.clear(); + do_test(!uvars.is_ok_to_save() && "Should no longer be OK to save"); + uvars.set(L"SOMEVAR", {L"SOMEVALUE"}, false); + uvars.sync(cbs); + + // Ensure file is same. + file_id_t after_id = file_id_for_path(UVARS_TEST_PATH); + do_test(before_id == after_id && "UVARS_TEST_PATH should not have changed"); + (void)system("rm -Rf test/fish_uvars_test/"); +} + bool poll_notifier(const std::unique_ptr ¬e) { bool result = false; if (note->usec_delay_between_polls() > 0) { @@ -4924,6 +4952,7 @@ int main(int argc, char **argv) { if (should_test_function("universal")) test_universal_parsing_legacy(); if (should_test_function("universal")) test_universal_callbacks(); if (should_test_function("universal")) test_universal_formats(); + if (should_test_function("universal")) test_universal_ok_to_save(); if (should_test_function("notifiers")) test_universal_notifiers(); if (should_test_function("completion_insertions")) test_completion_insertions(); if (should_test_function("autosuggestion_ignores")) test_autosuggestion_ignores(); From 9690ac5974c3f57b582c65ecbd876583075ee5f2 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 21 Oct 2018 11:12:11 -0700 Subject: [PATCH 10/11] Rename fish_universal_variables file to fish_variables This is to avoid development versions of fish 3.0 freaking out when the file format is changed. We now have better support for for future universal variable formats so it's unlikely we'll have to change the file name again. --- CHANGELOG.md | 2 +- src/env_universal_common.cpp | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2b0e7c00..b9260e29d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,7 +39,7 @@ fish 3.0 is a major release which brings with it both improvements in functional - `while` sets `$status` to a more useful value (#4982) - Command substitution output is now limited to 10 MB by default (#3822). - The machine hostname, where available, is now exposed as `$hostname` which is now a reserved variable. This drops the dependency on the `hostname` executable (#4422). -- The universal variables file no longer contains the MAC address. It is now at the fixed location `.config/fish/fish_universal_variables` (#1912). +- The universal variables file no longer contains the MAC address. It is now at the fixed location `.config/fish/fish_variables` (#1912). - Bare `bind` invocations in config.fish now work. The `fish_user_key_bindings` function is no longer necessary, but will still be executed if it exists (#5191). - `$fish_pid` and `$last_pid` are available as an alternatives to `%self` and `%last`. diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 517a276ff..2797268f9 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -93,6 +93,10 @@ static wcstring get_machine_identifier(); /// return a list of paths where the uvars file has been historically stored. static wcstring_list_t get_legacy_paths(const wcstring &wdir) { wcstring_list_t result; + // A path used during fish 3.0 development. + result.push_back(wdir + L"/fish_universal_variables"); + + // Paths used in 2.x. result.push_back(wdir + L"/fishd." + get_machine_identifier()); wcstring hostname_id; if (get_hostname_identifier(hostname_id)) { @@ -109,7 +113,7 @@ static maybe_t default_vars_path_directory() { static maybe_t default_vars_path() { if (auto path = default_vars_path_directory()) { - path->append(L"/fish_universal_variables"); + path->append(L"/fish_variables"); return path; } return none(); From 5899694233bbc9bc34a53c1e24b3d1b4d5f27b5b Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Mon, 22 Oct 2018 00:49:09 -0700 Subject: [PATCH 11/11] Allow setting universal path variables Support for path and unpath in universal variables. Fixes #5271 --- src/env.cpp | 77 ++++++++++++++++++++---------------- src/env_universal_common.cpp | 19 ++++----- src/env_universal_common.h | 8 ++-- src/fish_tests.cpp | 30 +++++++------- tests/test3.err | 3 ++ tests/test3.in | 14 +++++++ tests/test3.out | 7 ++++ 7 files changed, 96 insertions(+), 62 deletions(-) diff --git a/src/env.cpp b/src/env.cpp index 19b799b3c..e502daa92 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -1014,6 +1014,38 @@ static int set_umask(const wcstring_list_t &list_val) { return ENV_OK; } +/// Set a universal variable, inheriting as applicable from the given old variable. +static void env_set_internal_universal(const wcstring &key, wcstring_list_t val, + env_mode_flags_t input_var_mode) { + ASSERT_IS_MAIN_THREAD(); + if (!uvars()) return; + env_mode_flags_t var_mode = input_var_mode; + auto oldvar = uvars()->get(key); + // Resolve whether or not to export. + if ((var_mode & (ENV_EXPORT | ENV_UNEXPORT)) == 0) { + bool doexport = oldvar ? oldvar->exports() : false; + var_mode |= (doexport ? ENV_EXPORT : ENV_UNEXPORT); + } + // Resolve whether to be a path variable. + // Here we fall back to the auto-pathvar behavior. + if ((var_mode & (ENV_PATHVAR | ENV_UNPATHVAR)) == 0) { + bool dopathvar = oldvar ? oldvar->is_pathvar() : variable_should_auto_pathvar(key); + var_mode |= (dopathvar ? ENV_PATHVAR : ENV_UNPATHVAR); + } + + // Construct and set the new variable. + env_var_t::env_var_flags_t varflags = 0; + if (var_mode & ENV_EXPORT) varflags |= env_var_t::flag_export; + if (var_mode & ENV_PATHVAR) varflags |= env_var_t::flag_pathvar; + env_var_t new_var{val, varflags}; + + uvars()->set(key, new_var); + env_universal_barrier(); + if (new_var.exports() || (oldvar && oldvar->exports())) { + vars_stack().mark_changed_exported(); + } +} + /// Set the value of the environment variable whose name matches key to val. /// /// \param key The key @@ -1062,24 +1094,8 @@ static int env_set_internal(const wcstring &key, env_mode_flags_t input_var_mode } if (var_mode & ENV_UNIVERSAL) { - const bool old_export = uvars() && uvars()->get_export(key); - bool new_export; - if (var_mode & ENV_EXPORT) { - // Export the var. - new_export = true; - } else if (var_mode & ENV_UNEXPORT) { - // Unexport the var. - new_export = false; - } else { - // Not changing the export status of the var. - new_export = old_export; - } if (uvars()) { - uvars()->set(key, val, new_export); - env_universal_barrier(); - if (old_export || new_export) { - vars_stack().mark_changed_exported(); - } + env_set_internal_universal(key, std::move(val), var_mode); } } else { // Determine the node. @@ -1113,20 +1129,10 @@ static int env_set_internal(const wcstring &key, env_mode_flags_t input_var_mode set_proc_had_barrier(true); env_universal_barrier(); } - if (uvars() && uvars()->get(key)) { - bool exportv; - if (var_mode & ENV_EXPORT) { - exportv = true; - } else if (var_mode & ENV_UNEXPORT) { - exportv = false; - } else { - exportv = uvars()->get_export(key); - } - uvars()->set(key, val, exportv); - env_universal_barrier(); - done = 1; - + // Modifying an existing universal variable. + env_set_internal_universal(key, std::move(val), var_mode); + done = true; } else { // New variable with unspecified scope node = vars_stack().resolve_unspecified_scope(); @@ -1268,8 +1274,13 @@ int env_remove(const wcstring &key, int var_mode) { } if (!erased && !(var_mode & ENV_GLOBAL) && !(var_mode & ENV_LOCAL)) { - bool is_exported = uvars()->get_export(key); - erased = uvars() && uvars()->remove(key); + bool is_exported = false; + if (uvars()) { + if (auto old_flags = uvars()->get_flags(key)) { + is_exported = *old_flags & env_var_t::flag_export; + } + erased = uvars()->remove(key); + } if (erased) { env_universal_barrier(); event_t ev = event_t::variable_event(key); @@ -1377,7 +1388,7 @@ maybe_t env_get(const wcstring &key, env_mode_flags_t mode) { // universal var return that. if (uvars()) { auto var = uvars()->get(key); - if (var && (uvars()->get_export(key) ? search_exported : search_unexported)) { + if (var && (var->exports() ? search_exported : search_unexported)) { return var; } } diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 2797268f9..7388b4825 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -255,17 +255,15 @@ maybe_t env_universal_t::get(const wcstring &name) const { return none(); } -bool env_universal_t::get_export(const wcstring &name) const { - bool result = false; +maybe_t env_universal_t::get_flags(const wcstring &name) const { var_table_t::const_iterator where = vars.find(name); if (where != vars.end()) { - result = where->second.exports(); + return where->second.get_flags(); } - return result; + return none(); } -void env_universal_t::set_internal(const wcstring &key, wcstring_list_t vals, bool exportv, - bool overwrite) { +void env_universal_t::set_internal(const wcstring &key, env_var_t var, bool overwrite) { ASSERT_IS_LOCKED(lock); if (!overwrite && this->modified.find(key) != this->modified.end()) { // This value has been modified and we're not overwriting it. Skip it. @@ -273,9 +271,8 @@ void env_universal_t::set_internal(const wcstring &key, wcstring_list_t vals, bo } env_var_t &entry = vars[key]; - if (entry.exports() != exportv || entry.as_list() != vals) { - entry.set_vals(std::move(vals)); - entry.set_exports(exportv); + if (entry != var) { + entry = var; // If we are overwriting, then this is now modified. if (overwrite) { @@ -284,9 +281,9 @@ void env_universal_t::set_internal(const wcstring &key, wcstring_list_t vals, bo } } -void env_universal_t::set(const wcstring &key, wcstring_list_t vals, bool exportv) { +void env_universal_t::set(const wcstring &key, env_var_t var) { scoped_lock locker(lock); - this->set_internal(key, std::move(vals), exportv, true /* overwrite */); + this->set_internal(key, std::move(var), true /* overwrite */); } bool env_universal_t::remove_internal(const wcstring &key) { diff --git a/src/env_universal_common.h b/src/env_universal_common.h index 7ea32e665..d418db7cc 100644 --- a/src/env_universal_common.h +++ b/src/env_universal_common.h @@ -56,7 +56,7 @@ class env_universal_t { bool load_from_path(const wcstring &path, callback_data_list_t &callbacks); void load_from_fd(int fd, callback_data_list_t &callbacks); - void set_internal(const wcstring &key, wcstring_list_t val, bool exportv, bool overwrite); + void set_internal(const wcstring &key, env_var_t var, bool overwrite); bool remove_internal(const wcstring &name); // Functions concerned with saving. @@ -93,11 +93,11 @@ class env_universal_t { // Get the value of the variable with the specified name. maybe_t get(const wcstring &name) const; - // Returns whether the variable with the given name is exported, or false if it does not exist. - bool get_export(const wcstring &name) const; + // \return flags from the variable with the given name. + maybe_t get_flags(const wcstring &name) const; // Sets a variable. - void set(const wcstring &key, wcstring_list_t val, bool exportv); + void set(const wcstring &key, env_var_t var); // Removes a variable. Returns true if it was found, false if not. bool remove(const wcstring &name); diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index ac5f58a31..1fb3d1666 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2834,7 +2834,7 @@ static int test_universal_helper(int x) { for (int j = 0; j < UVARS_PER_THREAD; j++) { const wcstring key = format_string(L"key_%d_%d", x, j); const wcstring val = format_string(L"val_%d_%d", x, j); - uvars.set(key, {val}, false); + uvars.set(key, env_var_t{val, 0}); bool synced = uvars.sync(callbacks); if (!synced) { err(L"Failed to sync universal variables after modification"); @@ -2966,28 +2966,30 @@ static void test_universal_callbacks() { env_universal_t uvars1(UVARS_TEST_PATH); env_universal_t uvars2(UVARS_TEST_PATH); + env_var_t::env_var_flags_t noflags = 0; + // Put some variables into both. - uvars1.set(L"alpha", {L"1"}, false); - uvars1.set(L"beta", {L"1"}, false); - uvars1.set(L"delta", {L"1"}, false); - uvars1.set(L"epsilon", {L"1"}, false); - uvars1.set(L"lambda", {L"1"}, false); - uvars1.set(L"kappa", {L"1"}, false); - uvars1.set(L"omicron", {L"1"}, false); + uvars1.set(L"alpha", env_var_t{L"1", noflags}); + uvars1.set(L"beta", env_var_t{L"1", noflags}); + uvars1.set(L"delta", env_var_t{L"1", noflags}); + uvars1.set(L"epsilon", env_var_t{L"1", noflags}); + uvars1.set(L"lambda", env_var_t{L"1", noflags}); + uvars1.set(L"kappa", env_var_t{L"1", noflags}); + uvars1.set(L"omicron", env_var_t{L"1", noflags}); uvars1.sync(callbacks); uvars2.sync(callbacks); // Change uvars1. - uvars1.set(L"alpha", {L"2"}, false); // changes value - uvars1.set(L"beta", {L"1"}, true); // changes export + uvars1.set(L"alpha", env_var_t{L"2", noflags}); // changes value + uvars1.set(L"beta", env_var_t{L"1", env_var_t::flag_export}); // changes export uvars1.remove(L"delta"); // erases value - uvars1.set(L"epsilon", {L"1"}, false); // changes nothing + uvars1.set(L"epsilon", env_var_t{L"1", noflags}); // changes nothing uvars1.sync(callbacks); // Change uvars2. It should treat its value as correct and ignore changes from uvars1. - uvars2.set(L"lambda", {L"1"}, false); // same value - uvars2.set(L"kappa", {L"2"}, false); // different value + uvars2.set(L"lambda", {L"1", noflags}); // same value + uvars2.set(L"kappa", {L"2", noflags}); // different value // Now see what uvars2 sees. callbacks.clear(); @@ -3047,7 +3049,7 @@ static void test_universal_ok_to_save() { uvars.sync(cbs); cbs.clear(); do_test(!uvars.is_ok_to_save() && "Should no longer be OK to save"); - uvars.set(L"SOMEVAR", {L"SOMEVALUE"}, false); + uvars.set(L"SOMEVAR", env_var_t{wcstring{L"SOMEVALUE"}, 0}); uvars.sync(cbs); // Ensure file is same. diff --git a/tests/test3.err b/tests/test3.err index e69de29bb..30aa0088a 100644 --- a/tests/test3.err +++ b/tests/test3.err @@ -0,0 +1,3 @@ + +#################### +# Path universal variables diff --git a/tests/test3.in b/tests/test3.in index 5cad24e86..e4d8c2b42 100644 --- a/tests/test3.in +++ b/tests/test3.in @@ -311,4 +311,18 @@ set -x DONT_ESCAPE_COLONS 1: 2: :3: ; env | grep '^DONT_ESCAPE_COLONS=' set -x DONT_ESCAPE_SPACES '1 ' '2 ' ' 3 ' 4 ; env | grep '^DONT_ESCAPE_SPACES=' set -x DONT_ESCAPE_COLONS_PATH 1: 2: :3: ; env | grep '^DONT_ESCAPE_COLONS_PATH=' +logmsg Path universal variables +set __fish_test_path_not a b c +set __fish_test_PATH 1 2 3 +echo "$__fish_test_path_not $__fish_test_PATH" +set --unpath __fish_test_PATH $__fish_test_PATH +echo "$__fish_test_path_not $__fish_test_PATH" +set --path __fish_test_path_not $__fish_test_path_not +echo "$__fish_test_path_not $__fish_test_PATH" +set --path __fish_test_PATH $__fish_test_PATH +echo "$__fish_test_path_not $__fish_test_PATH" + + + + true diff --git a/tests/test3.out b/tests/test3.out index c466c8f6b..9c8bfd2da 100644 --- a/tests/test3.out +++ b/tests/test3.out @@ -46,3 +46,10 @@ MANPATH=man1:man2:man3 DONT_ESCAPE_COLONS=1: 2: :3: DONT_ESCAPE_SPACES=1 2 3 4 DONT_ESCAPE_COLONS_PATH=1::2:::3: + +#################### +# Path universal variables +a b c 1:2:3 +a b c 1 2 3 +a:b:c 1 2 3 +a:b:c 1:2:3