Could you review and merge my changes ? #107

mtourne · 2012-04-26T03:56:16Z

No description provided.

… added 073-backtrace.t to test it. see github pull request #107.

…uest header and could hang. thanks Matthieu Tourne for the patch in pull request #107.

Conflicts: src/ngx_http_lua_socket.c src/ngx_http_lua_util.c t/067-req-socket.t

…_by_lua".

…the current lua source file name (if any), the current source line number, and the current calling lua function name (if any), into the nginx error.log file. thanks Matthieu Tourne for the patch.

….log() and print(). thanks Matthieu Tourne for the patch.

fixed minor grammatical errors in the README.markdown file

Conflicts: README.markdown

…[lua] "; also updated tests to reflect recent changes.

… print().

agentzh · 2012-05-22T08:22:50Z

In the "prefix-var" branch, I've renamed $ngx_prefix to $prefix and the notation ${prefix} is also supported :)

mtourne · 2012-05-22T17:30:27Z

Just received the merge request, but I assume you or chaoslawful want to
merge it back into master.

Let me know how we should organize the workflow :)

Matt

On Tue, May 22, 2012 at 1:22 AM, agentzh (章亦春) <
reply@reply.github.com

wrote:

In the "prefix-var" branch, I've renamed $ngx_prefix to $prefix and the
notation ${prefix} is also supported :)

Reply to this email directly or view it on GitHub:

#107 (comment)

Conflicts: src/ngx_http_lua_util.c

agentzh · 2012-05-23T02:34:06Z

On Wed, May 23, 2012 at 1:30 AM, Matthieu Tourne
reply@reply.github.com
wrote:

Just received the merge request, but I assume you or chaoslawful want to
merge it back into master.

Let me know how we should organize the workflow :)

I'll review and merge your changes (either in a custom branch in the
official repos or any branch in your own fork repos) into "master" :)
Thanks!

Regards,
-agentzh

agentzh · 2012-05-23T15:42:25Z

On Wed, May 23, 2012 at 1:30 AM, Matthieu Tourne
reply@reply.github.com
wrote:

Just received the merge request, but I assume you or chaoslawful want to
merge it back into master.

Let me know how we should organize the workflow :)

One more small suggestion: could you please put each independent
change set into separate branches (either in your own fork repos or in
the mainstream repos) for future development? I mean branch names like
receiveuntil-inclusive, expect-fixes, new-req-api, and logby, instead
of putting everything into a single "master" branch or something like
that. By doing this, it can give me more freedom in merging your
patches by any order and it can ensure independencies among these
change sets ;)

Thank you so much for those excellent patches! I'll surely speed up
merging your patches into the mainstream master ;)

Thanks again!
-agentzh

mtourne · 2012-05-23T15:45:13Z

On Wed, May 23, 2012 at 8:42 AM, agentzh (章亦春) <
reply@reply.github.com

wrote:

On Wed, May 23, 2012 at 1:30 AM, Matthieu Tourne
reply@reply.github.com
wrote:

Just received the merge request, but I assume you or chaoslawful want to
merge it back into master.

Let me know how we should organize the workflow :)

One more small suggestion: could you please put each independent
change set into separate branches (either in your own fork repos or in
the mainstream repos) for future development? I mean branch names like
receiveuntil-inclusive, expect-fixes, new-req-api, and logby, instead
of putting everything into a single "master" branch or something like
that. By doing this, it can give me more freedom in merging your
patches by any order and it can ensure independencies among these
change sets ;)

Ok, I'll do that for any new feature I'm adding from now on.

I don't know how to separate the existing commits into different branches
:|

Thank you so much for those excellent patches! I'll surely speed up
merging your patches into the mainstream master ;)

Thanks again!
-agentzh

Reply to this email directly or view it on GitHub:

#107 (comment)

agentzh · 2012-05-24T01:56:04Z

On Wed, May 23, 2012 at 11:45 PM, Matthieu Tourne
reply@reply.github.com
wrote:

Ok, I'll do that for any new feature I'm adding from now on.

Thank you very much!

I don't know how to separate the existing commits into different branches
:|

git co my-new-feature-branch
git cherry-pick <commit-id>

If the existing commits are too hard to cherry pick (due to non-atomic
commits), just leave them as is. I can handle that on my side by
applying your patches manually from the "master" branch in your fork
repos ;)

