You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Alexander Thomas <al...@collab.net> on 2005/07/05 13:38:54 UTC
Re: [PATCH] issue#2317 - Problem with Peg Revisions
for filescontaining '@' - V2
Hey Julian,
Welcome Back :)
On Tue, 2005-07-05 at 14:53 +0100, Julian Foad wrote:
> Alexander Thomas wrote:
> > [[[
> > Version 2: Fix for issue 2317 - Problem with Peg Revisions for files
> > containing '@'.
>
> Please state here what the patch does, and don't state here (in the log
> message) that it is "Version 2".
Sorry, Version 2 of the patch attached
[[[
Fix for issue 2317 - Problem with Peg Revisions for files containing
'@'.
* subversion/libsvn_subr/opt.c
(svn_opt_parse_path): Modified.
* subversion/tests/clients/cmdline/basic_tests.py
(basic_peg_revision): New test.
(test_list): Added basic_peg_revision to list.
]]]
-Alexander Thomas (AT)
Re: [PATCH] issue#2317 - Problem with Peg Revisions for filescontaining '@' - V2
Posted by "C. Michael Pilato" <cm...@collab.net>.
Alexander Thomas <al...@collab.net> writes:
> Sorry, Version 2 of the patch attached
>
> [[[
> Fix for issue 2317 - Problem with Peg Revisions for files containing
> '@'.
>
> * subversion/libsvn_subr/opt.c
> (svn_opt_parse_path): Modified.
Modified how?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue#2317 - Problem with Peg Revisions for filescontaining '@' - V2
Posted by kf...@collab.net.
Julian Foad <ju...@btopenworld.com> writes:
> kfogel@collab.net wrote:
> > Alexander, perhaps this meta-explanation will help with log messages:
> > The intended audience is a Subversion developer who is not familiar
> > with the change. Usually when someone goes back and reads a change
> > (which happens surprisingly frequently), they no longer have in their
> > head all the context around that change. They've forgotten the mails
> > about it, the issue, the code discussions, etc. It could be six
> > months later and they're porting it to a release branch and need to
> > review it, or something like that.
> > Your log message is the introduction to the change. Write it for an
> > experienced Subversion developer who is starting with no particular
> > knowledge of what this change is about, but who knows Subversion in
> > general.
>
> Karl, you are good at saying these kinds of things. It would be nice
> to be able to point people at some existing explanation each time,
> rather than trying to re-explain it each time. Therefore could you
> perhaps add the above to the "Writing log messages" secion of HACKING?
Okay, I've tried to do this in r15270. Further tweaks welcome!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue#2317 - Problem with Peg Revisions for filescontaining
'@' - V2
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
> Alexander, perhaps this meta-explanation will help with log messages:
>
> The intended audience is a Subversion developer who is not familiar
> with the change. Usually when someone goes back and reads a change
> (which happens surprisingly frequently), they no longer have in their
> head all the context around that change. They've forgotten the mails
> about it, the issue, the code discussions, etc. It could be six
> months later and they're porting it to a release branch and need to
> review it, or something like that.
>
> Your log message is the introduction to the change. Write it for an
> experienced Subversion developer who is starting with no particular
> knowledge of what this change is about, but who knows Subversion in
> general.
Karl, you are good at saying these kinds of things. It would be nice to be
able to point people at some existing explanation each time, rather than trying
to re-explain it each time. Therefore could you perhaps add the above to the
"Writing log messages" secion of HACKING?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue#2317 - Problem with Peg Revisions for filescontaining '@' - V2
Posted by kf...@collab.net.
Julian Foad <ju...@btopenworld.com> writes:
> Alexander Thomas wrote:
> > Hey Julian,
> > Welcome Back :)
>
> Thanks.
>
> > On Tue, 2005-07-05 at 14:53 +0100, Julian Foad wrote:
> >>Alexander Thomas wrote:
> >>>[[[
> >>>Version 2: Fix for issue 2317 - Problem with Peg Revisions for files
> >>>containing '@'.
> >>
> >> Please state here what the patch does, and don't state here (in the
> >> log message) that it is "Version 2".
> > Sorry, Version 2 of the patch attached
> > [[[
> > Fix for issue 2317 - Problem with Peg Revisions for files containing
> > '@'.
>
> Um, when I said "state what the patch does" I meant that you should
> write a sentence or two describing the effect of this patch - in other
> words, how the behaviour of Subversion after this patch differs from
> the behaviour before the patch. If this were a simple bug with a
> single, obvious solution, that would be unnecessary, but it's not.
Alexander, perhaps this meta-explanation will help with log messages:
The intended audience is a Subversion developer who is not familiar
with the change. Usually when someone goes back and reads a change
(which happens surprisingly frequently), they no longer have in their
head all the context around that change. They've forgotten the mails
about it, the issue, the code discussions, etc. It could be six
months later and they're porting it to a release branch and need to
review it, or something like that.
Your log message is the introduction to the change. Write it for an
experienced Subversion developer who is starting with no particular
knowledge of what this change is about, but who knows Subversion in
general.
Hope this helps,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue#2317 - Problem with Peg Revisions for filescontaining
'@' - V2
Posted by Julian Foad <ju...@btopenworld.com>.
Alexander Thomas wrote:
> Hey Julian,
> Welcome Back :)
Thanks.
> On Tue, 2005-07-05 at 14:53 +0100, Julian Foad wrote:
>>Alexander Thomas wrote:
>>>[[[
>>>Version 2: Fix for issue 2317 - Problem with Peg Revisions for files
>>>containing '@'.
>>
>>Please state here what the patch does, and don't state here (in the log
>>message) that it is "Version 2".
>
> Sorry, Version 2 of the patch attached
>
> [[[
> Fix for issue 2317 - Problem with Peg Revisions for files containing
> '@'.
Um, when I said "state what the patch does" I meant that you should write a
sentence or two describing the effect of this patch - in other words, how the
behaviour of Subversion after this patch differs from the behaviour before the
patch. If this were a simple bug with a single, obvious solution, that would
be unnecessary, but it's not.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue#2317 - Peg Revision and filename containing '@'
sign - V3
Posted by Alexander Thomas <al...@collab.net>.
On Thu, 2005-07-07 at 13:49 -0500, kfogel@collab.net wrote:
>
> By the way, I think you didn't run the entire 'make check' on this
> change :-). It causes subversion/tests/libsvn_subr/opt-test to fail:
>
Yes you are right. Bit lazy when it comes to writing and running
tests :).
Thanks Karl, for review and commit.
Regards,
-Alexander Thomas (AT)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue#2317 - Peg Revision and filename containing '@' sign - V3
Posted by kf...@collab.net.
Alexander Thomas <al...@collab.net> writes:
> [[[
> Fix issue #2317: Handles filenames containing '@' for svn commands
> supporting peg revision.
>
> * subversion/libsvn_subr/opt.c
> (svn_opt_parse_path): Translates empty suffix '@' sign in
> filename to '@HEAD' or '@BASE' depending on URL or PATH respectively.
>
> * subversion/tests/clients/cmdline/basic_tests.py
> (basic_peg_revision): New test.
> (test_list): Added basic_peg_revision to list.
> ]]]
> Fix issue #2317: Handles filenames containing '@' for svn commands
> supporting peg revision.
>
> * subversion/libsvn_subr/opt.c
> (svn_opt_parse_path): Translates empty suffix '@' sign in
> filename to '@HEAD' or '@BASE' depending on URL or PATH respectively.
>
> * subversion/tests/clients/cmdline/basic_tests.py
> (basic_peg_revision): New test.
> (test_list): Added basic_peg_revision to list.
Please include the log message exactly once. It can be in the body of
the message (using "[[[ ... ]]]"), or it can be an attachment, or
wherever -- just as long as it's not duplicated.
> Index: subversion/libsvn_subr/opt.c
> ===================================================================
> --- subversion/libsvn_subr/opt.c (revision 15206)
> +++ subversion/libsvn_subr/opt.c (working copy)
> @@ -498,13 +498,37 @@
>
> if (path[i] == '@')
> {
> + svn_boolean_t is_url;
> + int ret;
> svn_opt_revision_t start_revision, end_revision;
>
> end_revision.kind = svn_opt_revision_unspecified;
> - if (svn_opt_parse_revision (&start_revision,
> - &end_revision,
> - path + i + 1, pool)
> - || end_revision.kind != svn_opt_revision_unspecified)
> +
> + /* URLs and wc-paths get treated differently. */
> + is_url = svn_path_is_url (path);
Say instead "URLs get treated different from wc paths" to avoid
ambiguity. (Otherwise, it might mean "URLs and wc paths get treated
different from some other thing").
> +
> + /* If empty peg-rev was attached to URL target, then assume HEAD. */
> + if (path [i + 1] == '\0' && is_url)
> + {
> + ret = svn_opt_parse_revision (&start_revision,
> + &end_revision,
> + "head", pool);
> + }
> + /* If empty peg-rev was attached to PATH target, then assume BASE. */
> + else if (path [i + 1] == '\0' && !is_url)
Say "path[i + 1]"; we don't put a space before an array bracket.
> + {
> + ret = svn_opt_parse_revision (&start_revision,
> + &end_revision,
> + "base", pool);
> + }
> + else
> + {
> + ret = svn_opt_parse_revision (&start_revision,
> + &end_revision,
> + path + i + 1, pool);
> + }
To me it felt weird to test path[i + 1] twice. I'd write instead
if (path[i + 1] == '\0')
{
if (is_url)
...;
else /* wc path */
...;
}
else
{
...;
}
It's not a big deal -- it won't make any noticeable difference at
run-time, it's just a question of reading flow, really.
> +
> + if (ret || end_revision.kind != svn_opt_revision_unspecified)
> return svn_error_createf (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> _("Syntax error parsing revision '%s'"),
> path + i + 1);
> Index: subversion/tests/clients/cmdline/basic_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/basic_tests.py (revision 15206)
> +++ subversion/tests/clients/cmdline/basic_tests.py (working copy)
> @@ -1584,6 +1584,30 @@
> "lambda"))
> check_repos_root(output)
>
> +def basic_peg_revision(sbox):
> + "checks peg revision on filename with @ sign"
> +
> + sbox.build()
> + wc_dir = sbox.wc_dir
> + repos_dir = sbox.repo_url
> + filename = 'abc@abc'
> +
> + wc_file = wc_dir + '/' + filename
> + repos_file = repos_dir + '/' + filename
Typically we call it 'repos_url' or 'url' or something, instead of
'repos_file'. No big deal, just alerting you to the convention.
> +
> + svntest.main.file_append(wc_file, 'A probe into the secrets about the Solar System')
Please keep lines under 80 columns, as per HACKING.
> + svntest.main.run_svn(None, 'add', wc_file)
> + svntest.main.run_svn(None, 'ci', '-m', 'secret log msg', wc_file)
> +
> + output, errlines = svntest.main.run_svn(0, 'cat', wc_file+'@')
> + if len(output) < 1:
> + raise svntest.Failure
> +
> + output, errlines = svntest.main.run_svn(0, 'cat', repos_file+'@')
> + if len(output) < 1:
> + raise svntest.Failure
Better to use svntest.actions.run_and_verify_svn() -- this sort of
thing is exactly what it's for.
Also, it would be good to test the failure case:
$ svn cat abc@abc
subversion/libsvn_subr/opt.c:508: (apr_err=205000)
svn: Syntax error parsing revision 'abc'
$
By the way, I think you didn't run the entire 'make check' on this
change :-). It causes subversion/tests/libsvn_subr/opt-test to fail:
START: opt-test
subversion/tests/libsvn_subr/opt-test.c:83: (apr_err=200006)
svn_tests: svn_opt_parse_path ('foo/bar@') returned 'foo/bar' \
instead of 'NULL'
FAIL: lt-opt-test 1: test svn_opt_parse_path
END: opt-test
Obviously your code is correct; it's the test that needs to be adjusted.
> #----------------------------------------------------------------------
> ########################################################################
> # Run the tests
> @@ -1617,6 +1641,7 @@
> basic_add_local_ignores,
> basic_add_no_ignores,
> repos_root,
> + basic_peg_revision,
> ### todo: more tests needed:
> ### test "svn rm http://some_url"
> ### not sure this file is the right place, though.
I've made all the above changes, and committed in r15289. Thanks!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue#2317 - Peg Revision and filename containing '@'
sign - V3
Posted by Alexander Thomas <al...@collab.net>.
[[[
Fix issue #2317: Handles filenames containing '@' for svn commands
supporting peg revision.
* subversion/libsvn_subr/opt.c
(svn_opt_parse_path): Translates empty suffix '@' sign in
filename to '@HEAD' or '@BASE' depending on URL or PATH respectively.
* subversion/tests/clients/cmdline/basic_tests.py
(basic_peg_revision): New test.
(test_list): Added basic_peg_revision to list.
]]]