Add "if" option to the "match" object.

[hongzhidao]

Two things:

1. restrict variables to only support synchronous way. Initially, variable query was designed to accomodate both synchronous and asynchronous operations. However, upon consideration of actual requirements, we recognized that asynchronous support was not needed. This refactoring is intended to simplify the usage of variable queries and prepare for the subsequent feature of variable updating.

2. Add if option to match object to make matching scripting. For example:

   {
   "listeners": {
       "*:8080": {
           "pass": "routes"
       }
   },
   "routes": [
       {
           "match": {
               "if": "!`${headers['User-Agent'].split('/')[0] == 'curl'}`"
           },
           "action": {
               "return": 204
           }
       }
   ]
   }

[ac000]

Couple of quick questions.

What variables are we talking about here?

Is the afore mentioned discussion available anywhere?

[hongzhidao]

What variables are we talking about here?

Variable-related APIs, correspond to nxt_tstr_{api}.

Is the afore mentioned discussion available anywhere?

I'm afraid no discussion was mentioned.

[ac000]

│ What variables are we talking about here?

Variable-related APIs, correspond to nxt_tstr_{api}.

njs variables?

│ Is the afore mentioned discussion available anywhere?

I'm afraid no discussion was mentioned.

I thought the below might have been discussed somewhere...

upon consideration of actual requirements, we recognized that asynchronous support was not needed.

[hongzhidao]

njs variables?

It means Unit variables, for example, $host. This has nothing to do with njs here.

I thought the below might have been discussed somewhere...

We did consider supporting certain variables, such as $db_query_some, so that's why the API has callbacks ready and error handlers that are called after the async operations. But no such requirements are asked.

[ac000]

It means Unit variables, for example, $host . This has nothing to do with njs here.

Perhaps that should be made clear in the commit message then...

[hongzhidao]

It means Unit variables, for example, $host . This has nothing to do with njs here.

Perhaps that should be made clear in the commit message then...

Added, thanks.

[ac000]

I wonder if

4d515d54 Var: Removed unused functions and structure fields

and

2cb6b1bf Var: Removed unused functions

should be combined?

[hongzhidao]

I wonder if

4d515d54 Var: Removed unused functions and structure fields

and

2cb6b1bf Var: Removed unused functions

should be combined?

Good idea.

[ac000]

HI @hongzhidao

The only thing you might want to do to be consistent with recent commits is to lowercase the Var's and HTTPs in the commit subjects...

[hongzhidao]

HI @hongzhidao

The only thing you might want to do to be consistent with recent commits is to lowercase the Var's and HTTPs in the commit subjects...

Good reminder, thanks. Btw, I'll wait until variable updating feature or demo is ready before submitting these patches.

[callahad]

Status: The team needs to discuss if we merge this now or wait until variable updating is done.

As I understand it:

1. This PR is intended to ease the development of a new feature (updating Unit variables).
2. That feature is not yet ready.
3. We can't be certain that this refactor actually achieves its goals with its current design until that feature is developed.

Merging now avoids the risk of merge conflicts and burden of re-review, while waiting ensures that these patches are fully correct, necessary, and sufficient.

[callahad]

The team's decision is to defer to @hongzhidao :)

If you think we should merge it as-is, please do so.

Otherwise, please convert it back to a draft until it's ready for re-review and merging. There's a button at the top right under Reviewers:

[ac000]

Ah, while this is awaiting merging (or not), we should change the commit subjects to be in "imperative mood", e.g

var: Restrict nxt_tstr_query() to only support synchronous operation => no change (already imperative mood) http: Refactored return action => http: Refactor return action http: Refactored route pass query => http: Refactor route pass query http: Refactored static action => http: Refactor static action http: Refactored access log write => http: Refactor access log write var: Removed unused functions and structure fields => var: Remove unused functions and structure fields http: Added "if" option to match object => http: Add "if" option to match object

[hongzhidao]

Ah, while this is awaiting merging (or not), we should change the commit subjects to be in "imperative mood", e.g

var: Restrict nxt_tstr_query() to only support synchronous operation => no change (already imperative mood) http: Refactored return action => http: Refactor return action http: Refactored route pass query => http: Refactor route pass query http: Refactored static action => http: Refactor static action http: Refactored access log write => http: Refactor access log write var: Removed unused functions and structure fields => var: Remove unused functions and structure fields http: Added "if" option to match object => http: Add "if" option to match object

Ok. And I'm going to merge the prepared patches after I test the "if" option, then I'll create a new PR for it.

[ac000]

No need to make a new PR. You can un-draft this one!

See the Ready for review button? (just below the checks...)

I've had a look at the latest patch, I'm assuming the rest didn't change and it was just rebased against master... You can always re-request a persons review if things changed enough to warrant re-reviewing...

[hongzhidao]

Hi all, Welcome to review. Next, we need to add tests and I'm trying to refactor out the duplicate code between match "if" and access log "if", I feel they can be reused.

[hongzhidao]

Hi @ac000, Would you mind I merge the prepared variable related patches now? I think they are ready.

[ac000]

Hi @ac000, Would you mind I merge the prepared variable related patches now? I think they are ready.

Which patches exactly? These still have at least the typo in one of the subjects...

[hongzhidao]

