Skip to content

Adds path expansion to the config file parser.#803

Closed
proxypoke wants to merge 2 commits intoredis:unstablefrom
proxypoke:config_path_expansion
Closed

Adds path expansion to the config file parser.#803
proxypoke wants to merge 2 commits intoredis:unstablefrom
proxypoke:config_path_expansion

Conversation

@proxypoke
Copy link

It's now possible to use ~ or environmental variables like $HOME in
the config file (where appropriate).

slowpoke added 2 commits December 1, 2012 12:12
It's now possible to use ~ or environmental variables like $HOME in
the config file (where appropriate).
Copy link
Contributor

@yoav-steinberg yoav-steinberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks decent.
I'd like feedback from more people too see if this feature makes sense. @yossigo?
Requires a rebase and handling of buffer size checks..

break;
}
if (error == 0) {
strcpy(path, exp.we_wordv[0]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we know there's room in the destination?

@yoav-steinberg yoav-steinberg added state:needs-coding the solution is trivial, some coding (or additional coding) is needed state:needs-review the PR requires additional review labels May 20, 2021
@yossigo
Copy link
Collaborator

yossigo commented May 22, 2021

@yoav-steinberg Do you see a lot of value in this?
I see a few issues:

  • Will CONFIG REWRITE use the expanded values? Otherwise we need to retain the originals.
  • Will CONFIG SET expand values as well? If not, a sequence of CONFIG SET followed by CONFIG REWRITE and a restart may lead to inconsistent/unexpected configuration.

@yoav-steinberg
Copy link
Contributor

yoav-steinberg commented May 25, 2021

@yossigo You're right, config rewrite does complicate things. I don't see a lot of value here. Maybe some. I suggest closing this because of the added complication you mentioned and the limited value. Agree?

@yossigo
Copy link
Collaborator

yossigo commented May 30, 2021

@yoav-steinberg I agree

@yoav-steinberg
Copy link
Contributor

@proxypoke thanks. Although this is a nice Idea the PR it too limited and complicates config rewrite. We decide the value is too marginal at this point. Closing.

sundb pushed a commit to sundb/redis that referenced this pull request Jul 28, 2025
Primary side: Remove read handler upon RDB connection close.

At this stage we do not expect any writed form that connection
so it should be safe to remove the read handler. Otherwise the
read handler will keep printing the `Client closed connection`
logs, see handleReadResult.

Signed-off-by: naglera <anagler123@gmail.com>
minchopaskal pushed a commit to minchopaskal/redis that referenced this pull request Oct 16, 2025
XXH_ASSUME macro using `__builtin_assume` if supported (clang only fo…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state:needs-coding the solution is trivial, some coding (or additional coding) is needed state:needs-review the PR requires additional review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants