You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Hyrum K. Wright" <hy...@mail.utexas.edu> on 2007/02/20 18:22:40 UTC
[PATCH] Peg revision support for svn:externals
I've added support for peg revision syntax to svn:externals. It seemed
pretty straight forward, I just want to make sure that I haven't missed
anything. I'd appreciate any review.
If I don't hear anything, I'll commit it in a day or two.
-Hyrum
[[[
Support peg revision syntax with svn:externals. This requires rev'ing
the svn_wc_external_item_t structure, and related functions.
* subversion/include/svn_wc.h
(svn_wc_external_item2_t, svn_wc_external_item2_dup): New.
(svn_wc_external_item_t, svn_wc_external_item_dup): Deprecated.
(svn_wc_parse_externals_description3): New.
(svn_wc_parse_externals_description2): Deprecated.
* subversion/libsvn_wc/props.c
(svn_wc_parse_externals_description3): New. Return an array of
svn_wc_external_item2_t structures, and include parsing of the peg
revisions.
(svn_wc_parse_externals_description2): Implement as a thin wrapper
around svn_wc_parse_externals_description3().
* subversion/libsvn_wc/util.c
(svn_wc_external_item2_dup): New.
* subversion/libsvn_wc/status.c
(get_dir_status): Update to use the new externals API and type.
* subversion/libsvn_client/externals.c
(compare_external_items, svn_client__do_external_status):
Update to use svn_wc_external_item2_t.
(switch_external): Add a peg revision paramter, and use it when
checking out the external module.
(handle_external_item_change): Use the supplied peg revision when
exporting or checking out an external module.
(handle_externals_desc_change): Update to use the new externals API
and type.
* subversion/tests/cmdline/externals_tests.py
(externals_test_setup): Use peg revision syntax when setting up the
working copies.
(update_receive_new_external, update_lose_external,
update_change_pristine_external, update_change_modified_external,
modify_and_update_receive_new_external): Use peg revision syntax.
(external_with_peg_and_op_revision): New test.
(test_list): Run the new test.
]]]
Re: [PATCH] Peg revision support for svn:externals
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
C. Michael Pilato wrote:
> Hyrum K. Wright wrote:
>> Well, after playing around with that, and running into a few deadends,
>> another thought occurred to me. Instead of inventing a new syntax, why
>> don't we just set the peg revision equal to the provided revision if no
>> peg revision exists, and apply the normal resolution rules when a peg
>> revision is given? This would maintain backward compatibility, without
>> having to introduce another syntax.
>
> I considered that, too, but I thought the whole point was to get externals'
> syntax to match that of 'svn checkout'? At least, that's always been a goal
> of *mine*. By making 'URL -rN' mean one thing (URL@N -rN) for externals and
> another (URL@HEAD -rN) elsewhere, you've introduced a subtle bit of
> inconsistency that merely complicates things for users.
Good point, and that inconsistency would be a Bad Thing. Scratch that
idea. I've got the "-rN URL@M PATH" syntax finished, and I'll commit it
later today.
-Hyrum
Re: [PATCH] Peg revision support for svn:externals
Posted by "C. Michael Pilato" <cm...@collab.net>.
Hyrum K. Wright wrote:
> Well, after playing around with that, and running into a few deadends,
> another thought occurred to me. Instead of inventing a new syntax, why
> don't we just set the peg revision equal to the provided revision if no
> peg revision exists, and apply the normal resolution rules when a peg
> revision is given? This would maintain backward compatibility, without
> having to introduce another syntax.
I considered that, too, but I thought the whole point was to get externals'
syntax to match that of 'svn checkout'? At least, that's always been a goal
of *mine*. By making 'URL -rN' mean one thing (URL@N -rN) for externals and
another (URL@HEAD -rN) elsewhere, you've introduced a subtle bit of
inconsistency that merely complicates things for users.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] Peg revision support for svn:externals
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Hyrum K. Wright wrote:
> Michael Sinz wrote:
>> On 2/28/07, C. Michael Pilato <cm...@collab.net> wrote:
>>> Hyrum K. Wright wrote:
>>>> Does anybody have any other comments on the backwards compatibility
>>>> implications here? I'm leaning toward the "this is just a little too
>>>> much breakage" side of the things (though I could be convinced
>>> otherwise
>>>> :), and I'm planning on putting this in the issue tracker for 2.0.
>>> What if we did the following (WARNING, I'm in a weird mood):
>>>
>>> I almost always mess up the setting of the svn:externals property the
>>> first
>>> time because the order of the "arguments"...
>>>
>>> target-dir [-rM] http://.../path/to/checkout
>>>
>>> ...is exactly the opposite of the typical checkout command-line
>>> syntax. So
>>> what if we declared a new version of the syntax, that looked like this:
>>>
>>> http://.../path/to/checkout[@PEG-REV] [-rM] target-dir
>>>
>>> (or maybe)
>>>
>>> [-rM] http://.../path/to/checkout[@PEG-REV] target-dir
>> I like this one as it is obvious as to it being different (options first)
>> plus it very much matches the "usual" svn command line order.
>>
>> Anyway, the point is that a) you'd be able to detect the new format using
>>> svn_path_is_url() checks on the arguments, and b) now folks can verify
>>> their
>>> externals definitions by tacking them onto the end of 'svn checkout' at
>>> the
>>> command-line.
>>>
>>> Thoughts? Tomatoes?
>>
>> Actually, no tomatoes but a nice, large apple :-) To me, at least, this
>> would allow for backwards compatibility with older externals. The only
>> issue left is what this does to older clients. If it fails "cleanly" (no
>> pollution of the WC) then that is reasonable. I guess that it requires
>> some
>> testing to validate that.
>
> Thanks for the comments!
>
> My 1.3.2 client errors out cleanly on both of CMike's suggestions, but
> in different ways. Using the first syntax, I get an error when
> updating, but when using the second syntax, I get an error when
> attempting to set the svn:external property.
>
> I like the second error mode better, because it prevents somebody from
> setting a bogus property that their client can't understand. They may
> still pull one down as part of an update from some other client (which I
> haven't tested), but at least it eliminates their ability to set it locally.
>
> Although, I prefer the first suggestion syntactically, it leaves junk
> laying around in the WC after it fails. I'm going to go ahead and
> implement the second version.
Well, after playing around with that, and running into a few deadends,
another thought occurred to me. Instead of inventing a new syntax, why
don't we just set the peg revision equal to the provided revision if no
peg revision exists, and apply the normal resolution rules when a peg
revision is given? This would maintain backward compatibility, without
having to introduce another syntax.
-Hyrum
Re: [PATCH] Peg revision support for svn:externals
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Michael Sinz wrote:
> On 2/28/07, C. Michael Pilato <cm...@collab.net> wrote:
>>
>> Hyrum K. Wright wrote:
>> >
>> > Does anybody have any other comments on the backwards compatibility
>> > implications here? I'm leaning toward the "this is just a little too
>> > much breakage" side of the things (though I could be convinced
>> otherwise
>> > :), and I'm planning on putting this in the issue tracker for 2.0.
>>
>> What if we did the following (WARNING, I'm in a weird mood):
>>
>> I almost always mess up the setting of the svn:externals property the
>> first
>> time because the order of the "arguments"...
>>
>> target-dir [-rM] http://.../path/to/checkout
>>
>> ...is exactly the opposite of the typical checkout command-line
>> syntax. So
>> what if we declared a new version of the syntax, that looked like this:
>>
>> http://.../path/to/checkout[@PEG-REV] [-rM] target-dir
>>
>> (or maybe)
>>
>> [-rM] http://.../path/to/checkout[@PEG-REV] target-dir
>
> I like this one as it is obvious as to it being different (options first)
> plus it very much matches the "usual" svn command line order.
>
> Anyway, the point is that a) you'd be able to detect the new format using
>> svn_path_is_url() checks on the arguments, and b) now folks can verify
>> their
>> externals definitions by tacking them onto the end of 'svn checkout' at
>> the
>> command-line.
>>
>> Thoughts? Tomatoes?
>
>
> Actually, no tomatoes but a nice, large apple :-) To me, at least, this
> would allow for backwards compatibility with older externals. The only
> issue left is what this does to older clients. If it fails "cleanly" (no
> pollution of the WC) then that is reasonable. I guess that it requires
> some
> testing to validate that.
Thanks for the comments!
My 1.3.2 client errors out cleanly on both of CMike's suggestions, but
in different ways. Using the first syntax, I get an error when
updating, but when using the second syntax, I get an error when
attempting to set the svn:external property.
I like the second error mode better, because it prevents somebody from
setting a bogus property that their client can't understand. They may
still pull one down as part of an update from some other client (which I
haven't tested), but at least it eliminates their ability to set it locally.
Although, I prefer the first suggestion syntactically, it leaves junk
laying around in the WC after it fails. I'm going to go ahead and
implement the second version.
-Hyrum
Re: [PATCH] Peg revision support for svn:externals
Posted by Michael Sinz <Mi...@sinz.org>.
On 2/28/07, C. Michael Pilato <cm...@collab.net> wrote:
>
> Hyrum K. Wright wrote:
> >
> > Does anybody have any other comments on the backwards compatibility
> > implications here? I'm leaning toward the "this is just a little too
> > much breakage" side of the things (though I could be convinced otherwise
> > :), and I'm planning on putting this in the issue tracker for 2.0.
>
> What if we did the following (WARNING, I'm in a weird mood):
>
> I almost always mess up the setting of the svn:externals property the
> first
> time because the order of the "arguments"...
>
> target-dir [-rM] http://.../path/to/checkout
>
> ...is exactly the opposite of the typical checkout command-line
> syntax. So
> what if we declared a new version of the syntax, that looked like this:
>
> http://.../path/to/checkout[@PEG-REV] [-rM] target-dir
>
> (or maybe)
>
> [-rM] http://.../path/to/checkout[@PEG-REV] target-dir
I like this one as it is obvious as to it being different (options first)
plus it very much matches the "usual" svn command line order.
Anyway, the point is that a) you'd be able to detect the new format using
> svn_path_is_url() checks on the arguments, and b) now folks can verify
> their
> externals definitions by tacking them onto the end of 'svn checkout' at
> the
> command-line.
>
> Thoughts? Tomatoes?
Actually, no tomatoes but a nice, large apple :-) To me, at least, this
would allow for backwards compatibility with older externals. The only
issue left is what this does to older clients. If it fails "cleanly" (no
pollution of the WC) then that is reasonable. I guess that it requires some
testing to validate that.
--
Michael Sinz Technology and Engineering Director/Consultant
"Starting Startups" mailto:Michael.Sinz@sinz.org
My place on the web http://www.sinz.org/Michael.Sinz
Re: [PATCH] Peg revision support for svn:externals
Posted by "C. Michael Pilato" <cm...@collab.net>.
Hyrum K. Wright wrote:
> Hyrum K. Wright wrote:
>> C. Michael Pilato wrote:
>>> Hyrum K. Wright wrote:
>>>> Vlad Georgescu wrote:
>>>>> The old code used the revision specified by -r as both a peg rev and an
>>>>> op rev, but now svn_opt_resolve_revisions will set the peg rev to HEAD
>>>>> for all existing externals definitions, which I think is a
>>>>> backward-incompatible change.
>>>> That is correct. There is precedent for doing this, though. We made
>>>> the same change when adding peg revision syntax to commands, such as
>>>> 'svn copy' and 'svn log'.
>>> This line of thought feels incomplete to me.
>> Ah, sorry to be a bit terse. Let me elaborate.
>>
>> My reference comes from implementing peg revision support for 'svn
>> copy'. After doing so, I noticed that some of the tests started
>> breaking, ones that were attempting to resurrect old versions of deleted
>> files. 'svn cp -r 1 deleted_file new_file' failed because new_file
>> wasn't available in HEAD.
>>
>> When I mentioned this on irc, the response was, in not so many words,
>> "the original meaning of -r 1 is broken, people should really be using
>> @1 if they want to refer to something that is different than it is in
>> HEAD." I updated the tests and moved on.
>>
>>> Yes, users can change their behaviors at the command-line to take advantage
>>> of (or otherwise deal with) a new syntax. But they *can't* easily modify
>>> their version history and tweak old svn:externals values such that they
>>> continue to work after an upgrade of their Subversion client. Then again,
>>> one could argue that they'd have equal trouble retroactively fixing
>>> versioned build scripts and such that made use of the pre-peg-rev
>>> command-line syntax.
>> Thanks for pointing that out; I hadn't considered *old* svn:externals
>> properties that may be affected by this change.
>>
>> Adding peg revision support to svn:externals is an important part to
>> maintaining consistency across the Subversion interface, as well as
>> adding the flexibility that peg revisions provide. But if the
>> compatibility ramifications are too large, I'm happy to file this one in
>> the issue tracker for 2.0, and move on.
>>
>>> Is this an apples-to-apples comparison, or is there passion fruit in our midst?
>> I suspect that there is a bit of apple-passion-mango involved. :)
>
> Does anybody have any other comments on the backwards compatibility
> implications here? I'm leaning toward the "this is just a little too
> much breakage" side of the things (though I could be convinced otherwise
> :), and I'm planning on putting this in the issue tracker for 2.0.
What if we did the following (WARNING, I'm in a weird mood):
I almost always mess up the setting of the svn:externals property the first
time because the order of the "arguments"...
target-dir [-rM] http://.../path/to/checkout
...is exactly the opposite of the typical checkout command-line syntax. So
what if we declared a new version of the syntax, that looked like this:
http://.../path/to/checkout[@PEG-REV] [-rM] target-dir
(or maybe)
[-rM] http://.../path/to/checkout[@PEG-REV] target-dir
Anyway, the point is that a) you'd be able to detect the new format using
svn_path_is_url() checks on the arguments, and b) now folks can verify their
externals definitions by tacking them onto the end of 'svn checkout' at the
command-line.
Thoughts? Tomatoes?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] Peg revision support for svn:externals
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Hyrum K. Wright wrote:
> C. Michael Pilato wrote:
>> Hyrum K. Wright wrote:
>>> Vlad Georgescu wrote:
>>>> The old code used the revision specified by -r as both a peg rev and an
>>>> op rev, but now svn_opt_resolve_revisions will set the peg rev to HEAD
>>>> for all existing externals definitions, which I think is a
>>>> backward-incompatible change.
>>> That is correct. There is precedent for doing this, though. We made
>>> the same change when adding peg revision syntax to commands, such as
>>> 'svn copy' and 'svn log'.
>> This line of thought feels incomplete to me.
>
> Ah, sorry to be a bit terse. Let me elaborate.
>
> My reference comes from implementing peg revision support for 'svn
> copy'. After doing so, I noticed that some of the tests started
> breaking, ones that were attempting to resurrect old versions of deleted
> files. 'svn cp -r 1 deleted_file new_file' failed because new_file
> wasn't available in HEAD.
>
> When I mentioned this on irc, the response was, in not so many words,
> "the original meaning of -r 1 is broken, people should really be using
> @1 if they want to refer to something that is different than it is in
> HEAD." I updated the tests and moved on.
>
>> Yes, users can change their behaviors at the command-line to take advantage
>> of (or otherwise deal with) a new syntax. But they *can't* easily modify
>> their version history and tweak old svn:externals values such that they
>> continue to work after an upgrade of their Subversion client. Then again,
>> one could argue that they'd have equal trouble retroactively fixing
>> versioned build scripts and such that made use of the pre-peg-rev
>> command-line syntax.
>
> Thanks for pointing that out; I hadn't considered *old* svn:externals
> properties that may be affected by this change.
>
> Adding peg revision support to svn:externals is an important part to
> maintaining consistency across the Subversion interface, as well as
> adding the flexibility that peg revisions provide. But if the
> compatibility ramifications are too large, I'm happy to file this one in
> the issue tracker for 2.0, and move on.
>
>> Is this an apples-to-apples comparison, or is there passion fruit in our midst?
>
> I suspect that there is a bit of apple-passion-mango involved. :)
Does anybody have any other comments on the backwards compatibility
implications here? I'm leaning toward the "this is just a little too
much breakage" side of the things (though I could be convinced otherwise
:), and I'm planning on putting this in the issue tracker for 2.0.
Thoughts?
-Hyrum
Re: [PATCH] Peg revision support for svn:externals
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
C. Michael Pilato wrote:
> Hyrum K. Wright wrote:
>> Vlad Georgescu wrote:
>>> The old code used the revision specified by -r as both a peg rev and an
>>> op rev, but now svn_opt_resolve_revisions will set the peg rev to HEAD
>>> for all existing externals definitions, which I think is a
>>> backward-incompatible change.
>> That is correct. There is precedent for doing this, though. We made
>> the same change when adding peg revision syntax to commands, such as
>> 'svn copy' and 'svn log'.
>
> This line of thought feels incomplete to me.
Ah, sorry to be a bit terse. Let me elaborate.
My reference comes from implementing peg revision support for 'svn
copy'. After doing so, I noticed that some of the tests started
breaking, ones that were attempting to resurrect old versions of deleted
files. 'svn cp -r 1 deleted_file new_file' failed because new_file
wasn't available in HEAD.
When I mentioned this on irc, the response was, in not so many words,
"the original meaning of -r 1 is broken, people should really be using
@1 if they want to refer to something that is different than it is in
HEAD." I updated the tests and moved on.
> Yes, users can change their behaviors at the command-line to take advantage
> of (or otherwise deal with) a new syntax. But they *can't* easily modify
> their version history and tweak old svn:externals values such that they
> continue to work after an upgrade of their Subversion client. Then again,
> one could argue that they'd have equal trouble retroactively fixing
> versioned build scripts and such that made use of the pre-peg-rev
> command-line syntax.
Thanks for pointing that out; I hadn't considered *old* svn:externals
properties that may be affected by this change.
Adding peg revision support to svn:externals is an important part to
maintaining consistency across the Subversion interface, as well as
adding the flexibility that peg revisions provide. But if the
compatibility ramifications are too large, I'm happy to file this one in
the issue tracker for 2.0, and move on.
> Is this an apples-to-apples comparison, or is there passion fruit in our midst?
I suspect that there is a bit of apple-passion-mango involved. :)
-Hyrum
Re: [PATCH] Peg revision support for svn:externals
Posted by "C. Michael Pilato" <cm...@collab.net>.
Hyrum K. Wright wrote:
> Vlad Georgescu wrote:
>> The old code used the revision specified by -r as both a peg rev and an
>> op rev, but now svn_opt_resolve_revisions will set the peg rev to HEAD
>> for all existing externals definitions, which I think is a
>> backward-incompatible change.
>
> That is correct. There is precedent for doing this, though. We made
> the same change when adding peg revision syntax to commands, such as
> 'svn copy' and 'svn log'.
This line of thought feels incomplete to me.
Yes, users can change their behaviors at the command-line to take advantage
of (or otherwise deal with) a new syntax. But they *can't* easily modify
their version history and tweak old svn:externals values such that they
continue to work after an upgrade of their Subversion client. Then again,
one could argue that they'd have equal trouble retroactively fixing
versioned build scripts and such that made use of the pre-peg-rev
command-line syntax.
Is this an apples-to-apples comparison, or is there passion fruit in our midst?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] Peg revision support for svn:externals
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Vlad Georgescu wrote:
> Hi,
>
> Hyrum K. Wright wrote:
>> I've added support for peg revision syntax to svn:externals. It seemed
>> pretty straight forward, I just want to make sure that I haven't missed
>> anything. I'd appreciate any review.
>>
>> If I don't hear anything, I'll commit it in a day or two.
>>
> ...
>> + SVN_ERR(svn_opt_parse_path(&item->peg_revision, &item->url, item->url,
>> + pool));
>> + svn_opt_resolve_revisions(&item->peg_revision, &item->revision, TRUE,
>> + FALSE);
>> +
>> item->url = svn_path_canonicalize(item->url, pool);
>>
>> if (externals_p)
>> - APR_ARRAY_PUSH(*externals_p, svn_wc_external_item_t *) = item;
>> + APR_ARRAY_PUSH(*externals_p, svn_wc_external_item2_t *) = item;
>> }
>>
> ...
>> /* ... Hello, new hotness. */
>> - SVN_ERR(svn_client__checkout_internal(NULL, url, path, revision, revision,
>> + SVN_ERR(svn_client__checkout_internal(NULL, url, path, peg_revision, revision,
>> TRUE, FALSE, FALSE, timestamp_sleep,
>> ctx, pool));
>>
>
> The old code used the revision specified by -r as both a peg rev and an
> op rev, but now svn_opt_resolve_revisions will set the peg rev to HEAD
> for all existing externals definitions, which I think is a
> backward-incompatible change.
That is correct. There is precedent for doing this, though. We made
the same change when adding peg revision syntax to commands, such as
'svn copy' and 'svn log'.
-Hyrum
Re: [PATCH] Peg revision support for svn:externals
Posted by Vlad Georgescu <vg...@gmail.com>.
Hi,
Hyrum K. Wright wrote:
> I've added support for peg revision syntax to svn:externals. It seemed
> pretty straight forward, I just want to make sure that I haven't missed
> anything. I'd appreciate any review.
>
> If I don't hear anything, I'll commit it in a day or two.
>
...
> + SVN_ERR(svn_opt_parse_path(&item->peg_revision, &item->url, item->url,
> + pool));
> + svn_opt_resolve_revisions(&item->peg_revision, &item->revision, TRUE,
> + FALSE);
> +
> item->url = svn_path_canonicalize(item->url, pool);
>
> if (externals_p)
> - APR_ARRAY_PUSH(*externals_p, svn_wc_external_item_t *) = item;
> + APR_ARRAY_PUSH(*externals_p, svn_wc_external_item2_t *) = item;
> }
>
...
> /* ... Hello, new hotness. */
> - SVN_ERR(svn_client__checkout_internal(NULL, url, path, revision, revision,
> + SVN_ERR(svn_client__checkout_internal(NULL, url, path, peg_revision, revision,
> TRUE, FALSE, FALSE, timestamp_sleep,
> ctx, pool));
>
The old code used the revision specified by -r as both a peg rev and an
op rev, but now svn_opt_resolve_revisions will set the peg rev to HEAD
for all existing externals definitions, which I think is a
backward-incompatible change.
--
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org