This is just a suggestion for your future development ;)

Thanks!
-agentzh

mtourne · 2012-05-25T18:40:51Z

On Wed, May 23, 2012 at 6:56 PM, agentzh (章亦春)
reply@reply.github.com
wrote:

On Wed, May 23, 2012 at 11:45 PM, Matthieu Tourne
reply@reply.github.com
wrote:

Ok, I'll do that for any new feature I'm adding from now on.

Thank you very much!

I don't know how to separate the existing commits into different branches
:|

git co my-new-feature-branch
git cherry-pick

If the existing commits are too hard to cherry pick (due to non-atomic
commits), just leave them as is. I can handle that on my side by
applying your patches manually from the "master" branch in your fork
repos ;)

This is just a suggestion for your future development ;)

I will definitely observe that for future developments, been too busy
to put them back in their own branches.
I think the only thing you're missing from me is the ngx.init_body(),
ngx.append_body(), ngx.finish_body()

And the last ngx.decode_args()

I merged all the other changes that you backported with modifications

agentzh · 2012-05-26T12:56:44Z

Hello!

On Sat, May 26, 2012 at 2:40 AM, Matthieu Tourne
reply@reply.github.com
wrote:

I will definitely observe that for future developments, been too busy
to put them back in their own branches.

Understood :)

I think the only thing you're missing from me is the ngx.init_body(),
ngx.append_body(), ngx.finish_body()

And the last ngx.decode_args()

I merged ngx.decode_args() into master yesterday on the train. Until
now, the missing bits are:

ngx.req.init_body(), ngx.req.append_body(), and
ngx.req.finish_body() (already in the "req-body" branch, but not ready
to be merged into "master" yet).
log_by_lua and log_by_lua_file (already in the "logby" branch, but
not ready to be merged into "master")
the rewrite_by_lua_no_postpone directive (already in the
"no-postpone" branch, but not ready to be merged into "master" yet).
the inclusive option for cosocket:receiveuntil().

But I'm now wondering

if you have the tuits to add some test cases and docs to the branch
"req-body",
what's your intention of manipulating "rb->rest" in
ngx_http_lua_ngx_req_init_body() in the commit e0eea53:

commit e0eea53
Author: Matthieu Tourne matthieu@cloudflare.com
Date: Sat Apr 21 16:37:41 2012 -0700

Fix issues with POST using http 1.1 (test for expect)
what's your intention of scaling the size argument by 50% in
ngx_http_lua_ngx_req_init_body(), which is not appropriate in my
opinion, and
if you have tested your patch for the inclusive option for
receiveuntil() with very small buffer size (because buffer handling in
your patch looks not quite right to me).

I merged all the other changes that you backported with modifications

I've noticed that and I'm trying to tweak your existing commits
instead of manually applying modified versions of your patches to
reduce merge conflicts on your side.

Thank you!
-agentzh

agentzh · 2012-05-27T16:24:46Z

Status update:

The "no-postpone" branch has already been merged into "master".
I've fixed the receiveuntil() inclusive mode for small buffers in the "inclusive" branch.

mtourne · 2012-05-29T17:41:34Z

On Sat, May 26, 2012 at 5:56 AM, agentzh (章亦春)
reply@reply.github.com
wrote:

Hello!

On Sat, May 26, 2012 at 2:40 AM, Matthieu Tourne
reply@reply.github.com
wrote:

I will definitely observe that for future developments, been too busy
to put them back in their own branches.

Understood :)

I think the only thing you're missing from me is the ngx.init_body(),
ngx.append_body(), ngx.finish_body()

And the last ngx.decode_args()

I merged ngx.decode_args() into master yesterday on the train. Until
now, the missing bits are:

ngx.req.init_body(), ngx.req.append_body(), and
ngx.req.finish_body() (already in the "req-body" branch, but not ready
to be merged into "master" yet).

log_by_lua and log_by_lua_file (already in the "logby" branch, but
not ready to be merged into "master")

the rewrite_by_lua_no_postpone directive (already in the
"no-postpone" branch, but not ready to be merged into "master" yet).

the inclusive option for cosocket:receiveuntil().

But I'm now wondering

