Yasuhito FUTATSUKI wrote on Fri, Nov 23, 2018 at 16:43:24 +0900: > On 11/23/18 11:21 AM, Daniel Shahaf wrote: > > Thanks for the patch. I'm afraid I'm a bit disoriented, though; could > > you kindly clarify a few things? > > > > Is the patch destined for trunk, for branches/swig-py3@HEAD, or for > > branches/swig-py3 after a sync-merge from trunk? > > It's destined for branches/swig-py3@HEAD,
Okay. > but actually I did sync-merge from trunk@1846855 and tag 1.11.0, then I > confirm via diff summary that the change in my patches doesn't affect merge. I see, but note that even if the changes don't conflict textually, they might still interact in other ways. In this example, one of the revisions to be merged, r1824410, adds a call to svn_diff_file_output_unified4(), which takes an «svn_stream_t *output_stream» parameter. > (I'm sorry, I did it on git, > https://github.com/futatuki/subversion/tree/swig-py3) > Well, until INFRA-7524 gets implemented, we'll just have to get you commit access to ^/subversion/branches ☺ > > Does it replace your > > previous patch, or are the two patches meant to be applied on top of > > each other? > > This is the latter. My secondary patch doesn't replace previous patch, > but assuming first patch applied. > Okay. Generally we prefer a second iteration of the patch to be sent as a diff against HEAD, in order for the submissions to be self-contained. > > How do we test the patch to see the problem that it > > corrects --- I assume that we should just run check-swig-py in a py3 > > build and see that it has fewer failures with the patch than without; is > > that correct? > > It is partly correct, because my patch may affect some place where curent > test doesn't check. Actually I run check-swig-py before apply secondary > patch, it returns ok without failure, so I think it doen't test > svn_stream_write() and svn_stream_readline(), though I'm not see whole > of the test. > 'check-swig-py3' is the entire test suite, as far as I know. > So I did also bulding subversion/bindings/swig/python/ > both of before apply patch and after apply, then diff them, to make sure > the patch doesn't affect other functions other than functions related > to svn_stream_read*() and svn_stream_write(). > Good idea. > (I confess I made a diff only for subversion/bindings/swig/python/core.c > on my previous post, and now I found the patches affects > subversion/bindings/swig/python/core.py and > subversion/bindings/swig/python/libsvn/core.py, with incorrect function > annotation for svn_stream_readline() and svn_stream_invoke_readline_fn().) > Of course, I think it is not sufficient and I think it need tests for > svn_stream_readline() and svn_stream_write(). > Yes, please. Tests would help us review the change. You can use svn_stream_empty() or svn_stream_from_stringbuf() for the tests. > I ran ViewVC as a test for svn_stream_readline(), with branch merged > branch swig-py3 and 1.11.0 tree (for my own convenient), > but I know it is not appropriate here, and it isn't use svn_stream_write(). Yes, running ViewVC is a good smoke test, though one that doesn't necessarily have good coverage of the relevant interfaces. Then again, I'm not sure that check-swig-py has good coverage either… Cheers, Daniel