Which patches exactly?

Except for those patches on the last one. I mean the refactoring patches.

These still have at least the typo in one of the subjects...

Fixed, thanks.

[ac000]

Could you push just what you intend to merge for clarity?

[hongzhidao]

I'm going to push the following commits first if you think their review is done.

var: Restrict nxt_tstr_query() to only support synchronous 
http: Refactor return action
http: Refactor route pass query
http: Refactor static action
http: Refactor access log write
var: Remove unused functions and structure fields

Then I'll post a new refactoring patch in front of the patch of "if" option, which I'll introduce nxt_tstr_cond_t for reuse.

[ac000]

Ah, right I see now, OK then...

[hongzhidao]

Well, it looks like I can't post part of the PR patches. Will post them together after the review is done.

[ac000]

Yeah, it can be done, but can be a bit fiddly...

I'll remove my approval while I review the new stuff...

[hongzhidao]

Hi @ac000,

I'm fine with both of the two styles when we do some features like this: a. split into two patches.

1. Introduce a prepared refactoring patch.
2. Replace existing code with corresponding structures/functions, etc.

b. just finish it once.

Refactor existing code.

It depends on the code size and whether there is too much crossed code in some functions or not. If it's small and has no crossed code, I prefer the second style.

Here this PR is this style. For example:

