Closed hongzhidao closed 4 months ago
Remember you should always provide at the very least a brief description of the PR...
OK. So before looking at the code there are a couple of issues.
1) Again these are using your @gmail address. 2) Commit message line length.
Fixing (1) will be tricky for all but the HEAD commit. For this I'd use git rebase -i
...
For (2) NJS: variable access support.
looks OK, however the others look well, a picture is worth a thousand words
So this is a standard 80 column terminal. We can see the first message is wrapped at around 72 characters, this is good, as you can see, git shows the commit message indented by 4 and so wrapping at 72 chars gives 4 characters of padding each side of the message.
NOTE: As you've seen we don't wrap things like URIs, command and log output etc...
Again this would be fixed via git rebase -i
(there may be other ways, but that's what I would consider the standard/natural way to do it).
It would basically go like
$ git rebase -i HEAD~3
It will open your editor with the following
pick 44aac2b8 Var: generalized nxt_var_cache_value().
pick 2286f364 Var: introduced nxt_var_get().
pick 9e510eb5 NJS: variable access support.
# Rebase d9f5f1fb..9e510eb5 onto d9f5f1fb (3 commands)
#
# Commands:
# p, pick <commit> = use commit
# r, reword <commit> = use commit, but edit the commit message
# e, edit <commit> = use commit, but stop for amending
# s, squash <commit> = use commit, but meld into previous commit
# f, fixup [-C | -c] <commit> = like "squash" but keep only the previous
# commit's log message, unless -C is used, in which case
# keep only this commit's message; -c is same as -C but
# opens the editor
# x, exec <command> = run command (the rest of the line) using shell
# b, break = stop here (continue rebase later with 'git rebase --continue')
# d, drop <commit> = remove commit
# l, label <label> = label current HEAD with a name
# t, reset <label> = reset HEAD to a label
# m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]
# create a merge commit using the original merge commit's
# message (or the oneline, if no original merge commit was
# specified); use -c <commit> to reword the commit message
# u, update-ref <ref> = track a placeholder for the <ref> to be updated
# to this position in the new commits. The <ref> is
# updated at the end of the rebase
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.
#
Now change those 'picks's to 'e's
e 44aac2b8 Var: generalized nxt_var_cache_value().
e 2286f364 Var: introduced nxt_var_get().
e 9e510eb5 NJS: variable access support.
...
Save and exit, you will then be dropped back to your shell.
It will start off with Var: generalized nxt_var_cache_value().
We can now do
$ git commit --amend --author="Zhidao HONG <z.hong@f5.com>"
Fix the commit message, save and exit
Now you can move onto the next commit
$ git rebase --continue
$ git commit --amend --author="Zhidao HONG <z.hong@f5.com>"
Fix the commit message, save and exit
Again, move onto the next (and final) commit
$ git rebase --continue
$ git commit --amend --no-edit --author="Zhidao HONG <z.hong@f5.com>"
Note, no real need to edit the last message.
Now to finish
$ git rebase --continue
Successfully rebased and updated refs/heads/hongzhidao-njs-var.
You can check the result with
$ git log --pretty=fuller
Then push this back to the PR
$ git push -f <remote> <branch>
@ac000 Thanks, updated. And I'm not sure if I understand the message length correctly. I tried to use 72 chars as much as possible.
@ac000 Thanks, updated. And I'm not sure if I understand the message length correctly. I tried to use 72 chars as much as possible.
They look OK now, cheers.
I will probably have some questions about the changes themselves.
Sure, we can call them Unit variables but not any JS variables. Unit native variables: https://unit.nginx.org/configuration/#variables Btw, in the template string, users can use any JS variables/functions/objects as well, they belong to the JS engine. For example:
{
"pass": "`routes${uri + Math.random() * 100}`"
}
Sure, we can call them Unit variables but not any JS variables. Unit native variables: https://unit.nginx.org/configuration/#variables Btw, in the template string, users can use any JS variables/functions/objects as well, they belong to the JS engine. For example:
{ "pass": "`routes${uri + Math.random() * 100}`" }
Thanks. It's probably worth clarifying that in the commit message.
Btw, no need to terminate the subject lines with a full stop any more...
Rebased and reworded the subjects.
commit a425506b178c7752933015d26deb43649947b6f1
Author: Zhidao HONG <z.hong@f5.com>
Date: Wed Jan 31 14:08:57 2024 +0800
Var: Refactored nxt_var_ref_get()
delay | 1135 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
src/nxt_var.c | 14 +-
2 files changed, 1143 insertions(+), 6 deletions(-)
create mode 100644 delay
Is still adding this delay file...
diff --git a/delay b/delay
new file mode 100644
index 00000000..85f32ec7
--- /dev/null
+++ b/delay
commit a425506b178c7752933015d26deb43649947b6f1 Author: Zhidao HONG <z.hong@f5.com> Date: Wed Jan 31 14:08:57 2024 +0800 Var: Refactored nxt_var_ref_get() delay | 1135 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/nxt_var.c | 14 +- 2 files changed, 1143 insertions(+), 6 deletions(-) create mode 100644 delay
Is still adding this delay file...
diff --git a/delay b/delay new file mode 100644 index 00000000..85f32ec7 --- /dev/null +++ b/delay
It's strange why there is such file left, I removed it, thanks.
Yeah, that looks better!
Thanks for this great feature!
However, it seems quite buggy, it's only possible to use a single variable from vars
since all the latter ones will get the same value as the 1st one. I imagine this has something to do with caching.
I have created an issue on this: #1169.
This PR is to introduce the feature of njs variable access. Here are 3 commits:
Note, in the configuration, option values like
some $uri text
, and the parturi
which is a variable name turned into an internal reference index for performance optimization. For the new feature, users can write JS code like${uri}
in the configuration, the script is parsed with njs library, so it can only access variable with a string nameuri
.So we need to support two ways of accessing variables: ref index and string name.