if you have the tuits to add some test cases and docs to the branch
"req-body",

I don't really have test cases, I have a multipart.lua (heavily
insipired from upload.lua), it seems to work well so far.

what's your intention of manipulating "rb->rest" in
ngx_http_lua_ngx_req_init_body() in the commit e0eea53:

commit e0eea53
Author: Matthieu Tourne matthieu@cloudflare.com
Date: Sat Apr 21 16:37:41 2012 -0700

Fix issues with POST using http 1.1 (test for expect)

what's your intention of scaling the size argument by 50% in
ngx_http_lua_ngx_req_init_body(), which is not appropriate in my
opinion, and

Both those were inspired from ngx_http_request_body.c in Nginx core

Line 190:
size = clcf->client_body_buffer_size;
size += size >> 2;

if you have tested your patch for the inclusive option for
receiveuntil() with very small buffer size (because buffer handling in
your patch looks not quite right to me).

I merged all the other changes that you backported with modifications

I've noticed that and I'm trying to tweak your existing commits
instead of manually applying modified versions of your patches to
reduce merge conflicts on your side.

I thought this was how it would work :

I would make a little scrappy patch to add a feature
send a pull requests
you would comment on the style, or the lack of test or documentation,
I would rework it a little
we're both happy with it, you can integrate it \o/

Next feature, I'm doing a branch on the chaoslawful repo and we can
work this way.

The extra work should be on me, clean up etc so you can integrate it.

Thank you!
-agentzh

Reply to this email directly or view it on GitHub:
#107 (comment)

mtourne · 2012-05-29T22:41:18Z

Sorry sorry sorry, I pushed some bad code on the master :( :(
I suck at git ..

On Tue, May 29, 2012 at 10:41 AM, Matthieu Tourne
matthieu.tourne@gmail.com wrote:

On Sat, May 26, 2012 at 5:56 AM, agentzh (章亦春)
reply@reply.github.com
wrote:

Hello!

On Sat, May 26, 2012 at 2:40 AM, Matthieu Tourne
reply@reply.github.com
wrote:

I will definitely observe that for future developments, been too busy
to put them back in their own branches.

Understood :)

I think the only thing you're missing from me is the ngx.init_body(),
ngx.append_body(), ngx.finish_body()

And the last ngx.decode_args()

I merged ngx.decode_args() into master yesterday on the train. Until
now, the missing bits are:

ngx.req.init_body(), ngx.req.append_body(), and
ngx.req.finish_body() (already in the "req-body" branch, but not ready
to be merged into "master" yet).

log_by_lua and log_by_lua_file (already in the "logby" branch, but
not ready to be merged into "master")

the rewrite_by_lua_no_postpone directive (already in the
"no-postpone" branch, but not ready to be merged into "master" yet).

the inclusive option for cosocket:receiveuntil().

But I'm now wondering

if you have the tuits to add some test cases and docs to the branch
"req-body",

I don't really have test cases, I have a multipart.lua (heavily
insipired from upload.lua), it seems to work well so far.

what's your intention of manipulating "rb->rest" in
ngx_http_lua_ngx_req_init_body() in the commit e0eea53:

commit e0eea53
Author: Matthieu Tourne matthieu@cloudflare.com
Date: Sat Apr 21 16:37:41 2012 -0700

Fix issues with POST using http 1.1 (test for expect)

what's your intention of scaling the size argument by 50% in
ngx_http_lua_ngx_req_init_body(), which is not appropriate in my
opinion, and

Both those were inspired from ngx_http_request_body.c in Nginx core

Line 190:
size = clcf->client_body_buffer_size;
size += size >> 2;

if you have tested your patch for the inclusive option for
receiveuntil() with very small buffer size (because buffer handling in
your patch looks not quite right to me).

I merged all the other changes that you backported with modifications

I've noticed that and I'm trying to tweak your existing commits
instead of manually applying modified versions of your patches to
reduce merge conflicts on your side.

I thought this was how it would work :

I would make a little scrappy patch to add a feature

send a pull requests

you would comment on the style, or the lack of test or documentation,

I would rework it a little

we're both happy with it, you can integrate it \o/

Next feature, I'm doing a branch on the chaoslawful repo and we can
work this way.

The extra work should be on me, clean up etc so you can integrate it.

