[WIP] XDG support #3198
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -301,6 +301,15 @@ static char *(p_cot_values[]) = {"menu", "menuone", "longest", "preview", | |
| # include "option.c.generated.h" | ||
| #endif | ||
|
|
||
| static void set_runtimepath_default(void) | ||
| { | ||
| garray_T rtp_ga; | ||
| ga_init(&rtp_ga, (int)sizeof(const char *), 1); | ||
| GA_APPEND(const char *, &rtp_ga, get_user_conf_dir()); | ||
| GA_APPEND(const char *, &rtp_ga, concat_fnames(get_user_conf_dir(), "after", true)); | ||
| set_string_default("runtimepath", ga_concat_strings(&rtp_ga)); | ||
|
There was a problem hiding this comment. The memory allocated by There was a problem hiding this comment. Would you suggest that I precomute the length of the rtp string like this? There was a problem hiding this comment. The safest bet for allocating path names is just I just realized another issue: It looks like the resulting string will be char* conf_dir = get_user_conf_dir();
set_string_default("runtimepath", concat_fnames(conf_dir, "after", true);
xfree(conf_dir);There was a problem hiding this comment. Relying on There was a problem hiding this comment. Unfortunately (I guess), we have a lot of code that check paths are less than the maximum, so this convention wouldn't be easy to remove from the codebase, even if it's wrong. For the moment, I think it would be correct to just say "we don't support unreasonably large paths," and if supporting them means unleashing such inefficient code such as in the linked article, I don't see any benefit. Also, I double checked: I should have said There was a problem hiding this comment. @splinterofchaos Why not simply use my code with minimal modifications (to use different functions to get XDG_* variables)? I see that this variant uses only XDG_CONFIG_HOME which is not enough. There was a problem hiding this comment. (Note: do not use the commit you referenced, use the head of There was a problem hiding this comment. @ZyX-I “53. So many good ideas are never heard from again once they embark in a voyage on the semantic gulf.” — Alan J. Perlis |
||
| } | ||
|
|
||
| /* | ||
| * Initialize the options, first part. | ||
| * | ||
|
|
@@ -437,6 +446,12 @@ void set_init_1(void) | |
| "system('lpr' . (&printdevice == '' ? '' : ' -P' . &printdevice) . ' ' . v:fname_in) . delete(v:fname_in) + v:shell_error" | ||
| ); | ||
|
|
||
| set_string_default("viewdir", (char_u *)get_from_user_data("view")); | ||
| set_string_default("backupdir", (char_u *)get_from_user_data("backup")); | ||
| set_string_default("directory", (char_u *)get_from_user_data("swap")); | ||
| set_string_default("undodir", (char_u *)get_from_user_data("undo")); | ||
| set_runtimepath_default(); | ||
|
|
||
| /* | ||
| * Set all the options (except the terminal options) to their default | ||
| * value. Also set the global value for local options. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -176,7 +176,7 @@ return { | |
| vi_def=true, | ||
| expand=true, | ||
| varname='p_bdir', | ||
| defaults={if_true={vi=''}} | ||
| }, | ||
| { | ||
| full_name='backupext', abbreviation='bex', | ||
|
|
@@ -619,7 +619,7 @@ return { | |
| vi_def=true, | ||
| expand=true, | ||
| varname='p_dir', | ||
| defaults={if_true={vi=''}} | ||
| }, | ||
| { | ||
| full_name='display', abbreviation='dy', | ||
|
|
@@ -1908,7 +1908,7 @@ return { | |
| vi_def=true, | ||
| expand=true, | ||
| varname='p_rtp', | ||
| defaults={if_true={vi=''}} | ||
| }, | ||
| { | ||
| full_name='scroll', abbreviation='scr', | ||
|
|
@@ -2508,7 +2508,7 @@ return { | |
| vi_def=true, | ||
| expand=true, | ||
| varname='p_udir', | ||
| defaults={if_true={vi=''}} | ||
| }, | ||
| { | ||
| full_name='undofile', abbreviation='udf', | ||
|
|
@@ -2569,7 +2569,7 @@ return { | |
| vi_def=true, | ||
| expand=true, | ||
| varname='p_vdir', | ||
| defaults={if_true={vi=''}} | ||
|
There was a problem hiding this comment. I see that only Don’t forget that for default &runtimepath value you may take code from my diff --git a/src/nvim/option.c b/src/nvim/option.c
index 9a375c0..f9b8924 100644
--- a/src/nvim/option.c
+++ b/src/nvim/option.c
@@ -301,6 +301,148 @@ static char *(p_cot_values[]) = {"menu", "menuone", "longest", "preview",
# include "option.c.generated.h"
#endif
+/// Count commas in the given string
+static size_t count_commas(const char *const s, size_t len)
+ FUNC_ATTR_NONNULL_ALL FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT
+{
+ size_t ret = 0;
+ for (size_t i = 0; i < len; i++) {
+ if (s[i] == ',') {
+ ret++;
+ }
+ }
+ return ret;
+}
+
+/// Append string with escaped commas
+static char *strcpy_comma_escaped(char *dest, const char *src, const size_t len)
+ FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT
+{
+ size_t shift = 0;
+ for (size_t i = 0; i < len; i++) {
+ if (src[i] == ',') {
+ dest[i + shift++] = '\\';
+ }
+ dest[i + shift] = src[i];
+ }
+ return &dest[len + shift];
+}
+
+/// Set &runtimepath to default value
+static void set_runtimepath_default(void)
+{
+ size_t rtp_size = 0;
+ char *const data_home = vim_getenv("XDG_DATA_HOME");
+ char *const config_home = vim_getenv("XDG_CONFIG_HOME");
+ char *const vimruntime = vim_getenv("VIMRUNTIME");
+ char *const data_dirs = vim_getenv("XDG_DATA_DIRS");
+ char *const config_dirs = vim_getenv("XDG_CONFIG_DIRS");
+#define NVIM_SIZE (sizeof("/nvim") - 1)
+#define SITE_SIZE (sizeof("/site") - 1)
+#define AFTER_SIZE (sizeof("/after") - 1)
+ size_t data_len;
+ size_t config_len;
+ size_t vimruntime_len;
+ if (data_home != NULL) {
+ data_len = strlen(data_home);
+ rtp_size += ((data_len + count_commas(data_home, data_len)
+ + NVIM_SIZE + SITE_SIZE) * 2 + AFTER_SIZE) + 2;
+ }
+ if (config_home != NULL) {
+ config_len = strlen(config_home);
+ rtp_size += ((config_len + count_commas(config_home, config_len)
+ + NVIM_SIZE) * 2 + AFTER_SIZE) + 2;
+ }
+ if (vimruntime != NULL) {
+ vimruntime_len = strlen(vimruntime);
+ rtp_size += vimruntime_len + count_commas(vimruntime, vimruntime_len) + 1;
+ }
+#define COMPUTE_COLON_LEN(rtp_size, additional_size, val) \
+ do { \
+ if (val != NULL) { \
+ const void *iter = NULL; \
+ do { \
+ size_t dir_len; \
+ const char *dir; \
+ iter = vim_colon_env_iter(val, iter, &dir, &dir_len); \
+ if (dir != NULL && dir_len > 0) { \
+ rtp_size += ((dir_len + count_commas(dir, dir_len) \
+ + NVIM_SIZE + additional_size) * 2 \
+ + AFTER_SIZE) + 2; \
+ } \
+ } while (iter != NULL); \
+ } \
+ } while (0)
+ COMPUTE_COLON_LEN(rtp_size, SITE_SIZE, data_dirs);
+ COMPUTE_COLON_LEN(rtp_size, 0, config_dirs);
+#undef COMPUTE_COLON_LEN
+ if (rtp_size == 0) {
+ return;
+ }
+ // All additions were including comma.
+ rtp_size--;
+ char *const rtp = xmallocz(rtp_size);
+ char *rtp_cur = rtp;
+#define ADD_STRING(tgt, src, len) \
+ tgt = strcpy_comma_escaped(tgt, src, len)
+#define ADD_STATIC_STRING(tgt, src) \
+ do { memmove(tgt, src, sizeof(src) - 1); tgt += sizeof(src) - 1; } while (0)
+#define ADD_COLON_DIRS(tgt, val, suffix, revsuffix) \
+ do { \
+ if (val != NULL) { \
+ const void *iter = NULL; \
+ do { \
+ size_t dir_len; \
+ const char *dir; \
+ iter = vim_colon_env_iter##revsuffix(val, iter, &dir, &dir_len); \
+ if (dir != NULL && dir_len > 0) { \
+ ADD_STRING(rtp_cur, dir, dir_len); \
+ ADD_STATIC_STRING(rtp_cur, "/nvim" suffix ","); \
+ } \
+ } while (iter != NULL); \
+ } \
+ } while (0)
+ if (config_home != NULL) {
+ ADD_STRING(rtp_cur, config_home, config_len);
+ ADD_STATIC_STRING(rtp_cur, "/nvim,");
+ }
+ ADD_COLON_DIRS(rtp_cur, config_dirs, "", );
+ if (data_home != NULL) {
+ ADD_STRING(rtp_cur, data_home, data_len);
+ ADD_STATIC_STRING(rtp_cur, "/nvim/site,");
+ }
+ ADD_COLON_DIRS(rtp_cur, data_dirs, "/site", );
+ if (vimruntime != NULL) {
+ ADD_STRING(rtp_cur, vimruntime, vimruntime_len);
+ *rtp_cur++ = ',';
+ }
+ ADD_COLON_DIRS(rtp_cur, data_dirs, "/site/after", _rev);
+ if (data_home != NULL) {
+ ADD_STRING(rtp_cur, data_home, data_len);
+ ADD_STATIC_STRING(rtp_cur, "/nvim/site/after,");
+ }
+ ADD_COLON_DIRS(rtp_cur, config_dirs, "/after", _rev);
+ if (config_home != NULL) {
+ ADD_STRING(rtp_cur, config_home, config_len);
+ ADD_STATIC_STRING(rtp_cur, "/nvim/after");
+ } else {
+ // Strip trailing comma.
+ rtp[rtp_size] = NUL;
+ }
+#undef ADD_COLON_DIRS
+#undef ADD_STATIC_STRING
+#undef ADD_STRING
+#undef NVIM_SIZE
+#undef SITE_SIZE
+#undef AFTER_SIZE
+ set_string_default("runtimepath", (char_u *)rtp);
+ xfree(data_dirs);
+ xfree(config_dirs);
+ xfree(data_home);
+ xfree(config_home);
+ xfree(vimruntime);
+}
+
/*
* Initialize the options, first part.
*
@@ -437,6 +579,10 @@ void set_init_1(void)
"system('lpr' . (&printdevice == '' ? '' : ' -P' . &printdevice) . ' ' . v:fname_in) . delete(v:fname_in) + v:shell_error"
);
+ // Set default for &runtimepath. All necessary expansions are performed in
+ // this function.
+ set_runtimepath_default();
+
/*
* Set all the options (except the terminal options) to their default
* value. Also set the global value for local options.
diff --git a/src/nvim/os/env.c b/src/nvim/os/env.c
index 7be8a86..e47a63b 100644
--- a/src/nvim/os/env.c
+++ b/src/nvim/os/env.c
@@ -415,6 +415,74 @@ static char *remove_tail(char *p, char *pend, char *name)
return pend;
}
+/// Iterate over colon-separated list
+///
+/// @note Environment variables must not be modified during iteration.
+///
+/// @param[in] val Value of the environment variable to iterate over.
+/// @param[in] iter Pointer used for iteration. Must be NULL on first
+/// iteration.
+/// @param[out] dir Location where pointer to the start of the current
+/// directory name should be saved. May be set to NULL.
+/// @param[out] len Location where current directory length should be saved.
+///
+/// @return Next iter argument value or NULL when iteration should stop.
+const void *vim_colon_env_iter(const char *const val,
+ const void *const iter,
+ const char **const dir,
+ size_t *const len)
+ FUNC_ATTR_NONNULL_ARG(1,3,4) FUNC_ATTR_WARN_UNUSED_RESULT
+{
+ const char *varval = (const char *) iter;
+ if (varval == NULL) {
+ varval = val;
+ }
+ *dir = varval;
+ const char *const dirend = strchr(varval, ':');
+ if (dirend == NULL) {
+ *len = strlen(varval);
+ return NULL;
+ } else {
+ *len = (size_t) (dirend - varval);
+ return dirend + 1;
+ }
+}
+
+/// Iterate over colon-separated list in reverse order
+///
+/// @note Environment variables must not be modified during iteration.
+///
+/// @param[in] val Value of the environment variable to iterate over.
+/// @param[in] iter Pointer used for iteration. Must be NULL on first
+/// iteration.
+/// @param[out] dir Location where pointer to the start of the current
+/// directory name should be saved. May be set to NULL.
+/// @param[out] len Location where current directory length should be saved.
+///
+/// @return Next iter argument value or NULL when iteration should stop.
+const void *vim_colon_env_iter_rev(const char *const val,
+ const void *const iter,
+ const char **const dir,
+ size_t *const len)
+ FUNC_ATTR_NONNULL_ARG(1,3,4) FUNC_ATTR_WARN_UNUSED_RESULT
+{
+ const char *varend = (const char *) iter;
+ if (varend == NULL) {
+ varend = val + strlen(val) - 1;
+ }
+ const size_t varlen = (size_t) (varend - val) + 1;
+ const char *const colon = xmemrchr(val, ':', varlen);
+ if (colon == NULL) {
+ *len = varlen;
+ *dir = val;
+ return NULL;
+ } else {
+ *dir = colon + 1;
+ *len = (size_t) (varend - colon);
+ return colon - 1;
+ }
+}
+
/// Vim's version of getenv().
/// Special handling of $HOME, $VIM and $VIMRUNTIME, allowing the user to
/// override the vim runtime directory at runtime. Also does ACP to 'enc', just replace There was a problem hiding this comment. Yup, I plan to do all of it. Just want to make sure that everyone else agrees with this approach first. |
||
| }, | ||
| { | ||
| full_name='viewoptions', abbreviation='vop', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| #include "nvim/os/os.h" | ||
|
There was a problem hiding this comment. Perhaps a comment up here mentioning |
||
| #include "nvim/strings.h" | ||
| #include "nvim/path.h" | ||
| #include "nvim/garray.h" | ||
|
|
||
| typedef enum { | ||
| kXDGConfigHome, | ||
| kXDGDataHome, | ||
| kXDGCacheHome, | ||
| kXDGRuntimeDir, | ||
| kXDGConfigDirs, | ||
| kXDGDataDirs, | ||
| } XDGDirType; | ||
|
|
||
| static const char *xdg_env_vars[] = { | ||
| [kXDGConfigHome] = "XDG_CONFIG_HOME", | ||
| [kXDGDataHome] = "XDG_DATA_HOME", | ||
| [kXDGCacheHome] = "XDG_CACHE_HOME", | ||
| [kXDGRuntimeDir] = "XDG_RUNTIME_DIR", | ||
| [kXDGConfigDirs] = "XDG_CONFIG_DIRS", | ||
| [kXDGDataDirs] = "XDG_DATA_DIRS", | ||
| }; | ||
|
|
||
|
There was a problem hiding this comment. @ZyX-I I changed the enum field names as per the style guide. There was a problem hiding this comment. Common prefix for all enum values is just There was a problem hiding this comment. To be clear, these should be renamed like: |
||
| static const char *const xdg_defaults[] = { | ||
| // Windows, Apple stuff are just shims right now | ||
| #ifdef WIN32 | ||
| // Windows | ||
| #elif APPLE | ||
| // Apple (this includes iOS, which we might need to handle differently) | ||
| [kXDGConfigHome] = "~/Library/Preferences", | ||
| [kXDGDataHome] = "~/Library/Application Support", | ||
| [kXDGCacheHome] = "~/Library/Caches", | ||
| [kXDGRuntimeDir] = "~/Library/Application Support", | ||
| [kXDGConfigDirs] = "/Library/Application Support", | ||
| [kXDGDataDirs] = "/Library/Application Support", | ||
| #else | ||
| // Linux, BSD, CYGWIN | ||
| [kXDGConfigHome] = "~/.config", | ||
| [kXDGDataHome] = "~/.local/share", | ||
| [kXDGCacheHome] = "~/.cache", | ||
| [kXDGRuntimeDir] = "", | ||
| [kXDGConfigDirs] = "/etc/xdg/", | ||
| [kXDGDataDirs] = "/usr/local/share/:/usr/share/", | ||
| }; | ||
| #endif | ||
|
There was a problem hiding this comment. @pyrohh Does this look good? There was a problem hiding this comment. I prefer explicit defines (as opposed to else), but in this case why not. There was a problem hiding this comment. @jck Looks fine to me, no comment on the actual defines though. I'm not sure if it should instead be |
||
|
|
||
| static const char *get_xdg(XDGDirType idx) | ||
| { | ||
| const char *env = xdg_env_vars[idx]; | ||
| const char *fallback = xdg_defaults[idx]; | ||
|
|
||
| const char *ret = os_getenv(env); | ||
| if (!ret && fallback) { | ||
| ret = (const char *)expand_env_save((char_u *)fallback); | ||
| } | ||
|
|
||
| return ret; | ||
|
There was a problem hiding this comment. This returns statically allocated memory when const char *ret = os_getenv(env);
if (ret) {
ret = xstrdup(ret);
} else if (fallback) {
// ... same as before
}There was a problem hiding this comment. ^ This. This should fix the leak ASAN is complainign about. |
||
| } | ||
|
|
||
| static const char *get_xdg_home(XDGDirType idx) | ||
| { | ||
| const char *dir = get_xdg(idx); | ||
| if (dir) { | ||
| dir = (const char *)concat_fnames(dir, "nvim", true); | ||
|
There was a problem hiding this comment. why not just There was a problem hiding this comment. Because xdg_runtime_dir has no default value. When we reach a consensus on how we handle that, I'll change this. There was a problem hiding this comment. There is a leak here, according to ASAN |
||
| } | ||
| return dir; | ||
| } | ||
|
|
||
| static void create_dir(const char *dir, int mode, const char *suffix) | ||
| { | ||
| char *failed; | ||
| if (!os_mkdir_recurse(dir, mode, &failed)) { | ||
| // TODO: Create a folder in $TMPDIR instead | ||
|
There was a problem hiding this comment. I know this isn't fully implemented, but it might be better off in There was a problem hiding this comment. Maybe, but right now what I have in mind is that this function will be tailored to create data,cache and runtime dirs. Let us revisit this later. There was a problem hiding this comment. Sounds good, I don't want to bog this down in minutiae. |
||
| DLOG("Create dir failed"); | ||
| } | ||
| } | ||
|
|
||
| const char *get_user_conf_dir(void) | ||
| { | ||
| return get_xdg_home(kXDGConfigHome); | ||
| } | ||
|
|
||
| const char *get_from_user_conf(const char * fname) | ||
|
|
||
| { | ||
| return (const char *)concat_fnames(get_user_conf_dir(), fname, true); | ||
| } | ||
|
|
||
| const char *get_user_data_dir(void) | ||
| { | ||
| return get_xdg_home(kXDGDataHome); | ||
| } | ||
|
|
||
| const char *get_from_user_data(const char * fname) | ||
| { | ||
| const char *dir = (const char *)concat_fnames(get_user_data_dir(), fname, true); | ||
|
There was a problem hiding this comment. clint says this line is too long There was a problem hiding this comment. Also, there is a leak here. There was a problem hiding this comment. Speaking of that, is there any static analysis planned ? It could be a nice addition. There was a problem hiding this comment. We already have static analysis: https://neovim.io/doc/reports/clang/ However coverity has been broken for weeks. |
||
| if (!os_isdir((char_u *)dir)) { | ||
| create_dir(dir, 0755, fname); | ||
| } | ||
| return dir; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,12 +31,6 @@ | |
| #ifndef USR_EXRC_FILE | ||
| # define USR_EXRC_FILE "~/.exrc" | ||
| #endif | ||
| #ifndef EXRC_FILE | ||
| # define EXRC_FILE ".exrc" | ||
|
There was a problem hiding this comment. Does this mean that users will be forced to use the new location? Isn't it better to use the old files in case it is not found in the XDG dirs? cc @justinmk There was a problem hiding this comment. In #78 @ZyX-I made the case that there is no reason for us to support both old and new config locations. It bothers me because it means the default location is different on each system (this makes support more difficult, and requires extra setup to share configs across systems using GitHub etc). I would be less bothered by it if we defaulted to There was a problem hiding this comment. It's not your place to question it really. These are the standards set forth by the operating systems you target. I'll quote some of Raymond Chen's ironic maxims about his frustration when dealing with software during the MS-DOS to Windows 95 transition: Please don't keep repeating the same mistakes. As for support, different locations is trivial to deal with, please don't use it as an excuse to do the wrong thing. @tarruda This software is pre-1.0 release (assuming you're following semver), there is no reason to support legacy. Just do the right thing now instead. There was a problem hiding this comment. I'm with @Earnestly (and @ZyX-I) on this one. There was a problem hiding this comment. @Earnestly Where does the standard specify the default locations for OSX and Windows? So far only source examples like https://github.com/ActiveState/appdirs/blob/master/appdirs.py have been offered, which indicate "best guesses" about default locations, there is no reference to a specification. There was a problem hiding this comment. @kopischke I'm not a OSX user (never really used it, actually), so I was just being conservative ;) There was a problem hiding this comment. @justinmk Taking a look at the basedir spec again, most of the http://standards.freedesktop.org/basedir-spec/latest/ar01s03.html There was a problem hiding this comment. @justinmk Windows is out of scope of XDG Basedir, follow Windows' standards for Windows instead. For example, Vala's What you want to do about Windows however is up to you, There was a problem hiding this comment. @fmoralesc nothing wrong with that, especially as OS X’ “almost, but not quite, Unix” system does need special treatment in many areas. The XDG spec as applied to CLI applications is not one of them, however, so let’s count our blessings and limit special treatment to The Odd Man Out™, aka Windows :). |
||
| #endif | ||
|
|
@@ -47,27 +41,4 @@ | |
| # define VIMINFO_FILE "~/.nviminfo" | ||
| #endif | ||
|
|
||
| #endif // NVIM_OS_UNIX_DEFS_H | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above comment is pretty outdated.