langgenius
http节点，报错422. 变量如果是直接写没事，如果引用变量就422
Self Checks
- [x] This is only for bug report, if you would like to ask a question, please head to Discussions.
- [x] I have searched for existing issues search for existing issues, including closed ones.
- [x] I confirm that I am using English to submit this report (我已阅读并同意 Language Policy).
- [x] [FOR CHINESE USERS] 请务必使用英文提交 Issue，否则会被关闭。谢谢！:)
- [x] Please do not modify this template :) and fill in all the required fields.
Dify version
1.0
Cloud or Self Hosted
Self Hosted (Source)
Steps to reproduce
[
](url)
✔️ Expected Behavior
No response
❌ Actual Behavior
No response
@justinmk Thanks for noticing my contribution here :-) May I ask, what does the "needs:response" label stand for? I couldn't find a question (this PR, referred issue, etc.) but I wonder if it may have potential further meaning? Thx in advance.
@justinmk Thanks very much for the review.
I had a question while working on this code. Isn't it an "overkill" to add the amount of (sensitive) changes to fix an --after all-- small issue? In case your judgement is to rather have this change, I'm more than happy to polish it to perfect :-) (I'm actively working on tracking down the issue of the windows pipeline.)
Isn't it an "overkill" to add the amount of (sensitive) changes to fix an --after all-- small issue?
I wouldn't say https://github.com/neovim/neovim/issues/32550 is a small issue. It's pointing to a bug I had worried about, and could affect other libraries loaded by nvim.
envmap was never really a good solution, and I'm glad you are finally getting rid of it :) It's true that this work is delicate and will require careful review, but we don't really have an alternative.
I think memfree can be avoided in some of these cases. Also, if there are many cases that only want to quickly check the result of os_getenv, we could add os_getenv_noalloc which writes to NameBuff (and returns a pointer to it), to save the caller the trouble of freeing the result.
@justinmk Thank you very much for additional suggestions and most importantly the confirmation of the value of this work.
I'm very glad it's useful :-)
Also thanks for the foreseen particularly careful review -- keeps me re-assured :-)
I carry on accordingly, evaluating cases for a potential os_getenv_noalloc. Thx very much :-)
Simple changes are applied, pipelines are :green_circle: :tada:
Making an attempt for os_getenv_noalloc in an upcoming commit.
@zeertzjq Thanks for your response. os_env_exists uses cool trick :-)
But I'm a bit confused, how to benefit from a similar approach here. Would you suggest to use uv_os_getenv() to retrieve the value to NameBuff directly? (I apologize if I'm overlooking something obvious...)
No, I mean there are some cases where the caller only needs to check if an environment variable is empty. In such cases an allocation can be avoided, as they don't actually need to use the string, and can use a boolean value instead.
I see your point, but I can't map that usage to the case in question. The actual value of NVIM_APPNAME seems to be used in all cases (potentially slightly transformed).
Case 1: Direct return value
const char *appname = get_appname(false)
Case 2: Using the (slightly transformed) value loaded to NameBuff (src/nvim/os/stdpaths.c)
(void *)get_appname(true); // Loading NVIM_APPNAME to NameBuff
int r = snprintf(..., name ? name : NameBuff, ...)
I'm not sure how an existence check may come useful :-( (Pls let me know if I'm on a wrong track.)
@zeertzjq Thanks for the clarification, now your message is clear. (The context was confusing, I thought the suggestion was targeting this issue.)
The existence check only appears on a few occasions, yet I agree that an os_env_defined() (similar to os_env_exists()) could increase clarity and avoid memory allocation. I'm adding it with in the next round. Thanks.
In order to keep individually fully functional stages separate, I've left this PR as a fully "polished" version of os_getenv() usage on each occasion.
I'm introducting the code with os_env_defined() (suggested by @zeertzjq ) and os_getenv_noalloc() (suggested by @justinmk ) in PR II., demonstrating the transition the code discussed here. (Of course, to be added here at the end, and merged to master as a single change.)
Added changes from PR II., including fixes for comments:
- @zeertzjq:
- https://github.com/juditnovak/neovim/pull/1#discussion_r1996402380
- https://github.com/juditnovak/neovim/pull/1#discussion_r1996402865
- @justinmk:
- https://github.com/juditnovak/neovim/pull/1#discussion_r1996825716
NOTE: All commits will be squashed into the first commit (that has the correct, linted structure) once the review process is over. Until then lintcommit pipeline errors could be ignored.
(Once final, I'd squash all commits into the 1st one, that already has the correct commit message format, so it would be prepared for merge to master.)
Could you pls suggest if the test failure I see after the latest rebase is a known flaky behaviour of the tests? I tried to reproduce both locally and on GH pipelines. Thanks.
@justinmk @zeertzjq Are there any outstanding actions on my side? Please let me know in case I may be missing something... (No pressure in case it's for you to take action. I just wanted to double-check if I may be blocking progress :-) )
Maybe we should also have a test for vim.uv.os_unsetenv (the repro steps from https://github.com/neovim/neovim/issues/32550 ) in test/functional/ (not test/unit/)? May need a new test/functional/core/env_spec.lua file.
@justinmk Functional test added as requested (with reference to the original issue). Pls check.
Once all approved, I'm squashing the new commits into 1st one (and of course do a rebase at that point as well :-) )
Once all approved, I'm squashing the new commits into 1st one (and of course do a rebase at that point as well :-) )
no need since you merged master here. we can squash at merge-time.
thanks!
@justinmk I apologize :facepalm:
Added now in the corresponding, last commit. (Previous content of this commit, i.e. variable rename on a unittest was added to the rest of the requested fixes.)