Thank you!
-agentzh

Reply to this email directly or view it on GitHub:
#107 (comment)

mtourne · 2012-05-29T22:57:17Z

I reverted the whole commit, we can drop it from the history if you
want. But I thought you'd prefer not to do that.

On Tue, May 29, 2012 at 3:40 PM, Matthieu Tourne
matthieu.tourne@gmail.com wrote:

Sorry sorry sorry, I pushed some bad code on the master :( :(
I suck at git ..

On Tue, May 29, 2012 at 10:41 AM, Matthieu Tourne
matthieu.tourne@gmail.com wrote:

On Sat, May 26, 2012 at 5:56 AM, agentzh (章亦春)
reply@reply.github.com
wrote:

Hello!

On Sat, May 26, 2012 at 2:40 AM, Matthieu Tourne
reply@reply.github.com
wrote:

I will definitely observe that for future developments, been too busy
to put them back in their own branches.

Understood :)

I think the only thing you're missing from me is the ngx.init_body(),
ngx.append_body(), ngx.finish_body()

And the last ngx.decode_args()

I merged ngx.decode_args() into master yesterday on the train. Until
now, the missing bits are:

ngx.req.init_body(), ngx.req.append_body(), and
ngx.req.finish_body() (already in the "req-body" branch, but not ready
to be merged into "master" yet).

log_by_lua and log_by_lua_file (already in the "logby" branch, but
not ready to be merged into "master")

the rewrite_by_lua_no_postpone directive (already in the
"no-postpone" branch, but not ready to be merged into "master" yet).

the inclusive option for cosocket:receiveuntil().

But I'm now wondering

if you have the tuits to add some test cases and docs to the branch
"req-body",

I don't really have test cases, I have a multipart.lua (heavily
insipired from upload.lua), it seems to work well so far.

what's your intention of manipulating "rb->rest" in
ngx_http_lua_ngx_req_init_body() in the commit e0eea53:

commit e0eea53
Author: Matthieu Tourne matthieu@cloudflare.com
Date: Sat Apr 21 16:37:41 2012 -0700

Fix issues with POST using http 1.1 (test for expect)

what's your intention of scaling the size argument by 50% in
ngx_http_lua_ngx_req_init_body(), which is not appropriate in my
opinion, and

Both those were inspired from ngx_http_request_body.c in Nginx core

Line 190:
size = clcf->client_body_buffer_size;
size += size >> 2;

if you have tested your patch for the inclusive option for
receiveuntil() with very small buffer size (because buffer handling in
your patch looks not quite right to me).

I merged all the other changes that you backported with modifications

I've noticed that and I'm trying to tweak your existing commits
instead of manually applying modified versions of your patches to
reduce merge conflicts on your side.

I thought this was how it would work :

I would make a little scrappy patch to add a feature

send a pull requests

you would comment on the style, or the lack of test or documentation,

I would rework it a little

we're both happy with it, you can integrate it \o/

Next feature, I'm doing a branch on the chaoslawful repo and we can
work this way.

The extra work should be on me, clean up etc so you can integrate it.

Thank you!
-agentzh

Reply to this email directly or view it on GitHub:
#107 (comment)

mtourne · 2012-06-01T07:48:28Z

Hello!

On Wed, May 30, 2012 at 1:41 AM, Matthieu Tourne
matthieu.tourne@gmail.com wrote:

But I'm now wondering

if you have the tuits to add some test cases and docs to the branch
"req-body",

I don't really have test cases, I have a multipart.lua (heavily
insipired from upload.lua), it seems to work well so far.

It's considered good practice to add test cases to ngx_lua's test
suite when adding new features or fixing existing bugs in ngx_lua :)

what's your intention of manipulating "rb->rest" in
ngx_http_lua_ngx_req_init_body() in the commit e0eea53:

commit e0eea53
Author: Matthieu Tourne matthieu@cloudflare.com
Date: Sat Apr 21 16:37:41 2012 -0700

Fix issues with POST using http 1.1 (test for expect)

what's your intention of scaling the size argument by 50% in
ngx_http_lua_ngx_req_init_body(), which is not appropriate in my
opinion, and

Both those were inspired from ngx_http_request_body.c in Nginx core

Line 190:
size = clcf->client_body_buffer_size;
size += size >> 2;

