subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <...@daniel.shahaf.name>
Subject Re: [swig-py3][patch] interfacing bytes object instead of str
Date Fri, 23 Nov 2018 08:57:47 GMT
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

Mime
View raw message