How to preserve err->apr_err error codes over the wire in ra_dav?
In order to properly use the svn_ra_change_rev_prop2() API (on the 'atomic-revprop') branch, I'd like to make it return a specific error code (over all RA layers) when the propchange fails due to the provided expected value not being the found value. (That's kinda the point of the new API.) To do so, I'd like to have mod_dav_svn transmit the error code it saw to ra_neon/ra_serf; currently it swallows the error code and I don't see yet how to unswallow it. I see from the code that this should be possible using and tags, but I don't see yet how. Over all RA layers, I get the following error message: "revprop 'flower' has unexpected value in filesystem" Over ra_local and ra_svn, the error chain contains the original error code (SVN_ERR_BAD_PROPERTY_VALUE, 125005): [[[ % ./subversion/tests/cmdline/atomic-ra-revprop-change file://$PWD/subversion/tests/cmdline/svn-test-work/repositories/prop_tests-34 0 flower "( 11 old_value_p 11 wrong value 5 value 6 violet )" neon 1 subversion/libsvn_repos/fs-wrap.c:320: (apr_err=125005) subversion/libsvn_fs_fs/fs_fs.c:609: (apr_err=125005) subversion/libsvn_fs_fs/fs_fs.c:609: (apr_err=125005) subversion/libsvn_fs_fs/fs_fs.c:7263: (apr_err=125005) atomic-ra-revprop-change: revprop 'flower' has unexpected value in filesystem zsh: exit 1 ./subversion/tests/cmdline/atomic-ra-revprop-change 0 flower neon 1 ]]] [[[ % ./subversion/tests/cmdline/atomic-ra-revprop-change svn://localhost/svn-test-work/repositories/prop_tests-34 0 flower "( 11 old_value_p 11 wrong value 5 value 6 violet )" neon 1 subversion/libsvn_ra_svn/client.c:855: (apr_err=21) subversion/svnserve/serve.c:1013: (apr_err=21) atomic-ra-revprop-change: Special code for wrapping server errors to report to client subversion/libsvn_repos/fs-wrap.c:320: (apr_err=125005) atomic-ra-revprop-change: revprop 'flower' has unexpected value in filesystem subversion/libsvn_fs_fs/fs_fs.c:609: (apr_err=125005) atomic-ra-revprop-change: revprop 'flower' has unexpected value in filesystem subversion/libsvn_fs_fs/fs_fs.c:609: (apr_err=125005) atomic-ra-revprop-change: revprop 'flower' has unexpected value in filesystem subversion/libsvn_fs_fs/fs_fs.c:7263: (apr_err=125005) atomic-ra-revprop-change: revprop 'flower' has unexpected value in filesystem zsh: exit 1 ./subversion/tests/cmdline/atomic-ra-revprop-change 0 flower neon 1 ]]] That's good. However, over ra_neon and ra_serf, while the the error chain contains the original message text, the only error codes available are SVN_ERR_RA_DAV_REQUEST_FAILED and SVN_ERR_RA_DAV_PROPPATCH_FAILED: [[[ % ./subversion/tests/cmdline/atomic-ra-revprop-change http://localhost:8081/trunk1/svn-test-work/repositories/prop_tests-34 0 flower "( 11 old_value_p 11 wrong value 5 value 6 violet )" neon 1 subversion/tests/cmdline/atomic-ra-revprop-change.c:199: (apr_err=175002) subversion/libsvn_ra_neon/fetch.c:1209: (apr_err=175002) atomic-ra-revprop-change: DAV request failed; it's possible that the repository's pre-revprop-change hook either failed or is non-existent subversion/libsvn_ra_neon/props.c:1231: (apr_err=175008) atomic-ra-revprop-change: At least one property change failed; repository is unchanged subversion/libsvn_ra_neon/util.c:1506: (apr_err=175002) subversion/libsvn_ra_neon/util.c:212: (apr_err=175002) atomic-ra-revprop-change: Error setting property 'flower': revprop 'flower' has unexpected value in filesystem zsh: exit 1 ./subversion/tests/cmdline/atomic-ra-revprop-change 0 flower neon 1 ]]] [[[ % ./subversion/tests/cmdline/atomic-ra-revprop-change http://localhost:8081/trunk1/svn-test-work/repositories/prop_tests-34 0 flower "( 11 old_value_p 11 wrong value 5 value 6 violet )" serf 1 subversion/tests/cmdline/atomic-ra-revprop-change.c:199: (apr_err=175002) subversion/libsvn_ra_serf/commit.c:2352: (apr_err=175002) atomic-ra-revprop-change: DAV request failed; it's possible that the repository's pre-revprop-change hook either failed or is non-existent subversion/libsvn_ra_serf/commit.c:229: (apr_err=175008) atomic-ra-revprop-change: At least one property change failed; repository is unchanged subversion/libsvn_ra_serf/util.c:1041: (apr_err=175002) atomic-ra-revprop-change: revprop 'flower' has unexpected value in filesystem subversion/libsvn_ra_serf/commit.c:229: (apr_err=175002) atomic-ra-revprop-change: PROPPATCH of '/trunk1/svn-test-work/repositories/prop_tests-34/!svn/rev/0': 207 Multi-Status zsh: exit 1 ./subversion/tests/cmdline/atomic-ra-revprop-change 0 flower serf 1 ]]] Is it possible to make mod_dav_svn allow another (specific) apr_err error code to be passed over the wire to the client? I'd like the error chain on the ra_neon/ra_serf side to contain SVN_ERR_BAD_PROPERTY_VALUE (125005). Looking at `egrep 'apr_err = |svn_error_create' libsvn_ra_neon/*`[1] tells me that the answer should involve ELEM_svn_error (svn:error) and ELEM_human_readable (). Hints, please? Thanks, Daniel [1] li
Re: svn commit: r983269 - in /subversion/branches/atomic-revprop: ./ notes/http-and-webdav/ subversion/include/private/ subversion/libsvn_ra_neon/ subversion/libsvn_ra_serf/ subversion/mod_dav_svn/
danie...@apache.org wrote on Sat, Aug 07, 2010 at 17:45:39 -:
> Author: danielsh
> Date: Sat Aug 7 17:45:38 2010
> New Revision: 983269
>
> URL: http://svn.apache.org/viewvc?rev=983269&view=rev
> Log:
> On the 'atomic-revprop' branch:
>
> Implement the API over ra_dav.
>
I'll appreciate some extra eyes on this revision.
Thanks.
Daniel
(reasons for this request: at the end)
>
> First, some general infrastructure:
>
> * notes/http-and-webdav/webdav-protocol
> (PROPPATCH): Document the protocol extension.
>
> * subversion/include/private/svn_dav_protocol.h
> (SVN_DAV__OLD_VALUE, SVN_DAV__OLD_VALUE__ABSENT): New macros.
> (svn_dav__two_props_t): New helper typedef.
>
>
> Then, mod_dav_svn support:
>
> * subversion/mod_dav_svn/deadprops.c
> (save_value):
> Grow and use an OLD_VALUE_P parameter, passing it
> to svn_repos_fs_change_rev_prop4().
> (decode_property_value):
> Grow an ABSENT out parameter, and parse the (new) 'V:absent' attribute.
> (db_store):
> Parse and 'V:absent' and pass OLD_VALUE_P to save_value().
>
>
> ra_serf support:
>
> * subversion/libsvn_ra_serf/ra_serf.h
> (svn_ra_serf__walk_all_props, svn_ra_serf__set_ver_prop):
> Remove "not implemented" markers from docstrings.
>
> * subversion/libsvn_ra_serf/property.c
> (svn_ra_serf__set_ver_prop):
> Store an svn_dav__two_props_t in the hash if an OLD_VALUE_P was provided.
> (svn_ra_serf__walk_all_props):
> Support the VALUES_ARE_PROPPAIRS parameter when calling the walker.
>
> * subversion/libsvn_ra_serf/commit.c
> (proppatch_context_t): Add 'atomic_props' member.
> (get_encoding_and_cdata):
> Refactor to return a string rather than a bucket,
> and to support a NULL value.
> (proppatch_walker):
> Track get_encoding_and_cdata() signature change.
> Set 'V:absent' and as appropriate.
> (create_proppatch_body):
> Walk/Iterate CTX->ATOMIC_PROPS too.
> (svn_ra_serf__change_rev_prop):
> Remove 'not implemented' sentinel. Initialize CTX->ATOMIC_PROPS.
> Pass OLD_VALUE_P to svn_ra_serf__set_prop() (which passes it
> to svn_ra_serf__set_ver_prop()).
>
>
> ra_neon support:
>
> * subversion/libsvn_ra_neon/ra_neon.h
> (svn_ra_neon__do_proppatch):
> Add PROP_OLD_VALUES parameter.
>
> * subversion/libsvn_ra_neon/fetch.c
> (svn_ra_neon__change_rev_prop):
> Remove 'not implemented' sentinel.
> Pass OLD_VALUE_P to svn_ra_neon__do_proppatch() via PROP_OLD_VALUES.
>
> * subversion/libsvn_ra_neon/props.c
> (append_setprop):
> Grow OLD_VALUE_P parameter. Use it to
> generate 'V:absent' and as appropriate.
> (svn_ra_neon__do_proppatch):
> Grow PROP_OLD_VALUES parameter, and use it.
>
>
> * subversion/libsvn_ra_neon/commit.c
> (do_proppatch, apply_revprops):
> Track signature change of svn_ra_neon__do_proppatch().
>
>
> Finally:
>
> * BRANCH-README: Mark this as done.
>
> Modified:
> subversion/branches/atomic-revprop/BRANCH-README
> subversion/branches/atomic-revprop/notes/http-and-webdav/webdav-protocol
>
> subversion/branches/atomic-revprop/subversion/include/private/svn_dav_protocol.h
> subversion/branches/atomic-revprop/subversion/libsvn_ra_neon/commit.c
> subversion/branches/atomic-revprop/subversion/libsvn_ra_neon/fetch.c
> subversion/branches/atomic-revprop/subversion/libsvn_ra_neon/props.c
> subversion/branches/atomic-revprop/subversion/libsvn_ra_neon/ra_neon.h
> subversion/branches/atomic-revprop/subversion/libsvn_ra_serf/commit.c
> subversion/branches/atomic-revprop/subversion/libsvn_ra_serf/property.c
> subversion/branches/atomic-revprop/subversion/libsvn_ra_serf/ra_serf.h
> subversion/branches/atomic-revprop/subversion/mod_dav_svn/deadprops.c
>
> Modified: subversion/branches/atomic-revprop/BRANCH-README
> URL:
> http://svn.apache.org/viewvc/subversion/branches/atomic-revprop/BRANCH-README?rev=983269&r1=983268&r2=983269&view=diff
> ==
> --- subversion/branches/atomic-revprop/BRANCH-README (original)
> +++ subversion/branches/atomic-revprop/BRANCH-README Sat Aug 7 17:45:38 2010
> @@ -21,11 +21,7 @@ Planned work
>achieved with older servers, and because of planned protocol
> extensions)
>- [DONE] ra_local: Trivial.
>- [DONE] ra_svn: Add a 'change-rev-prop2' verb.
> - - ra_dav
> - Extend PROPPATCH syntax to provide the old value: add a
> - tag inside the <${propname}> tag.
> -Q: where to document this protocol extension?
> -A: notes/http-and-webdav/webdav-protocol
> + - [DONE] ra_dav: Extend PROPPATCH: see
> notes/http-and-webdav/webdav-protocol
>- [DONE] unit test
>prop_tests.py 34: atomic_over_ra()
>- TODO: extend it to test props set to the empty string and unset props
>
> Modified:
> subversion/branches/atomic-revprop/notes/http-and-webdav/webdav-protocol
> URL:
> http://svn.apache.org/viewvc/subversi
Re: Bikeshed: configuration override order
On Sat, 2010-08-07 at 07:58 -0400, Daniel Shahaf wrote: > Stefan Küng wrote on Sat, Aug 07, 2010 at 12:59:26 +0200: > > On 07.08.2010 12:44, Daniel Shahaf wrote: > >> If corporations want to have configuration override, fine. > >> > >> But I want a way to disable that completely. I don't necessarily want to > >> allow a random sourceforge repository to control my auto-props settings for > >> a wc of that repository. > > > > Maybe a stupid question: why not? > > Why don't I let ezmlm configure my mailer's "use html?" setting? I think he was asking for an answer specifically relating to auto-props, not an answer about configuration in general. There's not generally a lot of room for individual disagreement about what auto-props should be for a given project. My thinking about repository configuration is that the uses cases should be divided into two categories: 1. Stuff that isn't really client configuration at all, like auto-props, should come from the repository instead, and should only come from client configuration for compatibility. 2. Stuff that is client configuration should only come from client configuration. Client control is not legitimate business for an open source product, though it could be the business of a proprietary value-added fork. Note that there's no general extension of the config framework here, no whitelisting or blacklisting, no override settings. Invent a mechanism for getting repository configuration from the server and apply it to the specific use cases in (1), falling back to client configuration as a legacy mechanism.
Re: svn commit: r983222 - /subversion/trunk/subversion/svnrdump/load_editor.c
On 07.08.2010 16:32, Ramkumar Ramachandra wrote: > Hi Daniel, > > Daniel Shahaf writes: > >> artag...@apache.org wrote on Sat, Aug 07, 2010 at 12:31:50 -: >> >>> Author: artagnon >>> Date: Sat Aug 7 12:31:50 2010 >>> New Revision: 983222 >>> >>> URL: http://svn.apache.org/viewvc?rev=983222&view=rev >>> Log: >>> svnrdump: Fix a bug in the load_editor; it was unable to handle >>> revisions without node information previously. >>> >>> * subversion/svnrdump/load_editor.c >>> (close_revision): Add a new if-branch; if the commit_editor doesn't >>> exist, create one, open_root and close_edit on it to indicate that >>> we've finished processing the revision. While at it, also fix indentation. >>> >> >> ^^ >> >> I take it you haven't seen my previous commit review yet? >> > The trade-off is the creation of many trivial commits :) > We do have a long-standing preference to not mix functional and stylistic changes in the same commit. It's even documented in Hacking. -- Brane
Re: svn commit: r983096 - in /subversion/trunk/subversion/svnrdump: load_editor.c load_editor.h
Ramkumar Ramachandra wrote on Sat, Aug 07, 2010 at 20:00:28 +0530:
> Hi Daniel,
>
> Daniel Shahaf writes:
> > Ramkumar, I've noticed before in your patches that they tend to have
> > multiple separable parts, and this commit is a good opportunity to explain:
>
> Thanks for the review.
>
> > >if (strcmp(hname, SVN_REPOS_DUMPFILE_NODE_COPYFROM_REV) == 0)
> > > nb->copyfrom_rev = atoi(hval);
> > >if (strcmp(hname, SVN_REPOS_DUMPFILE_NODE_COPYFROM_PATH) == 0)
> > > - nb->copyfrom_path =
> > > -svn_path_url_add_component2(rb->pb->root_url,
> > > -apr_pstrdup(rb->pool, hval),
> > > -rb->pool);
> > > +nb->copyfrom_path =
> > > + svn_path_url_add_component2(rb->pb->root_url,
> > > + apr_pstrdup(rb->pool, hval),
> > > + rb->pool);
> >
> > Whitespace-only change; should have been done in a separate commit. (Don't
> > mix functional and non-functional change in the same commit.)
>
> Yeah, this is a stray change.
>
I've grown the habit of reviewing the 'svn diff' before committing.
(I have a Vim function to do that, in fact.)
> > The line reordering makes it harder to spot the functional change done here:
> > passing nb->base_checksum instead of NULL.
(this)
>
> Right. Should have probably been in two separate commits - I get the
> point now.
>
> > While here, how is this change related to the svn_node_action_delete bug?
> > (The log message doesn't say.) Are the checksum-related changes and the
> > delete-related changes logically part of one patch/bugfix? Or could you
> > have committed them as two separate patches?
> >
> > >return SVN_NO_ERROR;
> > > }
> > > @@ -429,8 +419,8 @@ close_node(void *baton)
> > >
> > >if (nb->kind == svn_node_file)
> > > {
> > > - SVN_ERR(commit_editor->close_file(nb->file_baton, NULL,
> > > nb->rb->pool));
> > >LDR_DBG(("Closing file %p\n", nb->file_baton));
> > > + SVN_ERR(commit_editor->close_file(nb->file_baton, NULL,
> > > nb->rb->pool));
> >
> > This actually *is* a functional change (which is not mentioned in the log
> > message), but should have been in a separate commit.
(and this)
>
> Oh, I thought "changed tense of LDR_DBG messages" would suffice. Have
> to be more explicit/ careful.
>
The marked two points of mine may have been more nit-picky than normal; but
it's hard for me to explain exactly what I mean here (without getting into
long paragraphs of trivialities). There's a balance to be made, and I think
this commit was on the wrong side thereof, but we certainly don't go all the
way to the other extreme either (e.g., it's okay to combine several
non-functional changes).
Your best bet is to review your patches more carefully before committing
them, and seeing how others do it. With ~50k revisions in our history, you
have plenty of examples to follow :-)
> -- Ram
Re: svn commit: r983222 - /subversion/trunk/subversion/svnrdump/load_editor.c
Hi Daniel, Daniel Shahaf writes: > artag...@apache.org wrote on Sat, Aug 07, 2010 at 12:31:50 -: > > Author: artagnon > > Date: Sat Aug 7 12:31:50 2010 > > New Revision: 983222 > > > > URL: http://svn.apache.org/viewvc?rev=983222&view=rev > > Log: > > svnrdump: Fix a bug in the load_editor; it was unable to handle > > revisions without node information previously. > > > > * subversion/svnrdump/load_editor.c > > (close_revision): Add a new if-branch; if the commit_editor doesn't > > exist, create one, open_root and close_edit on it to indicate that > > we've finished processing the revision. While at it, also fix indentation. > ^^ > > I take it you haven't seen my previous commit review yet? The trade-off is the creation of many trivial commits :) -- Ram
Re: svn commit: r983096 - in /subversion/trunk/subversion/svnrdump: load_editor.c load_editor.h
Hi Daniel,
Daniel Shahaf writes:
> Ramkumar, I've noticed before in your patches that they tend to have
> multiple separable parts, and this commit is a good opportunity to explain:
Thanks for the review.
> >if (strcmp(hname, SVN_REPOS_DUMPFILE_NODE_COPYFROM_REV) == 0)
> > nb->copyfrom_rev = atoi(hval);
> >if (strcmp(hname, SVN_REPOS_DUMPFILE_NODE_COPYFROM_PATH) == 0)
> > - nb->copyfrom_path =
> > -svn_path_url_add_component2(rb->pb->root_url,
> > -apr_pstrdup(rb->pool, hval),
> > -rb->pool);
> > +nb->copyfrom_path =
> > + svn_path_url_add_component2(rb->pb->root_url,
> > + apr_pstrdup(rb->pool, hval),
> > + rb->pool);
>
> Whitespace-only change; should have been done in a separate commit. (Don't
> mix functional and non-functional change in the same commit.)
Yeah, this is a stray change.
> > }
> >
> >if (svn_path_compare_paths(svn_relpath_dirname(nb->path, pool),
> > @@ -252,14 +254,14 @@ new_node_record(void **node_baton,
> >nb->copyfrom_path,
> >nb->copyfrom_rev,
> >rb->pool, &(nb->file_baton)));
> > - LDR_DBG(("Adding file %s to dir %p as %p\n", nb->path,
> > rb->db->baton, nb->file_baton));
> > + LDR_DBG(("Added file %s to dir %p as %p\n", nb->path,
> > rb->db->baton, nb->file_baton));
>
> Non-functional change; should have been in a separate commit (if at all).
Right, I probably shouldn't do more than one thing at once. I'll try
to commit more often then at the risk of making many trivial commits.
> >break;
> > case svn_node_action_replace:
> >/* Absent in dumpstream; represented as a delete + add */
> > @@ -411,9 +401,9 @@ apply_textdelta(svn_txdelta_window_handl
> >nb = node_baton;
> >commit_editor = nb->rb->pb->commit_editor;
> >pool = nb->rb->pool;
> > - SVN_ERR(commit_editor->apply_textdelta(nb->file_baton, NULL /*
> > base_checksum */,
> > - pool, handler, handler_baton));
> >LDR_DBG(("Applying textdelta to %p\n", nb->file_baton));
> > + SVN_ERR(commit_editor->apply_textdelta(nb->file_baton, nb->base_checksum,
> > + pool, handler, handler_baton));
> >
>
> The line reordering makes it harder to spot the functional change done here:
> passing nb->base_checksum instead of NULL.
Right. Should have probably been in two separate commits - I get the
point now.
> While here, how is this change related to the svn_node_action_delete bug?
> (The log message doesn't say.) Are the checksum-related changes and the
> delete-related changes logically part of one patch/bugfix? Or could you
> have committed them as two separate patches?
>
> >return SVN_NO_ERROR;
> > }
> > @@ -429,8 +419,8 @@ close_node(void *baton)
> >
> >if (nb->kind == svn_node_file)
> > {
> > - SVN_ERR(commit_editor->close_file(nb->file_baton, NULL,
> > nb->rb->pool));
> >LDR_DBG(("Closing file %p\n", nb->file_baton));
> > + SVN_ERR(commit_editor->close_file(nb->file_baton, NULL,
> > nb->rb->pool));
>
> This actually *is* a functional change (which is not mentioned in the log
> message), but should have been in a separate commit.
Oh, I thought "changed tense of LDR_DBG messages" would suffice. Have
to be more explicit/ careful.
-- Ram
Re: svn commit: r983222 - /subversion/trunk/subversion/svnrdump/load_editor.c
artag...@apache.org wrote on Sat, Aug 07, 2010 at 12:31:50 -:
> Author: artagnon
> Date: Sat Aug 7 12:31:50 2010
> New Revision: 983222
>
> URL: http://svn.apache.org/viewvc?rev=983222&view=rev
> Log:
> svnrdump: Fix a bug in the load_editor; it was unable to handle
> revisions without node information previously.
>
> * subversion/svnrdump/load_editor.c
> (close_revision): Add a new if-branch; if the commit_editor doesn't
> exist, create one, open_root and close_edit on it to indicate that
> we've finished processing the revision. While at it, also fix indentation.
^^
I take it you haven't seen my previous commit review yet?
>
> Modified:
> subversion/trunk/subversion/svnrdump/load_editor.c
>
> Modified: subversion/trunk/subversion/svnrdump/load_editor.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/svnrdump/load_editor.c?rev=983222&r1=983221&r2=983222&view=diff
> ==
> --- subversion/trunk/subversion/svnrdump/load_editor.c (original)
> +++ subversion/trunk/subversion/svnrdump/load_editor.c Sat Aug 7 12:31:50
> 2010
> @@ -434,26 +434,43 @@ close_revision(void *baton)
> - else {
> -/* Close all pending open directories, and then close the edit
> - session itself */
> -while (rb->db && rb->db->parent)
> - {
> -LDR_DBG(("Closing dir %p\n", rb->db->baton));
> -SVN_ERR(commit_editor->close_directory(rb->db->baton, rb->pool));
> -rb->db = rb->db->parent;
> - }
> -LDR_DBG(("Closing edit on %p\n", commit_edit_baton));
> -SVN_ERR(commit_editor->close_edit(commit_edit_baton, rb->pool));
> - }
> + else if (commit_editor)
> +{
> + /* Close all pending open directories, and then close the edit
> + session itself */
> + while (rb->db && rb->db->parent)
> +{
> + LDR_DBG(("Closing dir %p\n", rb->db->baton));
> + SVN_ERR(commit_editor->close_directory(rb->db->baton, rb->pool));
> + rb->db = rb->db->parent;
> +}
> + LDR_DBG(("Closing edit on %p\n", commit_edit_baton));
> + SVN_ERR(commit_editor->close_edit(commit_edit_baton, rb->pool));
> +}
Re: svn commit: r983096 - in /subversion/trunk/subversion/svnrdump: load_editor.c load_editor.h
Ramkumar, I've noticed before in your patches that they tend to have
multiple separable parts, and this commit is a good opportunity to explain:
artag...@apache.org wrote on Fri, Aug 06, 2010 at 19:10:20 -:
> Author: artagnon
> Date: Fri Aug 6 19:10:20 2010
> New Revision: 983096
>
> URL: http://svn.apache.org/viewvc?rev=983096&view=rev
> Log:
> svnrdump: Fix a bug involving svn_node_action_delete
>
> * subversion/svnrdump/load_editor.c
> (new_node_record, apply_textdelta, close_node): Fix the tense of the
> LDR_DBG messages.
> (new_node_record): Extract base_checksum header. Also, no Node-Kind
> information is present during a svn_node_action_delete, so don't
> look for it and blindly delete the given entry.
> (apply_textdelta): Use the extracted base_checksum while applying
> the textdelta.
>
> * subversion/svnrdump/load_editor.h
> (node_baton): Add a new base_checksum field.
>
> Modified:
> subversion/trunk/subversion/svnrdump/load_editor.c
> subversion/trunk/subversion/svnrdump/load_editor.h
>
> Modified: subversion/trunk/subversion/svnrdump/load_editor.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/svnrdump/load_editor.c?rev=983096&r1=983095&r2=983096&view=diff
> ==
> --- subversion/trunk/subversion/svnrdump/load_editor.c (original)
> +++ subversion/trunk/subversion/svnrdump/load_editor.c Fri Aug 6 19:10:20
> 2010
> @@ -189,13 +189,15 @@ new_node_record(void **node_baton,
>if (strcmp(hval, "replace") == 0)
> nb->action = svn_node_action_replace;
> }
> + if (strcmp(hname, SVN_REPOS_DUMPFILE_TEXT_DELTA_BASE_MD5) == 0)
> +nb->base_checksum = apr_pstrdup(rb->pool, hval);
This part is okay.
>if (strcmp(hname, SVN_REPOS_DUMPFILE_NODE_COPYFROM_REV) == 0)
> nb->copyfrom_rev = atoi(hval);
>if (strcmp(hname, SVN_REPOS_DUMPFILE_NODE_COPYFROM_PATH) == 0)
> - nb->copyfrom_path =
> -svn_path_url_add_component2(rb->pb->root_url,
> -apr_pstrdup(rb->pool, hval),
> -rb->pool);
> +nb->copyfrom_path =
> + svn_path_url_add_component2(rb->pb->root_url,
> + apr_pstrdup(rb->pool, hval),
> + rb->pool);
Whitespace-only change; should have been done in a separate commit. (Don't
mix functional and non-functional change in the same commit.)
> }
>
>if (svn_path_compare_paths(svn_relpath_dirname(nb->path, pool),
> @@ -252,14 +254,14 @@ new_node_record(void **node_baton,
>nb->copyfrom_path,
>nb->copyfrom_rev,
>rb->pool, &(nb->file_baton)));
> - LDR_DBG(("Adding file %s to dir %p as %p\n", nb->path,
> rb->db->baton, nb->file_baton));
> + LDR_DBG(("Added file %s to dir %p as %p\n", nb->path,
> rb->db->baton, nb->file_baton));
Non-functional change; should have been in a separate commit (if at all).
> @@ -288,21 +290,9 @@ new_node_record(void **node_baton,
> }
>break;
> case svn_node_action_delete:
> - switch (nb->kind)
> -{
> -case svn_node_file:
> - LDR_DBG(("Deleting file %s in %p\n", nb->path, rb->db->baton));
> - SVN_ERR(commit_editor->delete_entry(nb->path, rb->rev,
> - rb->db->baton, rb->pool));
> - break;
> -case svn_node_dir:
> - LDR_DBG(("Deleting dir %s in %p\n", nb->path, rb->db->baton));
> - SVN_ERR(commit_editor->delete_entry(nb->path, rb->rev,
> - rb->db->baton, rb->pool));
> - break;
> -default:
> - break;
> -}
> + LDR_DBG(("Deleting entry %s in %p\n", nb->path, rb->db->baton));
> + SVN_ERR(commit_editor->delete_entry(nb->path, rb->rev,
> + rb->db->baton, rb->pool));
Okay.
>break;
> case svn_node_action_replace:
>/* Absent in dumpstream; represented as a delete + add */
> @@ -411,9 +401,9 @@ apply_textdelta(svn_txdelta_window_handl
>nb = node_baton;
>commit_editor = nb->rb->pb->commit_editor;
>pool = nb->rb->pool;
> - SVN_ERR(commit_editor->apply_textdelta(nb->file_baton, NULL /*
> base_checksum */,
> - pool, handler, handler_baton));
>LDR_DBG(("Applying textdelta to %p\n", nb->file_baton));
> + SVN_ERR(commit_editor->apply_textdelta(nb->file_baton, nb->base_checksum,
> + pool, handler, handler_baton));
>
The line reordering makes it harder to spot the functional change done here:
passing nb->base_checksum instead of NULL.
While here, how is this change related to the svn
Re: Bikeshed: configuration override order
Stefan Küng wrote on Sat, Aug 07, 2010 at 12:59:26 +0200: > On 07.08.2010 12:44, Daniel Shahaf wrote: >> If corporations want to have configuration override, fine. >> >> But I want a way to disable that completely. I don't necessarily want to >> allow a random sourceforge repository to control my auto-props settings for >> a wc of that repository. > > Maybe a stupid question: why not? Why don't I let ezmlm configure my mailer's "use html?" setting? If I run with use-server-config=ask, I might accept the prompt most of the time. But, ultimately, it's software I run on my computer, and I should have the final say over what configuration it runs with.[1] I suggest the following: * one can say "don't honor the server's suggestions" without recompiling the client * one cannot make the client dishonor the suggestions *while reporting that it shall honor them* without recompiling. This way the server can rely on the self-report (and could even refuse the checkout if the self-report says "I will dishonor"). > I mean, if the developers of that project agree on certain rules and > decide to enforce them, shouldn't you also follow those rules if you use I agree with stsp's point elsethread: if it's truly project configuration, it doesn't belong in ~/.subversion/. > that repository/wc? Especially if you have commit access there, it would > be very bad if you would commit something that would break those rules. > No, it won't be "very bad". It will mean I have to make another commit to fix the broken auto-props. > Stefan > [1] That's why we parse ~/.subversion/ *after* /etc/subversion/ and why mailers can be configured to prompt before honoring the Reply-To header.
Re: svn_wc_translated_file3
On 07.08.2010 12:52, Daniel Shahaf wrote: What's the solution? I'm using svn_wc_get_pristine_copy_path() and disable the deprecated warnings for that API call. I won't get a translated file that way, but most if not all UI diff tools have options to ignore EOLs. And if keywords aren't expanded, well I guess users can live with those lines showing up as different when doing such diffs. Using svn_wc_get_pristine_contents() instead of svn_wc_get_pristine_copy_path() is not an option for me here because I can't pass a stream to an external diff application. Which means I would have to first create a copy of that file (i.e., save the stream), which defeats the whole purpose of doing a *fast* diff. This leads me to another request: would it be possible to 'un-deprecate' the svn_wc_get_pristine_copy_path() API? Not a big deal of course, I can use this deprecated API just fine. Stefan -- ___ oo // \\ "De Chelonian Mobile" (_,\/ \_/ \ TortoiseSVN \ \_/_\_/>The coolest Interface to (Sub)Version Control /_/ \_\ http://tortoisesvn.net
Re: Bikeshed: configuration override order
On 07.08.2010 12:44, Daniel Shahaf wrote: If corporations want to have configuration override, fine. But I want a way to disable that completely. I don't necessarily want to allow a random sourceforge repository to control my auto-props settings for a wc of that repository. Maybe a stupid question: why not? I mean, if the developers of that project agree on certain rules and decide to enforce them, shouldn't you also follow those rules if you use that repository/wc? Especially if you have commit access there, it would be very bad if you would commit something that would break those rules. Stefan -- ___ oo // \\ "De Chelonian Mobile" (_,\/ \_/ \ TortoiseSVN \ \_/_\_/>The coolest Interface to (Sub)Version Control /_/ \_\ http://tortoisesvn.net
Re: svn_wc_translated_file3
What's the solution? Stefan Küng wrote on Fri, Aug 06, 2010 at 23:56:37 +0200: > Never mind - I found a solution to the problem. > > Stefan > > On Fri, Aug 6, 2010 at 21:42, Stefan Küng wrote: > > Hi, > > > > Just noticed that the API svn_wc_translated_file3() has been removed in > > r963632: > > > > Message: > > Remove the public libsvn_wc file and stream translation APIs (while leaving > > their deprecated predecessors). These APIs are not consumed by the client. > > See: http://svn.haxx.se/dev/archive-2010-07/0100.shtml > > > > > > While I can understand the reasons for removing it, I still need some way to > > get done what this API did: get a file representing the WC BASE in a > > translated format. Sure I could use the stream API but that way I will > > always have to read from the stream and write it to a file. The removed API > > was very fast if no translation was necessary because it simply returned the > > existing BASE file path. > > > > As for what I need that API in TSVN: to do a quick diff between WC and BASE > > of files. For example, every time a user doubleclicks on a modified file in > > the commit dialog, TSVN calls that API to get the BASE file. > > > > As for using svn_wc_translated_file2(), that one requires an > > svn_wc_adm_access_t as a parameter which I can only get using deprecated > > APIs. > > > > Suggestions on what I should do? > > > > Stefan
Re: RFC: WCNG and Issue #2915: Merge tracking and missing subtrees due to non-svn removal
Paul Burba wrote on Fri, Aug 06, 2010 at 10:30:50 -0400: > As described in issue #2915, in 1.6 when a merge target is a missing > subtree due to its removal by non-svn means, we try to record > mergeinfo such that the missing subtree doesn't have (or inherit) > mergeinfo describing the merge: > > (If you already have a vague idea of how this works you can skip to > 'You might suggest that it makes more sense' below for the RFC) > > ### A file is missing in our merge target > > 1.6.13-dev>svn st > ! A_COPY\D\H\psi > > ### No initial mergeinfo > > 1.6.13-dev>svn pg svn:mergeinfo -vR > > ### Merge tries to apply change to missing file, but can't > ### and reports it as skipped > > 1.6.13-dev>svn merge ^^/A A_COPY -r2:4 > --- Merging r3 through r4 into 'A_COPY': > UA_COPY\D\G\rho > Skipped missing target: 'A_COPY\D\H\psi' > Summary of conflicts: > Skipped paths: 1 > > ### Merge target gets mergeinfo describing the merge > ### performed and the missing file gets empty "override" > ### mergeinfo so it doesn't inherit the target's mergeinfo > > 1.6.13-dev>svn st >M A_COPY > M A_COPY\D\G\rho > !M A_COPY\D\H\psi > > 1.6.13-dev>svn pg svn:mergeinfo -vR > Properties on 'A_COPY\D\H\psi': > svn:mergeinfo > > Properties on 'A_COPY': > svn:mergeinfo > /A:3-4 > > If the missing subtree was a directory we obviously can't set its > properties, so we treat this as a tree conflict: > When I read your "if you already know how it works", I expected that here we'd set non-inheritable mergeinfo on the parent of the missing dir (and then set normal mergeinfo on that dir's siblings). Perhaps there's a very good reason we don't do that --- I'm completely unacquainted with these mergeinfo subtleties --- but I'm just repeating what my intuition said... > 1.6.13-dev>svn st > ! A_COPY\D\H > > 1.6.13-dev>svn merge ^^/A A_COPY -r2:4 > --- Merging r3 through r4 into 'A_COPY': > UA_COPY\D\G\rho > C A_COPY\D\H > Summary of conflicts: > Tree conflicts: 1 > > 1.6.13-dev>svn st >M A_COPY > M A_COPY\D\G\rho > ! C A_COPY\D\H > > local delete, incoming edit upon merge > > ~
Re: Bikeshed: configuration override order
If corporations want to have configuration override, fine.
But I want a way to disable that completely. I don't necessarily want to
allow a random sourceforge repository to control my auto-props settings for
a wc of that repository.
So. We could have an allow-repos-config switch (parallel to the existing
allow-plaintext-passwords switch), which could be boolean or three-value
(yes/no/ask).
Daniel
(And if the switch is disabled but the repos wants to dictate client-side
config anyway? We could refuse the checkout (corp repos) or allow it anyway
with a warning (sourceforge repos).)
Hyrum K. Wright wrote on Fri, Aug 06, 2010 at 13:23:39 -0500:
> On Fri, Aug 6, 2010 at 1:08 PM, C. Michael Pilato wrote:
> > On 08/06/2010 01:50 PM, Hyrum K. Wright wrote:
> >> I'm doing some more thinking about repository-dictated configuration,
> >> and one of the things I'd like some discussion on is the order of
> >> configuration overrides. The consensus is that the repository can not
> >> be sure that it's dictated configuration is received and respected by
> >> the client, so it should treat whatever config it sends as purely
> >> suggestive. We currently have several (implemented or proposed)
> >> sources for configuration information, and I think they should be
> >> searched in the following order:
> >>
> >> * Client site-wide configuration (/etc/subversion)
> >> * Client user-specific configuration (~/subversion, 'svn --config-dir')
> >> * Repository-dictated configuration (as described above)
> >> * Explicit configuration supplied by the client application
> >> ('svn --config-option', or Eclipse configuration options)
> >>
> >> Not every location contains every bit of config, of course, but in the
> >> case of conflicts, the most recent encountered value sticks. In other
> >> words, a client could override repository-dictated configuration
> >> options by using 'svn --config-option', or the (as yet unimplemented)
> >> equivalent facility for other API consumers.
> >>
> >> Thoughts?
> >
> > It seems to me like, if we search the list above in the order presented (as
> > you suggest), we pretty much get the worst possible scenario. Maybe it's a
> > wording thing, though. (I'm thinking search-and-stop-on-first-find ...
> > maybe you mean overlay/overwrite configurations in this order, then search
> > the merged results?)
>
> I mean the latter: read a config, overwrite any previous values,
> repeat. This is how the current system is set up today, I believe (it
> uses apr hashes to store the configuration, and just blindly sets the
> values it finds, leading the most recent found to be the functional
> value).
>
> > Whatever you meant, the corporate customers I've spoken with understand
> > that, so long as they are using open source Subversion clients, anyone can
> > theoretically modify their client to change its behavior. But on the
> > assumption that that is a rare case, they want Subversion to try to treat
> > the repos-dictated config as more than merely suggestive. In other words,
> > they want to be able to reasonably expect that in order to get behavior that
> > opposes the repos-dictated configuration, the user *must* have modified
> > their client or used a client that doesn't honor Subversion's design in this
> > respect.
> >
> > In the past, I've proposed the idea of two repos-dictated configuration
> > sets: one that has the ultimately authority and cannot be overridden
> > without library changes, one that sits in about the slot you've described
> > above.
>
> I dunno. I think that may be a bit too complex for a first pass, and
> I think that adding a requirement to hack the library doesn't really
> add any value, other than obfuscation. The obfuscated approach may
> fool some people, but if they really want to override, they are going
> to do it. I'd rather accomplish that via the parsing order than
> hacking the library (having to specify 'svn --config-option', seems
> like a reasonable barrier).
>
> I just wrote that and then read Greg Hudson's mail elsethread, which
> makes me wonder if we need three piles:
> * always override from server
> * possibly override from server
> * never override from server
>
> -Hyrum
Re: Bikeshed: configuration override order
Stefan Sperling wrote on Fri, Aug 06, 2010 at 21:51:37 +0200: > Why do we have settings in the individual client config that all users > would want to share anyway? > > So maybe having auto-props in the config is part of the problem? What about > having an svn:auto-props property at the parent directory, specifying > autoconf settings for its direct children? Agreed. > Too inconvenient to set up due to lack of recursive property support? > Would 'svn propset -R' do to configure auto-props for an entire subtree > like '/trunk'? Or do we want something like "this is a branch root" property? Or do we want some "look in parent" value for svn:auto-props? for i in $subdirs_of_trunk; do svn propset --symlink svn:auto-props ../ $i done (and then, what happens if you checkout a proper subfolder of trunk --- e.g., ^/trunk/subversion --- without checkouting its parent, and attempt a propset? Not unsolvable, but needs to be thought of.) > Once those properties were set up and configured, users > could mostly forget about auto-props unless the default auto-props need > to be changed. Not whenever they start working on a different client > computer, as they do today. Yes.