Fair enough :)

But what about rb->rest? This is used by
ngx_http_read_client_request_body while reading the input data stream.
I'm wondering about its usage here :)

if you have tested your patch for the inclusive option for
receiveuntil() with very small buffer size (because buffer handling in
your patch looks not quite right to me).

I merged all the other changes that you backported with modifications

I've noticed that and I'm trying to tweak your existing commits
instead of manually applying modified versions of your patches to
reduce merge conflicts on your side.

I thought this was how it would work :

I would make a little scrappy patch to add a feature

send a pull requests

you would comment on the style, or the lack of test or documentation,

I would rework it a little

we're both happy with it, you can integrate it \o/

Next feature, I'm doing a branch on the chaoslawful repo and we can
work this way.

The extra work should be on me, clean up etc so you can integrate it.

That looks cool to me :)

Thanks!
-agentzh

agentzh · 2012-06-16T06:27:06Z

I've just merged the "inclusive" branch into "master". This feature is expected to be included in the next ngx_lua release 0.5.1 :)

Now for all your patches in this pull request, the only missing bit is the ngx.req.init_body(), ngx.req.append_body(), and ngx.req.finish_body() API included in the "req-body" branch that has not yet been merged into "master". I'm still working on it :)

BTW, I've already applied a modified version of your patches in the "cleanup" branch. Can I remove the "cleanup" branch for you?

agentzh · 2012-06-16T06:32:14Z

Do you have the tuits to add some docs and tests to the "req-body" branch for the ngx.req.init_body(), ngx.req.append_body(), and ngx.req.finish_body() API?

BTW, both README and README.markdown are automatically generated from docs/HttpLuaModule.wiki by the scripts here:

https://github.com/agentzh/nginx-devel-utils

Basically I just run the top-level script "ngx-releng" ;)

dakanji · 2012-06-16T13:22:56Z

If you simply copy "docs/HttpLuaModule.wiki" and save it as "README.mediawiki" in the top level directory, you can remove "README" and "README.markdown" and you will have your readme file with all your mediawiki style formatting maintained.

agentzh · 2012-06-16T15:26:54Z

Hello!

On Sat, Jun 16, 2012 at 9:22 PM, dakanji
reply@reply.github.com
wrote:

If you simply copy "docs/HttpLuaModule.wiki" and save it as "README.mediawiki" in the top level directory, you can remove "README" and "README.markdown" and you will have your readme file with all your mediawiki style formatting maintained.

No, not really, a lot of links in docs/HttpLuaModule.wiki require
relocating because they're only meaningful to the wiki.nginx.org site.

Regards,
-agentzh

dakanji · 2012-06-16T17:42:10Z

Yes. A lot of them are just "#XYZ".

However, since these need to be fixed with the full URLs in any case, perhaps just a "find and replace" to convert them to "FULL/URL/TO/WIKI/LUA/DOCS#XYZ" and use the "mediawiki" extension to get the formatting benefits and also avoid the current need for two readme files.

… added 073-backtrace.t to test it. see github pull request openresty#107.

…uest header and could hang. thanks Matthieu Tourne for the patch in pull request openresty#107.

Matthieu Tourne added 13 commits April 24, 2012 11:49

Adding directive rewrite_no_postpone

fa3eeb0

Adding trace to errors

2ee6f46

Adding "variable" $ngx_prefix to lua_package_path

a12d72f

Adding ngx.req.init_body() ngx.req.append_body() ngx.req.finish_body()

b80b502

Fix issues with POST using http 1.1 (test for expect)

e0eea53

Merge remote-tracking branch 'upstream/master'

776f499

Adding log_by_lua and log_by_lua_file and documentation

1901199

Adding README.markdown

77eb003

Merge remote-tracking branch 'upstream/master'

2db0ae3

Adding test for inclusive mode

ef8e2bf

Adding logic for the receiveuntil option { inclusive = true }

20b3395

Adding documentation for the receiveuntil option { inclusive = true }

34a36af

Merge remote-tracking branch 'upstream/master'

d4b111b

agentzh added a commit that referenced this pull request May 14, 2012

minor coding style fixes in the patch for adding Lua backtraces. also…

8e68773

… added 073-backtrace.t to test it. see github pull request #107.

