Hi, Luke. Thank you for bringing this to the dev list and, as I
mentioned in private, thank you for taking such a constructive and
helpful attitude to fixing the issue.
Luke Perkins wrote:
> Would this be an acceptable approach to address future direction for dump while still addressing legacy format?
I think something like this, writing the headers in a stable order,
would be welcome.
As I mentioned in the issue, the order of headers isn't by any means the
only non-stable part of the format, but if this helps you in practice, I
don't see any reason not to do it. Stability trumps instability, other
things being equal. (We could potentially add stability to other aspects
of the output later, on top of this.)
> How do we go about getting this checked into the trunk?
After sufficient review and iterations here on the dev list, one of the
committers will commit this with a note "Patch by: <your name & email
address>".
> I have defined a new switch for “svnadmin dump –pre-1.8-dump” to activate the old node key order. I did my best to try to keep the original authors style. Is this an acceptable switch name?
> A new parameter to the primary function, “svn_repos_dump_fs4”, called “svn_boolean_t pre_1_8_dump,”. I have search the source code and I think I have all of the function calls covered. Are there any other considerations?
Every new option we add carries new costs with it -- maintenance,
documentation, user education, more variations to test, etc.
I think there is no significant reason to disable the ordering. Of
course compatibility is always a concern, and we don't want to fix one
use case while breaking another. I suppose it could be that the
supposedly unstable order has actually been coming out stable for some
people, in which case they would potentially benefit by leaving it is as
it -- but that's being too "tricky" -- we should keep it simple by not
adding an option.
Do you have any particular reason why you think the option is necessary?
> What is the verification procedure for implementing this change?
Running the regression test suite with the (rather hidden)
"--dump-load-cross-check" option enabled, plus feedback from yourself
and one or two others that it "works for you".
A couple of questions regarding your patch code itself:
It looks like code for writing an initial subset of headers in a fixed
order was already present, and all you need to do is add more header
names to the array "revision_headers_order" like this:
[[[
Index: subversion/libsvn_repos/dump.c
===================================================================
--- subversion/libsvn_repos/dump.c (revision 1777947)
+++ subversion/libsvn_repos/dump.c (working copy)
@@ -414,12 +414,16 @@ write_revision_headers(svn_stream_t *str
const char **h;
apr_hash_index_t *hi;
static const char *revision_headers_order[] =
{
SVN_REPOS_DUMPFILE_REVISION_NUMBER, /* must be first */
+ SVN_REPOS_DUMPFILE_CONTENT_LENGTH, /* ### really? see below */
+ SVN_REPOS_DUMPFILE_TEXT_CONTENT_LENGTH,
+ SVN_REPOS_DUMPFILE_TEXT_CONTENT_SHA1,
+ SVN_REPOS_DUMPFILE_TEXT_CONTENT_MD5,
NULL
};
/* Write some headers in a given order */
for (h = revision_headers_order; *h; h++)
{
]]]
Do you agree?
In your patch you put CONTENT_LENGTH as the first header. Previously the
code put this as the last header, with a comment saying "Content-length
must be last". Can you comment on that? Was the current code completely
wrong about that? I haven't recently checked the 1.8 code, but I'm sure
I checked it when I last changed that function. (I will need to research
that more fully, but I'd like to know your opinion first.)
Thanks.
- Julian
> [[[
> Fix issue #4668: svnadmin dump node header order has changed
>
> These changes provide a means for the user to output the svnadmin dump using
> the node key
> order used in svnadmin version 1.8 and earlier.
>
> * subversion/include/svn_repos.h
> Added comment regarding the switch usage.
> (svn_repos_dump_fs4): Added Boolean parameter to function prototype called
> pre_1_8_dump
> * subversion/svnadmin/svnadmin.c
> (svnadmin_cmdline_options_t ): Added svnadmin__pre_1_8_dump to enumerated
> definition.
> (options_table): Added pre_1_8_dump definition to the table.
> (cmd_table ): Modified the help usage text to include the pre-1.8-dump
> switch.
> (svnadmin_opt_state ): Added pre_1_8_dump boolean member to structure
> definition.
> (subcommand_dump): Added pre_1_8_dump variable to svn_repos_dump_fs4
> function call.
> (sub_main): Added case supporting svnadmin__pre_1_8_dump value.
> * subversion/libsvn_repos/dump.c
> (write_revision_headers, write_revision_headers_v1651614 ): Renamed original
> write_revision_headers function to write_revision_headers_v1651614.
> (write_revision_headers_svn_4668): Added new function with fixed node key
> ordering as prescribed in the prose of Issue 4668.
> (write_revision_headers): Modified function to switch between two node key
> formatting methods.
> (svn_repos__dump_revision_record, write_revision_record,
> svn_repos_dump_fs4): Added pre_1_8_dump parameter to define the node key
> ordering during svnadmin dump operations.
>
> Patch by: L. Perkins <lukeperkins_at_epicdgs.us)
> ]]]
>
> Thank-you,
>
> Luke Perkins
>
Received on 2017-01-24 10:52:02 CET