--- a/src/nxt_router_access_log.c
+++ b/src/nxt_router_access_log.c
@@ -143,15 +143,8 @@ nxt_router_access_log_create(nxt_task_t *task, nxt_router_conf_t *rtcf,
     if (alcf.expr != NULL) {
         nxt_conf_get_string(alcf.expr, &str);

-        if (str.length > 0 && str.start[0] == '!') {
-            rtcf->log_negate = 1;
-
-            str.start++;
-            str.length--;
-        }
-
-        rtcf->log_expr = nxt_tstr_compile(rtcf->tstr_state, &str, 0);
-        if (nxt_slow_path(rtcf->log_expr == NULL)) {
+        ret = nxt_tstr_cond_compile(rtcf->tstr_state, &str, &rtcf->log_cond);
+        if (nxt_slow_path(ret != NXT_OK)) {
             return NXT_ERROR;
         }
     }

But I'm fine if developers like to split it into two commits when I review it. So let me keep it as it is.

Although I'm curious for the reason to move nxt_http_request_access_log()?

The function has only about 4 lines of code after replacing its code with nxt_http_cond_value(). I think it's time to get rid of it.

[ac000]

Hi @ac000,

I'm fine with both of the two styles when we do some features like this: a. split into two patches.

1. Introduce a prepared refactoring patch.
2. Replace existing code with corresponding structures/functions, etc.

b. just finish it once.

Refactor existing code.

But this is more than just a refactoring... which makes it hard to review... we have multiple different types of changes all muddled up... whereas each commit should make one logical change...

It depends on the code size and whether there is too much crossed code in some functions or not. If it's small and has no crossed code, I prefer the second style.

Here this PR is this style. For example:

--- a/src/nxt_router_access_log.c
+++ b/src/nxt_router_access_log.c
@@ -143,15 +143,8 @@ nxt_router_access_log_create(nxt_task_t *task, nxt_router_conf_t *rtcf,
     if (alcf.expr != NULL) {
         nxt_conf_get_string(alcf.expr, &str);

-        if (str.length > 0 && str.start[0] == '!') {
-            rtcf->log_negate = 1;
-
-            str.start++;
-            str.length--;
-        }
-
-        rtcf->log_expr = nxt_tstr_compile(rtcf->tstr_state, &str, 0);
-        if (nxt_slow_path(rtcf->log_expr == NULL)) {
+        ret = nxt_tstr_cond_compile(rtcf->tstr_state, &str, &rtcf->log_cond);
+        if (nxt_slow_path(ret != NXT_OK)) {
             return NXT_ERROR;
         }
     }

But I'm fine if developers like to split it into two commits when I review it. So let me keep it as it is.

Although I'm curious for the reason to move nxt_http_request_access_log()?

The function has only about 4 lines of code after replacing its code with nxt_http_cond_value(). I think it's time to get rid of it.

Hmm, I'm talking about nxt_http_request_access_log() which has been moved, renamed & changed all in one go!

[hongzhidao]

I’m afraid splitting into smaller patches doesn’t make review easier. It will be like this:

1. Introduce nxt_tstr_cond_t stuff.
2. Rework nxt_http_request_access_log() with nxt_tstr_cond_t.

I feel the confusion is because of the remove of nxt_http_request_access_log, right?

Btw, moving it should happen finally.

[ac000]

I’m afraid splitting into smaller patches doesn’t make review easier. It will be like this:

1. Introduce nxt_tstr_cond_t stuff.

2. Rework nxt_http_request_access_log() with nxt_tstr_cond_t.

I think it would. The patches would be smaller and more concise...

I feel the confusion is because of the remove of nxt_http_request_access_log, right?

It certainly muddies things...

[hongzhidao]

1. Introduce nxt_tstr_cond_t stuff.
2. Rework nxt_http_request_access_log() with nxt_tstr_cond_t.

I think it would. The patches would be smaller and more concise...

It's the topic of the style I mentioned above, I'm fine with both of the styles.

It certainly muddies things...

So we are talking about the change of nxt_http_request_access_log(). I didn't delete it at first, it's like this:

static nxt_int_t
nxt_http_request_access_log(nxt_task_t *task, nxt_http_request_t *r,
    nxt_router_conf_t *rtcf)
{
   if (nxt_http_cond_value(task, r, &rtcf->log_cond)) {
        access_log->handler(task, r, access_log, rtcf->log_format);
        return NXT_OK;
    }

    return NXT_DECLIEND;
}

So removing it should happen at last. I feel it's ok to do it once.

[ac000]

1. Introduce nxt_tstr_cond_t stuff.
2. Rework nxt_http_request_access_log() with nxt_tstr_cond_t.

I think it would. The patches would be smaller and more concise...

It's the topic of the style I mentioned above, I'm fine with both of the styles.

I think we'll have to agree to disagree...

It certainly muddies things...

So we are talking about the change of nxt_http_request_access_log(). I didn't delete it at first, it's like this:

static nxt_int_t
nxt_http_request_access_log(nxt_task_t *task, nxt_http_request_t *r,
    nxt_router_conf_t *rtcf)
{
   if (nxt_http_cond_value(task, r, &rtcf->log_cond)) {
        access_log->handler(task, r, access_log, rtcf->log_format);
        return NXT_OK;
    }

    return NXT_DECLIEND;
}

So removing it should happen at last. I feel it's ok to do it once.

OK, so staring at it some more, I think the issue is that nxt_http_request_access_log() and nxt_http_cond_value() look very similar.

If you had changed nxt_http_request_access_log() in place rather than, I mean it looks like it was moved, but OK, adding the new function in a different location, things would look very different.

Hmm, let me show you what I mean. Refactor out nxt_tstr_cond_t from access log. (we should loose the trailing . btw..) would now look like

diff --git ./src/nxt_http.h ./src/nxt_http.h
index fe5e72a8..5369c8e1 100644
--- ./src/nxt_http.h
+++ ./src/nxt_http.h
@@ -441,6 +441,9 @@ void nxt_h1p_complete_buffers(nxt_task_t *task, nxt_h1proto_t *h1p,
     nxt_bool_t all);
 nxt_msec_t nxt_h1p_conn_request_timer_value(nxt_conn_t *c, uintptr_t data);

+int nxt_http_cond_value(nxt_task_t *task, nxt_http_request_t *r,
+    nxt_tstr_cond_t *cond);
+
 extern const nxt_conn_state_t  nxt_h1p_idle_close_state;

 #endif  /* _NXT_HTTP_H_INCLUDED_ */
diff --git ./src/nxt_http_request.c ./src/nxt_http_request.c
index ccd2b141..b142a052 100644
--- ./src/nxt_http_request.c
+++ ./src/nxt_http_request.c
@@ -24,8 +24,6 @@ static void nxt_http_request_proto_info(nxt_task_t *task,
 static void nxt_http_request_mem_buf_completion(nxt_task_t *task, void *obj,
     void *data);
 static void nxt_http_request_done(nxt_task_t *task, void *obj, void *data);
-static nxt_int_t nxt_http_request_access_log(nxt_task_t *task,
-    nxt_http_request_t *r, nxt_router_conf_t *rtcf);

 static u_char *nxt_http_date_cache_handler(u_char *buf, nxt_realtime_t *now,
     struct tm *tm, size_t size, const char *format);
@@ -861,12 +859,12 @@ nxt_http_request_error_handler(nxt_task_t *task, void *obj, void *data)
 void
 nxt_http_request_close_handler(nxt_task_t *task, void *obj, void *data)
 {
-    nxt_int_t                ret;
     nxt_http_proto_t         proto;
     nxt_router_conf_t        *rtcf;
     nxt_http_request_t       *r;
     nxt_http_protocol_t      protocol;
     nxt_socket_conf_joint_t  *conf;
+    nxt_router_access_log_t  *access_log;

     r = obj;
     proto.any = data;
@@ -878,8 +876,10 @@ nxt_http_request_close_handler(nxt_task_t *task, void *obj, void *data)
         r->logged = 1;

         if (rtcf->access_log != NULL) {
-            ret = nxt_http_request_access_log(task, r, rtcf);
-            if (ret == NXT_OK) {
+            access_log = rtcf->access_log;
+
+            if (nxt_http_cond_value(task, r, &rtcf->log_cond)) {
+                access_log->handler(task, r, access_log, rtcf->log_format);
                 return;
             }
         }
@@ -911,34 +911,34 @@ nxt_http_request_close_handler(nxt_task_t *task, void *obj, void *data)
 }

-static nxt_int_t
-nxt_http_request_access_log(nxt_task_t *task, nxt_http_request_t *r,
-    nxt_router_conf_t *rtcf)
+int
+nxt_http_cond_value(nxt_task_t *task, nxt_http_request_t *r,
+    nxt_tstr_cond_t *cond)
 {
-    nxt_int_t                ret;
-    nxt_str_t                str;
-    nxt_bool_t               expr;
-    nxt_router_access_log_t  *access_log;
+    nxt_int_t          ret;
+    nxt_str_t          str;
+    nxt_bool_t         expr;
+    nxt_router_conf_t  *rtcf;

-    access_log = rtcf->access_log;
+    rtcf = r->conf->socket_conf->router_conf;

     expr = 1;

-    if (rtcf->log_expr != NULL) {
+    if (cond->expr != NULL) {

-        if (nxt_tstr_is_const(rtcf->log_expr)) {
-            nxt_tstr_str(rtcf->log_expr, &str);
+        if (nxt_tstr_is_const(cond->expr)) {
+            nxt_tstr_str(cond->expr, &str);

         } else {
             ret = nxt_tstr_query_init(&r->tstr_query, rtcf->tstr_state,
                                       &r->tstr_cache, r, r->mem_pool);
             if (nxt_slow_path(ret != NXT_OK)) {
-                return NXT_DECLINED;
+                return -1;
             }

-            ret = nxt_tstr_query(task, r->tstr_query, rtcf->log_expr, &str);
+            ret = nxt_tstr_query(task, r->tstr_query, cond->expr, &str);
             if (nxt_slow_path(ret != NXT_OK)) {
-                return NXT_DECLINED;
+                return -1;
             }
         }

@@ -952,12 +952,7 @@ nxt_http_request_access_log(nxt_task_t *task, nxt_http_request_t *r,
         }
     }

-    if (rtcf->log_negate ^ expr) {
-        access_log->handler(task, r, access_log, rtcf->log_format);
-        return NXT_OK;
-    }
-
-    return NXT_DECLINED;
+    return cond->negate ^ expr;
 }

diff --git ./src/nxt_router.h ./src/nxt_router.h
index cfc7258c..06c6bb32 100644
--- ./src/nxt_router.h
+++ ./src/nxt_router.h
@@ -54,8 +54,7 @@ typedef struct {

     nxt_router_access_log_t  *access_log;
     nxt_tstr_t               *log_format;
-    nxt_tstr_t               *log_expr;
-    uint8_t                  log_negate;  /* 1 bit */
+    nxt_tstr_cond_t          log_cond;
 } nxt_router_conf_t;

diff --git ./src/nxt_router_access_log.c ./src/nxt_router_access_log.c
index cc8d5e4f..afecd0b1 100644
--- ./src/nxt_router_access_log.c
+++ ./src/nxt_router_access_log.c
@@ -143,15 +143,8 @@ nxt_router_access_log_create(nxt_task_t *task, nxt_router_conf_t *rtcf,
     if (alcf.expr != NULL) {
         nxt_conf_get_string(alcf.expr, &str);

-        if (str.length > 0 && str.start[0] == '!') {
-            rtcf->log_negate = 1;
-
-            str.start++;
-            str.length--;
-        }
-
-        rtcf->log_expr = nxt_tstr_compile(rtcf->tstr_state, &str, 0);
-        if (nxt_slow_path(rtcf->log_expr == NULL)) {
+        ret = nxt_tstr_cond_compile(rtcf->tstr_state, &str, &rtcf->log_cond);
+        if (nxt_slow_path(ret != NXT_OK)) {
             return NXT_ERROR;
         }
     }
diff --git ./src/nxt_tstr.c ./src/nxt_tstr.c
index a6d2e7ad..50df4c47 100644
--- ./src/nxt_tstr.c
+++ ./src/nxt_tstr.c
@@ -196,6 +196,26 @@ nxt_tstr_state_release(nxt_tstr_state_t *state)
 }

+nxt_int_t
+nxt_tstr_cond_compile(nxt_tstr_state_t *state, nxt_str_t *str,
+    nxt_tstr_cond_t *cond)
+{
+    if (str->length > 0 && str->start[0] == '!') {
+        cond->negate = 1;
+
+        str->start++;
+        str->length--;
+    }
+
+    cond->expr = nxt_tstr_compile(state, str, 0);
+    if (nxt_slow_path(cond->expr == NULL)) {
+        return NXT_ERROR;
+    }
+
+    return NXT_OK;
+}
+
+
 nxt_bool_t
 nxt_tstr_is_const(nxt_tstr_t *tstr)
 {
diff --git ./src/nxt_tstr.h ./src/nxt_tstr.h
index 2aa905df..aca74e20 100644
--- ./src/nxt_tstr.h
+++ ./src/nxt_tstr.h
@@ -37,12 +37,20 @@ typedef enum {
 } nxt_tstr_flags_t;

+typedef struct {
+    nxt_tstr_t          *expr;
+    uint8_t             negate;  /* 1 bit */
+} nxt_tstr_cond_t;
+
+
 nxt_tstr_state_t *nxt_tstr_state_new(nxt_mp_t *mp, nxt_bool_t test);
 nxt_tstr_t *nxt_tstr_compile(nxt_tstr_state_t *state, const nxt_str_t *str,
     nxt_tstr_flags_t flags);
 nxt_int_t nxt_tstr_test(nxt_tstr_state_t *state, nxt_str_t *str, u_char *error);
 nxt_int_t nxt_tstr_state_done(nxt_tstr_state_t *state, u_char *error);
 void nxt_tstr_state_release(nxt_tstr_state_t *state);
+nxt_int_t nxt_tstr_cond_compile(nxt_tstr_state_t *state, nxt_str_t *str,
+    nxt_tstr_cond_t *cond);

 nxt_bool_t nxt_tstr_is_const(nxt_tstr_t *tstr);
 void nxt_tstr_str(nxt_tstr_t *tstr, nxt_str_t *str);

The diffstat goes from

 src/nxt_http.h              |   3 +++
 src/nxt_http_request.c      | 105 ++++++++++++++++++++++++++++++++++++----------------------------------------
 src/nxt_router.h            |   3 +--
 src/nxt_router_access_log.c |  11 ++------
 src/nxt_tstr.c              |  20 +++++++++++++++
 src/nxt_tstr.h              |   8 ++++++
 6 files changed, 84 insertions(+), 66 deletions(-)

to

 src/nxt_http.h              |  3 +++
 src/nxt_http_request.c      | 45 ++++++++++++++++++++-------------------------
 src/nxt_router.h            |  3 +--
 src/nxt_router_access_log.c | 11 ++---------
 src/nxt_tstr.c              | 20 ++++++++++++++++++++
 src/nxt_tstr.h              |  8 ++++++++
 6 files changed, 54 insertions(+), 36 deletions(-)

Whether you think it looks like a simpler patch (we no longer have the large chuck of code removed and then another large chunk of added code) or not you can't argue with numbers...

[hongzhidao]

@@ -911,34 +911,34 @@ nxt_http_request_close_handler(nxt_task_t *task, void *obj, void *data)
 }

-static nxt_int_t
-nxt_http_request_access_log(nxt_task_t *task, nxt_http_request_t *r,
-    nxt_router_conf_t *rtcf)
+int
+nxt_http_cond_value(nxt_task_t *task, nxt_http_request_t *r,
+    nxt_tstr_cond_t *cond)
 {
-    nxt_int_t                ret;
-    nxt_str_t                str;
-    nxt_bool_t               expr;
-    nxt_router_access_log_t  *access_log;
+    nxt_int_t          ret;
+    nxt_str_t          str;
+    nxt_bool_t         expr;
+    nxt_router_conf_t  *rtcf;

-    access_log = rtcf->access_log;
+    rtcf = r->conf->socket_conf->router_conf;

If it looks like this, I'll choose your way to split it into smaller patches to make the review easier.

It depends on the code size and whether there is too much crossed code in some functions or not.

I called it crossed code.

I think the issue is that nxt_http_request_access_log() and nxt_http_cond_value() look very similar.

While I did it, I created the new function nxt_http_cond_value() first, then I needed to rework nxt_http_request_access_log(), and finally I found the function was too small that it was better to get rid of it.

In the commit, it looks like there is only deleting(-) and adding(+), so I feel it doesn't make review hard.

[ac000]

@@ -911,34 +911,34 @@ nxt_http_request_close_handler(nxt_task_t *task, void *obj, void *data)
 }

-static nxt_int_t
-nxt_http_request_access_log(nxt_task_t *task, nxt_http_request_t *r,
-    nxt_router_conf_t *rtcf)
+int
+nxt_http_cond_value(nxt_task_t *task, nxt_http_request_t *r,
+    nxt_tstr_cond_t *cond)
 {
-    nxt_int_t                ret;
-    nxt_str_t                str;
-    nxt_bool_t               expr;
-    nxt_router_access_log_t  *access_log;
+    nxt_int_t          ret;
+    nxt_str_t          str;
+    nxt_bool_t         expr;
+    nxt_router_conf_t  *rtcf;

-    access_log = rtcf->access_log;
+    rtcf = r->conf->socket_conf->router_conf;

If it looks like this, I'll choose your way to split it into smaller patches to make the review easier.

Even if you just do that commit as I showed that would I think be OK. (You could add the cond_t stuff as a separate patch first, up to you...)

It depends on the code size and whether there is too much crossed code in some functions or not.

I called it crossed code.

You mean code duplication?

I think the issue is that nxt_http_request_access_log() and nxt_http_cond_value() look very similar.

While I did it, I created the new function nxt_http_cond_value() first, then I needed to rework nxt_http_request_access_log(), and finally I found the function was too small that it was better to get rid of it.

In the commit, it looks like there is only deleting(-) and adding(+), so I feel it doesn't make review hard.

Well, I found it very confusing because it looked like there were three things going with that nxt_http_request_access_log() function. The alternate diff I showed makes things much more clear.

[hongzhidao]

I called it crossed code. You mean code duplication?

I mean code like following that mixed with adding and deleting codes. In this case, it makes sense to do something like splitting into smaller patches to help review.

@@ -911,34 +911,34 @@ nxt_http_request_close_handler(nxt_task_t *task, void *obj, void *data)
 }

-static nxt_int_t
-nxt_http_request_access_log(nxt_task_t *task, nxt_http_request_t *r,
-    nxt_router_conf_t *rtcf)
+int
+nxt_http_cond_value(nxt_task_t *task, nxt_http_request_t *r,
+    nxt_tstr_cond_t *cond)
 {
-    nxt_int_t                ret;
-    nxt_str_t                str;
-    nxt_bool_t               expr;
-    nxt_router_access_log_t  *access_log;
+    nxt_int_t          ret;
+    nxt_str_t          str;
+    nxt_bool_t         expr;
+    nxt_router_conf_t  *rtcf;

-    access_log = rtcf->access_log;
+    rtcf = r->conf->socket_conf->router_conf;

[ac000]

I called it crossed code. You mean code duplication?

I mean code like following that mixed with adding and deleting codes. In this case, it makes sense to do something like splitting into smaller patches to help review.

@@ -911,34 +911,34 @@ nxt_http_request_close_handler(nxt_task_t *task, void *obj, void *data)
 }

-static nxt_int_t
-nxt_http_request_access_log(nxt_task_t *task, nxt_http_request_t *r,
-    nxt_router_conf_t *rtcf)
+int
+nxt_http_cond_value(nxt_task_t *task, nxt_http_request_t *r,
+    nxt_tstr_cond_t *cond)
 {
-    nxt_int_t                ret;
-    nxt_str_t                str;
-    nxt_bool_t               expr;
-    nxt_router_access_log_t  *access_log;
+    nxt_int_t          ret;
+    nxt_str_t          str;
+    nxt_bool_t         expr;
+    nxt_router_conf_t  *rtcf;

-    access_log = rtcf->access_log;
+    rtcf = r->conf->socket_conf->router_conf;

A simple thing to keep in mind and to ask yourself is 'are they separate logical changes that can be reviewed individually?'...

It's up to you, but if you want to just do it like I showed, I'm OK with that. All I did was edit commit Refactor out nxt_tstr_cond_t from access log. and moved the nxt_http_cond_value() function to where nxt_http_request_access_log() had been...

[hongzhidao]

Well, I found it very confusing because it looked like there were three things going with that nxt_http_request_access_log() function. The alternate diff I showed makes things much more clear.

Take a look again, I separated the remove of nxt_http_request_access_log().

[ac000]

I think the best thing would be to just put it back like before but make

Refactor out nxt_tstr_cond_t from access log.`

Look like I showed here.

I would also change the subject to just Factor out nxt_tstr_cond_t from access log as it's more than just a simple refactor...

[ac000]

OK, looking at it again, I don't think it's so bad.

My problem now is the commit subject and message

http: Refactor out nxt_tstr_cond_t from access log

This nxt_tstr_cond_t will be reused for the feature of adding "if"
option to the "match" object. The two "if" options have the same usage.

Which doesn't really describe the change.

Not only does it factor code out of nxt_http_request_access_log(), but it adds a bunch of new code.

I think there should be a commit before this one that adds the nxt_tstr_cond_compile() function and nxt_tstr_cond_t structure.

Then I'm not sure where this change belongs yet...

diff --git ./src/nxt_router_access_log.c ./src/nxt_router_access_log.c
index cc8d5e4f..afecd0b1 100644
--- ./src/nxt_router_access_log.c
+++ ./src/nxt_router_access_log.c
@@ -143,15 +143,8 @@ nxt_router_access_log_create(nxt_task_t *task, nxt_router_conf_t *rtcf,
     if (alcf.expr != NULL) {
         nxt_conf_get_string(alcf.expr, &str);

-        if (str.length > 0 && str.start[0] == '!') {
-            rtcf->log_negate = 1;
-
-            str.start++;
-            str.length--;
-        }
-
-        rtcf->log_expr = nxt_tstr_compile(rtcf->tstr_state, &str, 0);
-        if (nxt_slow_path(rtcf->log_expr == NULL)) {
+        ret = nxt_tstr_cond_compile(rtcf->tstr_state, &str, &rtcf->log_cond);
+        if (nxt_slow_path(ret != NXT_OK)) {
             return NXT_ERROR;
         }
     }

Does it directly depend on or is depended on by the change to nxt_http_request_access_log() or could it be it's own commit?

[hongzhidao]

http: Refactor out nxt_tstr_cond_t from access log

“access log” in the subject means “access log module” but not “nxt_http_request_access_log()”, the later is one function in the access log module.

In the commit, it touched the access log module but don’t need to mention specific details like which function “nxt_http_request_access_log()”.

Then in the next commit, we get rid of “nxt_http_request_access_log()”.

Does it directly depend on or is depended on by the change to nxt_http_request_access_log() or could it be it's own commit?

Belongs to "the access log module".

Btw, I change "access log" to "the access log module" in the subject. Hope it eliminates the confusion.

[ac000]

I'm Looking at this again and by this, I'm talking about

    http: Refactor out nxt_tstr_cond_t from the access log module

    This nxt_tstr_cond_t will be reused for the feature of adding "if"
    option to the "match" object. The two "if" options have the same usage.

I'm struggling to reconcile the commit message with the change itself.

It really looks like to me this commit should be split into three

1) Add the nxt_tstr_cond_t structure and nxt_tstr_cond_compile() function.

2) Do the main change of extracting the code out of nxt_http_request_access_log() into nxt_http_cond_value()

Do NOT remove the log_str & log_negate structure members from nxt_router_conf_t

2) Do the change that updates nxt_router_access_log_create()

Now you can remove the log_str & log_negate structure members.

Let me show you what I mean.

That commit now becomes the following in cronological order (i.e git log --reverse)

(1)

    tstr: Add nxt_tstr_cond_t and nxt_tstr_cond_compile()

diff --git ./src/nxt_tstr.c ./src/nxt_tstr.c
index a6d2e7ad..50df4c47 100644
--- ./src/nxt_tstr.c
+++ ./src/nxt_tstr.c
@@ -196,6 +196,26 @@ nxt_tstr_state_release(nxt_tstr_state_t *state)
 }

+nxt_int_t
+nxt_tstr_cond_compile(nxt_tstr_state_t *state, nxt_str_t *str,
+    nxt_tstr_cond_t *cond)
+{
+    if (str->length > 0 && str->start[0] == '!') {
+        cond->negate = 1;
+
+        str->start++;
+        str->length--;
+    }
+
+    cond->expr = nxt_tstr_compile(state, str, 0);
+    if (nxt_slow_path(cond->expr == NULL)) {
+        return NXT_ERROR;
+    }
+
+    return NXT_OK;
+}
+
+
 nxt_bool_t
 nxt_tstr_is_const(nxt_tstr_t *tstr)
 {
diff --git ./src/nxt_tstr.h ./src/nxt_tstr.h
index 2aa905df..aca74e20 100644
--- ./src/nxt_tstr.h
+++ ./src/nxt_tstr.h
@@ -37,12 +37,20 @@ typedef enum {
 } nxt_tstr_flags_t;

+typedef struct {
+    nxt_tstr_t          *expr;
+    uint8_t             negate;  /* 1 bit */
+} nxt_tstr_cond_t;
+
+
 nxt_tstr_state_t *nxt_tstr_state_new(nxt_mp_t *mp, nxt_bool_t test);
 nxt_tstr_t *nxt_tstr_compile(nxt_tstr_state_t *state, const nxt_str_t *str,
     nxt_tstr_flags_t flags);
 nxt_int_t nxt_tstr_test(nxt_tstr_state_t *state, nxt_str_t *str, u_char *error);
 nxt_int_t nxt_tstr_state_done(nxt_tstr_state_t *state, u_char *error);
 void nxt_tstr_state_release(nxt_tstr_state_t *state);
+nxt_int_t nxt_tstr_cond_compile(nxt_tstr_state_t *state, nxt_str_t *str,
+    nxt_tstr_cond_t *cond);

 nxt_bool_t nxt_tstr_is_const(nxt_tstr_t *tstr);

(2)

    http: Factor out code from nxt_http_request_access_log()

diff --git ./src/nxt_http.h ./src/nxt_http.h
index fe5e72a8..5369c8e1 100644
--- ./src/nxt_http.h
+++ ./src/nxt_http.h
@@ -441,6 +441,9 @@ void nxt_h1p_complete_buffers(nxt_task_t *task, nxt_h1proto_t *h1p,
     nxt_bool_t all);
 nxt_msec_t nxt_h1p_conn_request_timer_value(nxt_conn_t *c, uintptr_t data);

+int nxt_http_cond_value(nxt_task_t *task, nxt_http_request_t *r,
+    nxt_tstr_cond_t *cond);
+
 extern const nxt_conn_state_t  nxt_h1p_idle_close_state;

 #endif  /* _NXT_HTTP_H_INCLUDED_ */
diff --git ./src/nxt_http_request.c ./src/nxt_http_request.c
index ccd2b141..a65163d0 100644
--- ./src/nxt_http_request.c
+++ ./src/nxt_http_request.c
@@ -915,44 +915,11 @@ static nxt_int_t
 nxt_http_request_access_log(nxt_task_t *task, nxt_http_request_t *r,
     nxt_router_conf_t *rtcf)
 {
-    nxt_int_t                ret;
-    nxt_str_t                str;
-    nxt_bool_t               expr;
     nxt_router_access_log_t  *access_log;

     access_log = rtcf->access_log;

-    expr = 1;
-
-    if (rtcf->log_expr != NULL) {
-
-        if (nxt_tstr_is_const(rtcf->log_expr)) {
-            nxt_tstr_str(rtcf->log_expr, &str);
-
-        } else {
-            ret = nxt_tstr_query_init(&r->tstr_query, rtcf->tstr_state,
-                                      &r->tstr_cache, r, r->mem_pool);
-            if (nxt_slow_path(ret != NXT_OK)) {
-                return NXT_DECLINED;
-            }
-
-            ret = nxt_tstr_query(task, r->tstr_query, rtcf->log_expr, &str);
-            if (nxt_slow_path(ret != NXT_OK)) {
-                return NXT_DECLINED;
-            }
-        }
-
-        if (str.length == 0
-            || nxt_str_eq(&str, "0", 1)
-            || nxt_str_eq(&str, "false", 5)
-            || nxt_str_eq(&str, "null", 4)
-            || nxt_str_eq(&str, "undefined", 9))
-        {
-            expr = 0;
-        }
-    }
-
-    if (rtcf->log_negate ^ expr) {
+    if (nxt_http_cond_value(task, r, &rtcf->log_cond)) {
         access_log->handler(task, r, access_log, rtcf->log_format);
         return NXT_OK;
     }
@@ -1364,3 +1331,48 @@ nxt_http_cookie_hash(nxt_mp_t *mp, nxt_str_t *name)
 {
     return nxt_http_field_hash(mp, name, 1, NXT_HTTP_URI_ENCODING_NONE);
 }
+
+
+int
+nxt_http_cond_value(nxt_task_t *task, nxt_http_request_t *r,
+    nxt_tstr_cond_t *cond)
+{
+    nxt_int_t          ret;
+    nxt_str_t          str;
+    nxt_bool_t         expr;
+    nxt_router_conf_t  *rtcf;
+
+    rtcf = r->conf->socket_conf->router_conf;
+
+    expr = 1;
+
+    if (cond->expr != NULL) {
+
+        if (nxt_tstr_is_const(cond->expr)) {
+            nxt_tstr_str(cond->expr, &str);
+
+        } else {
+            ret = nxt_tstr_query_init(&r->tstr_query, rtcf->tstr_state,
+                                      &r->tstr_cache, r, r->mem_pool);
+            if (nxt_slow_path(ret != NXT_OK)) {
+                return -1;
+            }
+
+            ret = nxt_tstr_query(task, r->tstr_query, cond->expr, &str);
+            if (nxt_slow_path(ret != NXT_OK)) {
+                return -1;
+            }
+        }
+
+        if (str.length == 0
+            || nxt_str_eq(&str, "0", 1)
+            || nxt_str_eq(&str, "false", 5)
+            || nxt_str_eq(&str, "null", 4)
+            || nxt_str_eq(&str, "undefined", 9))
+        {
+            expr = 0;
+        }
+    }
+
+    return cond->negate ^ expr;
+}
diff --git ./src/nxt_router.h ./src/nxt_router.h
index cfc7258c..60fc9557 100644
--- ./src/nxt_router.h
+++ ./src/nxt_router.h
@@ -56,6 +56,7 @@ typedef struct {
     nxt_tstr_t               *log_format;
     nxt_tstr_t               *log_expr;
     uint8_t                  log_negate;  /* 1 bit */
+    nxt_tstr_cond_t          log_cond;
 } nxt_router_conf_t;

(3)

    Convert nxt_router_access_log_create() to use nxt_tstr_cond_compile()

diff --git ./src/nxt_router.h ./src/nxt_router.h
index 60fc9557..06c6bb32 100644
--- ./src/nxt_router.h
+++ ./src/nxt_router.h
@@ -54,8 +54,6 @@ typedef struct {

     nxt_router_access_log_t  *access_log;
     nxt_tstr_t               *log_format;
-    nxt_tstr_t               *log_expr;
-    uint8_t                  log_negate;  /* 1 bit */
     nxt_tstr_cond_t          log_cond;
 } nxt_router_conf_t;

diff --git ./src/nxt_router_access_log.c ./src/nxt_router_access_log.c
index cc8d5e4f..afecd0b1 100644
--- ./src/nxt_router_access_log.c
+++ ./src/nxt_router_access_log.c
@@ -143,15 +143,8 @@ nxt_router_access_log_create(nxt_task_t *task, nxt_router_conf_t *rtcf,
     if (alcf.expr != NULL) {
         nxt_conf_get_string(alcf.expr, &str);

-        if (str.length > 0 && str.start[0] == '!') {
-            rtcf->log_negate = 1;
-
-            str.start++;
-            str.length--;
-        }
-
-        rtcf->log_expr = nxt_tstr_compile(rtcf->tstr_state, &str, 0);
-        if (nxt_slow_path(rtcf->log_expr == NULL)) {
+        ret = nxt_tstr_cond_compile(rtcf->tstr_state, &str, &rtcf->log_cond);
+        if (nxt_slow_path(ret != NXT_OK)) {
             return NXT_ERROR;
         }
     }

I think that is much clearer what is going on. The commits have become smaller, more concise and really do just one thing.

I've pushed this up to https://github.com/ac000/unit/commits/var-sync-rework/ so you can see the thing as a whole. Feel free to take them if you want...

[hongzhidao]

I've pushed this up to https://github.com/ac000/unit/commits/var-sync-rework/ so you can see the thing as a whole. Feel free to take them if you want...

Thanks a lot. It's a different way to think of it, I'd like to share my thoughts while I was implementing it.

1. First, a few things around "tstr cond" should be introduced together, for example:

   nxt_tstr_cond_t;
   nxt_tstr_cond_compile(...);
   nxt_tstr_cond_query(...);

   They should be created at the same time, like "var_" stuff, "compile" and "query" are paired, one happens in configuration phase, and the other happens in the request runtime phase. But I agree that it's not perfect that I used "nxt_http_cond_value()" instead of "nxt_tstr_cond_query(...)" since it has to depend on "nxt_http_request_t" object.

2. Then I replaced related code with "tstr cond" in the access log module as much as possible.

3. Since "nxt_http_request_access_log()" is confusing, I separated an individual patch to eliminate it.

Is this design idea clear for you?

[ac000]

I've pushed this up to https://github.com/ac000/unit/commits/var-sync-rework/ so you can see the thing as a whole. Feel free to take them if you want...

Thanks a lot. It's a different way to think of it, I'd like to share my thoughts while I was implementing it.

I thik the difference is you're talking about committing the feature (i.e this commit) as a whole while, I'm talking about splitting it into logical components.

But I don't think we're going to reach equilibrium on this, so I'll say OK to this and move onto looking at the rest of the commits...

[ac000]

OK, thanks I think this is good to go now...