Matthieu Tourne added 2 commits May 14, 2012 12:26

Adding failing test

8cd749f

Fix receiveuntil("pattern", { inclusive = false })

9144d24

agentzh added a commit that referenced this pull request May 15, 2012

bugfix: ngx.req.socket() did not honor the "Expect: 100-continue" req…

754d547

…uest header and could hang. thanks Matthieu Tourne for the patch in pull request #107.

Matthieu Tourne and others added 13 commits May 15, 2012 11:12

Merge remote-tracking branch 'upstream/master'

8e24cd1

Conflicts: src/ngx_http_lua_socket.c src/ngx_http_lua_util.c t/067-req-socket.t

fixed minor coding styles

4174f0c

deleted a reduntant blank line

54f5a70

Add ngx.decode_args()

c465d2b

changed the chunk name of set_by_lua from "set_by_lua_inline" to "set…

f8e612a

…_by_lua".

feature: ngx.log() and print() now output more debugging info, i.e., …

f75f20d

…the current lua source file name (if any), the current source line number, and the current calling lua function name (if any), into the nginx error.log file. thanks Matthieu Tourne for the patch.

feature: added the "[lua]" prefix to the log messages produced by ngx…

2cd7586

….log() and print(). thanks Matthieu Tourne for the patch.

Merge branch 'master' of https://github.com/chaoslawful/lua-nginx-module

fd0fa3e

lua -> Lua

5f1c7c3

fixed some minor grammatical errors

77c715d

Merge pull request openresty#117 from zhuzhaoyuan/master

0538286

fixed minor grammatical errors in the README.markdown file

Merge remote-tracking branch 'upstream/master'

227b9c3

Conflicts: README.markdown

Merge remote-tracking branch 'upstream/logging'

932a748

agentzh added 2 commits May 22, 2012 13:04

modified the message prefix of print() from "[lua] print: " to just "…

33b3d45

…[lua] "; also updated tests to reflect recent changes.

optimize: eliminated unnecessary lua-side operations in ngx.log() and…

1e6de3d

… print().

Matthieu Tourne added 5 commits May 22, 2012 10:36

Merge remote-tracking branch 'upstream/logging'

849ca03

Merge remote-tracking branch 'upstream/prefix-var'

d0f5ed3

Conflicts: src/ngx_http_lua_util.c

Merge branch 'devel'

50f47a9

Adding documentation for ngx.decode_args()

886792c

Fixing incorrect merge from prefix-var branch

c5016c1

mtourne merged commit c5016c1 into openresty:master May 29, 2012

bakins pushed a commit to bakins/lua-nginx-module that referenced this pull request Jun 30, 2012

minor coding style fixes in the patch for adding Lua backtraces. also…

6fc0449

… added 073-backtrace.t to test it. see github pull request openresty#107.

bakins pushed a commit to bakins/lua-nginx-module that referenced this pull request Jun 30, 2012

bugfix: ngx.req.socket() did not honor the "Expect: 100-continue" req…

58e7304

…uest header and could hang. thanks Matthieu Tourne for the patch in pull request openresty#107.

Could you review and merge my changes ? #107

Could you review and merge my changes ? #107

Uh oh!

Conversation

mtourne commented Apr 26, 2012

Uh oh!

agentzh commented May 22, 2012

Uh oh!

mtourne commented May 22, 2012

Uh oh!

agentzh commented May 23, 2012

Uh oh!

agentzh commented May 23, 2012

Uh oh!

mtourne commented May 23, 2012

Uh oh!

agentzh commented May 24, 2012

Uh oh!

mtourne commented May 25, 2012

Uh oh!

agentzh commented May 26, 2012

Uh oh!

agentzh commented May 27, 2012

Uh oh!

mtourne commented May 29, 2012

Uh oh!

mtourne commented May 29, 2012

Uh oh!

mtourne commented May 29, 2012

Uh oh!

mtourne commented Jun 1, 2012

Uh oh!

agentzh commented Jun 16, 2012

Uh oh!

agentzh commented Jun 16, 2012

Uh oh!

dakanji commented Jun 16, 2012

Uh oh!

agentzh commented Jun 16, 2012

Uh oh!

dakanji commented Jun 16, 2012

Uh oh!

Uh oh!