Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Subject: Re: [PATCH] Issue #4668: Fixing the node key order during svnadmin
 dump
To: lukeperkins@epicdgs.us
References: <003001d275af$5afca8c0$10f5fa40$@epicdgs.us>
From: Julian Foad <julianfoad@apache.org>
Cc: dev@subversion.apache.org
Message-ID: <d0e1762f-fe75-c47b-0fae-8e17354d019a@apache.org>
Date: Tue, 24 Jan 2017 09:51:52 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.5.1
MIME-Version: 1.0
In-Reply-To: <003001d275af$5afca8c0$10f5fa40$@epicdgs.us>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 8bit
archived-at: Tue, 24 Jan 2017 09:52:02 -0000

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@epicdgs.us)
> ]]]
>
> Thank-you,
>
> Luke Perkins
>