On Tue, Feb 10, 2015 at 06:06:25PM +0000, Julian Foad wrote:
> 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.)
>
Thanks for poking me on these points. I'll look into them.
> 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.
That has been fixed in r1655872. The date string is preserved now.
BTW, I don't know why the Z was in the generated date string but
I don't think its intended meaning is "Zulu time". It's part of
a static string in our own code (libsvn_subr/time.c):
#define TIMESTAMP_FORMAT "%04d-%02d-%02dT%02d:%02d:%02d.%06dZ"
Received on 2015-02-10 19:20:45 CET