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@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"
