Stefan Sperling wrote:
> svn diff https://svn.apache.org/repos/asf/subversion/trunk@r1658686 \
> https://svn.apache.org/repos/asf/subversion/branches/pin-externals
>
> Some bugs have been fixed and the regression test has been made
> more fine-grained so tests can be run individually.
> Please see the branch's log messages for details.
>
> Are there any more objections to merging the branch to trunk?
Hi Stefan.
I don't have any objection to merging this to trunk. Comments from a partial review follow.
I'd like to repeat my request for a written description of what "pinning" means. Specifically, the condition for an external definition to be eligible for pinning, and in what way we change it. This should be written down somewhere in the source tree. The doc string of svn_client_copy7() should refer to it.
The doc string of svn_client_copy7() should describe the restriction on a WC copy source being a complete, single-rev tree.
These new static functions need doc strings:
make_external_description()
resolve_pinned_externals()
queue_externals_change_path_infos()
Please adjust svncopy.pl's help text to direct the users of its '--pin-externals' option to try 'svn copy --pin-externals' instead (or delete the script, but as I said before I mildly prefer keeping it for now).
test_copy_pin_externals():
I gather that the purpose of this test is just to check that the 'externals_to_pin' option takes effect, not to test the full range of possible transformations and non-transformations of external definitions, and that's why it only covers a small sample of possible forms of definition. The test might be much simpler if it would test the exact string format of the resulting definitions, instead of their parsed form. The input and output cases could then be defined next to each other in the test source code for easier reading. You might still want to parse the definitions just to double-check that they're parsable.
The test calls svn_wc_parse_externals_description3(canonicalize_url=TRUE) which is declared by its doc string as inappropriate when the input contains a repos-relative URL, which it does.
The test says it sets up "parameters for pinning 2 externals" but in fact sets up for 3; and while one case ensures a request for '^/A/D/H' doesn't match '^/A/D/H_at_1'
It lacks test cases for externals_to_pin entries that don't match any actual definition.
It lacks test cases for externals_to_pin entries that are keyed by the 'wrong' target path/URL but otherwise would match an actual definition.
Do the tests cover: cases that use {DATE} format, and cases that have both
an operative rev and a peg rev? (I'm staring at the tests and not easily seeing these.)
Repeating something I said before, re. {DATE}: Looks like timestamp rev specs can have an issue with ambiguous time zone.
That's a separate problem, but I wonder if converting it to a Zulu time
at this point is the right thing to do. I think not. In other words, this code should preserve the exact textual form of the {DATE} spec. I'm not sure if it currently does.
> Branko Čibej wrote:
>> Looks OK, apart from the minor detail that all 10 pin-externals tests in
>> externals_tests.py are failing now.
>
> Thanks for the heads up. This was due to the last sync with trunk which
> brought in changes from r1658410. [...]
Ahh, that was me :-) Sorry about that.
- Julian
Received on 2015-02-10 19:08:20 CET