Return-Path: X-Original-To: apmail-subversion-dev-archive@minotaur.apache.org Delivered-To: apmail-subversion-dev-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id DE8EE17A11 for ; Wed, 22 Oct 2014 13:41:57 +0000 (UTC) Received: (qmail 73837 invoked by uid 500); 22 Oct 2014 13:41:57 -0000 Delivered-To: apmail-subversion-dev-archive@subversion.apache.org Received: (qmail 73786 invoked by uid 500); 22 Oct 2014 13:41:57 -0000 Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Delivered-To: mailing list dev@subversion.apache.org Received: (qmail 73774 invoked by uid 99); 22 Oct 2014 13:41:56 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 22 Oct 2014 13:41:56 +0000 X-ASF-Spam-Status: No, hits=1.5 required=5.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_LOW,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (nike.apache.org: domain of stefan.fuhrmann@wandisco.com designates 209.85.223.178 as permitted sender) Received: from [209.85.223.178] (HELO mail-ie0-f178.google.com) (209.85.223.178) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 22 Oct 2014 13:41:30 +0000 Received: by mail-ie0-f178.google.com with SMTP id rl12so3432378iec.37 for ; Wed, 22 Oct 2014 06:41:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=wandisco.com; s=gapps; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=GWtb22x5zojHPkTyy6cxtAhvR+K98istXgXRFZFY1MM=; b=h4WmQBiWlHdJlWnVmYO8vrYgy9FPF2ZSkKVDyCnMfKhmHp2pkAu4U375gyULrS/ruk NH54zSod4MlC4OaeFC0rKFlDf+3YPJVuvMH/ywkBRooWleLu2Frz+Jc4/ZfftCNx019N 6vF52c8lBY3si9L6FwRXTKVlMdi7ypxz4c+q4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=GWtb22x5zojHPkTyy6cxtAhvR+K98istXgXRFZFY1MM=; b=hbB9eoLQzCKXfnLAXh1cL1dfeSK95bTtAHjt1bp2DX5OT2LSNJq23wCSUaqZIMaRRJ 031c2wIOKHDy/51l5uNxJ8WdKHmiIEBEzNv1P1jUZvZ+vG3NdKe6kipYDfojvooOsMDQ QfYjHg4W5dRNR0I7HrRSRLBkSg9/dD+VvhmviyJCiyiP/cUnOEntwWtVZGfjs27pv8px tb+GopcVvLn/bACWkTN8pTW/tHlguIEh8XWc5chZ4o4xzYxJ/0kVO+bj3WUGiVukrjxb fhGtQ25wlDsuNp4TlNkNc+nodEp9v9kCz2ibs0XLcvCnR1novvHl8IQdUUCAZFCC7fIo o5/w== X-Gm-Message-State: ALoCoQlLTO0SX0kpH25e5xnHQ971l74WzOlwKB8u8YbGyX9alIG2lzM3aZ9rW/Bb06dX92oddMiu MIME-Version: 1.0 X-Received: by 10.42.66.143 with SMTP id p15mr4569225ici.11.1413985286718; Wed, 22 Oct 2014 06:41:26 -0700 (PDT) Received: by 10.50.224.138 with HTTP; Wed, 22 Oct 2014 06:41:26 -0700 (PDT) In-Reply-To: <1412177727.76619.YahooMailNeo@web87704.mail.ir2.yahoo.com> References: <1412177727.76619.YahooMailNeo@web87704.mail.ir2.yahoo.com> Date: Wed, 22 Oct 2014 15:41:26 +0200 Message-ID: Subject: Re: FSFS changed-paths handling - review From: Stefan Fuhrmann To: Julian Foad Cc: "dev@subversion.apache.org" Content-Type: multipart/alternative; boundary=20cf30223ce9bba7340506031abb X-Virus-Checked: Checked by ClamAV on apache.org --20cf30223ce9bba7340506031abb Content-Type: text/plain; charset=UTF-8 Hey Julian, Belated thanks for the code review! On Wed, Oct 1, 2014 at 5:35 PM, Julian Foad wrote: > I took a look through the FSFS code that deals with the changed-path > lists, with a special interest in how it reports no-op changes. In the > course of this code inspection I found a number of issues that want > clarification and a few possible bugs. I have not tried to produce actual > buggy behaviour from these observations. > > Some of this code is new or changed for format 7, and some is old. > Some of this seems to have been broken for a long time and on all backends. Most of it is masked by the way we drive our editors and the fact the virtually nobody uses the FS API directly. Until r1631047, we didn't even test the svn_fs_paths_changed2 function. I'm thinking of changing the paths_changed API in 1.10 to fix a few conceptual issues. E.g. removing the noderev ID (it is invalid outside the transactions anyway) and using an array instead of a hash since this is an ordered list of operations. > Comments, concerns and suggested code updates together in the form of a > diff: > Feedback is inline: > [[[ > Index: subversion/include/svn_fs.h > =================================================================== > --- subversion/include/svn_fs.h (revision 1628514) > +++ subversion/include/svn_fs.h (working copy) > @@ -1427,38 +1427,61 @@ typedef enum svn_fs_path_change_kind_t > /** Change descriptor. > * > * @note Fields may be added to the end of this structure in future > * versions. Therefore, to preserve binary compatibility, users > * should not directly allocate structures of this type. > * > + * @note The @c text_mod, @c prop_mod and @c mergeinfo_mod flags mean the > + * text, properties and mergeinfo property (respectively) were "touched" > + * by the commit API; this does not mean the new value is different from > + * the old value. > + * > * @since New in 1.6. */ > typedef struct svn_fs_path_change2_t > { > /** node revision id of changed path */ > const svn_fs_id_t *node_rev_id; > > /** kind of change */ > svn_fs_path_change_kind_t change_kind; > > - /** were there text mods? */ > + /** was the text touched? > + * For node_kind=dir: always false. For node_kind=file: > + * modify: true iff text touched. > + * add (copy): true iff text touched. > + * add (plain): always true. > + * delete: always false. > + * replace: as for the add/copy part of the replacement. > + */ > svn_boolean_t text_mod; > > - /** were there property mods? */ > + /** were the properties touched? > + * modify: true iff props touched. > + * add (copy): true iff props touched. > + * add (plain): true iff props touched. > + * delete: always false. > + * replace: as for the add/copy part of the replacement. > + */ > svn_boolean_t prop_mod; > > /** what node kind is the path? > (Note: it is legal for this to be #svn_node_unknown.) */ > svn_node_kind_t node_kind; > > /** Copyfrom revision and path; this is only valid if copyfrom_known > * is true. */ > svn_boolean_t copyfrom_known; > svn_revnum_t copyfrom_rev; > const char *copyfrom_path; > > - /** were there mergeinfo mods? > + /** was the mergeinfo property touched? > + * modify: } true iff svn:mergeinfo property add/del/mod > + * add (copy): } and fs format supports mergeinfo. > Minor correction: The format must support the mergeinfo_mod flag (1.9) not just plain mergeinfo (1.5). > + * add (plain): } > + * delete: always false. > + * replace: as for the add/copy part of the replacement. > * (Note: Pre-1.9 repositories will report #svn_tristate_unknown.) > * @since New in 1.9. */ > svn_tristate_t mergeinfo_mod; > /* NOTE! Please update svn_fs_path_change2_create() when adding new > fields here. */ > } svn_fs_path_change2_t; Docstring changes committed as r1628759. > Index: subversion/libsvn_fs_fs/low_level.c > =================================================================== > --- subversion/libsvn_fs_fs/low_level.c (revision 1628514) > +++ subversion/libsvn_fs_fs/low_level.c (working copy) > @@ -353,12 +353,15 @@ read_change(change_t **change_p, > return svn_error_create(SVN_ERR_FS_CORRUPT, NULL, > _("Invalid prop-mod flag in rev-file")); > } > > /* Get the mergeinfo-mod flag if given. Otherwise, the next thing > is the path starting with a slash. */ > + /* ### Must initialize info->mergeinfo_mod to 'unknown' rather than 0, > + else write_change_entry() would assume it's definitely false. */ > + /* info->mergeinfo_mod = svn_tristate_unknown; */ > Nasty one, masked by the fact that higher level libs did not test for "unknown" but defaulted to it. Fixed in r1628762. > if (*last_str != '/') > { > str = svn_cstring_tokenize(" ", &last_str); > if (str == NULL) > return svn_error_create(SVN_ERR_FS_CORRUPT, NULL, > _("Invalid changes line in rev-file")); > @@ -470,9 +473,10 @@ > > -/* Write a single change entry, path PATH, change CHANGE, and copyfrom > - string COPYFROM, into the file specified by FILE. Only include the > +/* Write a single change entry, path PATH, change CHANGE, to STREAM. > + > + Only include the > Corrected in r1631049. > node kind field if INCLUDE_NODE_KIND is true. Only include the > mergeinfo-mod field if INCLUDE_MERGEINFO_MODS is true. All temporary > allocations are in SCRATCH_POOL. */ > static svn_error_t * > write_change_entry(svn_stream_t *stream, > const char *path, > @@ -563,18 +567,34 @@ svn_fs_fs__write_changes(svn_stream_t *s > apr_pool_t *iterpool = svn_pool_create(scratch_pool); > fs_fs_data_t *ffd = fs->fsap_data; > svn_boolean_t include_node_kinds = > ffd->format >= SVN_FS_FS__MIN_KIND_IN_CHANGED_FORMAT; > svn_boolean_t include_mergeinfo_mods = > ffd->format >= SVN_FS_FS__MIN_MERGEINFO_IN_CHANGES_FORMAT; > + /* ### Rename s/_CHANGES_/_CHANGED_/ to match MIN_KIND_...? */ > Renamed for consistency in r1631185. > apr_array_header_t *sorted_changed_paths; > int i; > > /* For the sake of the repository administrator sort the changes so > that the final file is deterministic and repeatable, however the > rest of the FSFS code doesn't require any particular order here. */ > + > + /* ### This sorting would not be effective if the caller writes changes > + in multiple batches. This sorting would not be safe if CHANGES > + included multiple changes for the same path. > + > + While building up the txn we call this from > svn_fs_fs__add_change(), > + multiple changes per path overall, but only one change at a time. > + While writing the final proto-rev file we call this from > + write_final_changed_path_info() with all the changes at once, but > + only one change per path. > + > + Both of the current callers get safe and effective behaviour, but > + consider sorting in write_final_changed_path_info() instead to > + make all this clearer. (This internal API would have to change.) > + */ > It turns out that leaving the sorting part to the caller is difficult since the existing changes array and hash representation use different structs. We would need to copy & realloc quite a bit, which means a lot of extra code and peak memory usage. In r1631186, I added a comment that explains that this ordering is only partial and why that is the way we want it to be. > sorted_changed_paths = svn_sort__hash(changes, > svn_sort_compare_items_lexically, > scratch_pool); > > /* Write all items to disk in the new order. */ > for (i = 0; i < sorted_changed_paths->nelts; ++i) > Index: subversion/libsvn_fs_fs/transaction.c > =================================================================== > --- subversion/libsvn_fs_fs/transaction.c (revision 1628676) > +++ subversion/libsvn_fs_fs/transaction.c (working copy) > @@ -624,20 +624,29 @@ replace_change > /* Copy the contents of NEW_CHANGE into OLD_CHANGE assuming that both > - belong to the same path. Allocate copies in POOL. > + belong to the same path. Do not copy the change_kind field. > + > + ### For semantic simplicity, I suggest making this copy the *whole* > + contents, and renaming it to 'copy_change' or > 'svn_fs_path_change2_dup' > + if it also allocates a new object (caller adjustment needed). It > + could call memcpy and then duplicate the sub-objects, simplifying > + the implementation too. > + > + Allocate copies in POOL. > r1631115 replaces this function with path_change_dup(). > */ > static void > replace_change(svn_fs_path_change2_t *old_change, > const svn_fs_path_change2_t *new_change, > apr_pool_t *pool) > { > old_change->node_kind = new_change->node_kind; > old_change->node_rev_id = svn_fs_fs__id_copy(new_change->node_rev_id, > pool); > old_change->text_mod = new_change->text_mod; > old_change->prop_mod = new_change->prop_mod; > old_change->mergeinfo_mod = new_change->mergeinfo_mod; > + /* ### What about copyfrom_known? (Is it *always* TRUE?) */ > For FSFS, it implicitly is. Also, see below. > if (new_change->copyfrom_rev == SVN_INVALID_REVNUM) > { > old_change->copyfrom_rev = SVN_INVALID_REVNUM; > old_change->copyfrom_path = NULL; > } > else > @@ -714,18 +723,22 @@ fold_change(apr_hash_t *changed_paths, > > case svn_fs_path_change_delete: > if (old_change->change_kind == svn_fs_path_change_add) > { > /* If the path was introduced in this transaction via an > add, and we are deleting it, just remove the path > - altogether. */ > + altogether. (The caller will delete any child paths.) */ > Part of r1631115. There was another problem lurking here which got fixed in 1631171. Deletions of replacements would report the wrong noderev ID and node type. > old_change = NULL; > } > else > { > - /* A deletion overrules all previous changes. */ > + /* A deletion overrules a previous change (modify/replace). > */ > + /* ### Why not call replace_change(old_change, info, pool); > ? */ > + /* ### What about node_rev_id? Would be wrong after a > replace? */ > + /* ### What about node_kind? Could be wrong after a > replace? */ > + /* ### What about copyfrom_known? (Is it *always* TRUE?) */ > old_change->change_kind = svn_fs_path_change_delete; > old_change->text_mod = info->text_mod; > old_change->prop_mod = info->prop_mod; > old_change->mergeinfo_mod = info->mergeinfo_mod; > old_change->copyfrom_rev = SVN_INVALID_REVNUM; > old_change->copyfrom_path = NULL; > Rewritten as part of r1631115. > @@ -739,12 +752,15 @@ fold_change(apr_hash_t *changed_paths, > replace_change(old_change, info, pool); > old_change->change_kind = svn_fs_path_change_replace; > break; > > case svn_fs_path_change_modify: > default: > + /* If the new change modifies some attribute of the node, set > + the corresponding flag, whether it already was set or not. > + Note: We do not reset a flag to FALSE if a change is undone. > */ > Committed as r1631180. > if (info->text_mod) > old_change->text_mod = TRUE; > if (info->prop_mod) > old_change->prop_mod = TRUE; > if (info->mergeinfo_mod == svn_tristate_true) > old_change->mergeinfo_mod = svn_tristate_true; > @@ -761,12 +777,13 @@ fold_change(apr_hash_t *changed_paths, > structure from the internal one (in the hash's pool), and dup > the path into the hash's pool, too. */ > new_change = apr_pmemdup(pool, info, sizeof(*new_change)); > new_change->node_rev_id = svn_fs_fs__id_copy(info->node_rev_id, > pool); > if (info->copyfrom_path) > new_change->copyfrom_path = apr_pstrdup(pool, > info->copyfrom_path); > + /* ### Use copy_change() or svn_fs_path_change2_dup() instead. */ > Done as part of r1631115. > /* Add this path. The API makes no guarantees that this (new) key > will not be retained. Thus, we copy the key into the target pool > to ensure a proper lifetime. */ > apr_hash_set(changed_paths, > apr_pstrmemdup(pool, path->data, path->len), path->len, > @@ -1585,16 +1602,24 @@ svn_fs_fs__add_change(svn_fs_t *fs, > change->text_mod = text_mod; > change->prop_mod = prop_mod; > change->mergeinfo_mod = mergeinfo_mod > ? svn_tristate_true > : svn_tristate_false; > change->node_kind = node_kind; > + /* ### copyfrom_known remains FALSE, but svn_fs_fs__write_changes() > + ignores it. Should set TRUE for clarity, and also document > + svn_fs_fs__write_changes() as ignoring it? */ > It turns out to be a backend-specific invariant. As of r1631050, we initialize it correctly. > change->copyfrom_rev = copyfrom_rev; > change->copyfrom_path = apr_pstrdup(pool, copyfrom_path); > + /* ### COPYFROM_PATH may be NULL. apr_pstrdup < 1.5 does not promise > + null-safety, although the implementation since 0.9.0 is > null-safe. > + For consistency, should conditionalize it. */ > Changed in r1631051. > svn_hash_sets(changes, path, change); > SVN_ERR(svn_fs_fs__write_changes(svn_stream_from_aprfile2(file, TRUE, > pool), > fs, changes, FALSE, pool)); > + /* ### There is no call (in this context) that sets the 'terminate_list' > + parameter true as required by the doc string. */ > That is o.k. We need to set that flag only if the file does not end with the list. Docstring corrected in r1631049. > return svn_io_file_close(file, pool); > } > > Index: subversion/libsvn_fs_fs/tree.c > =================================================================== > --- subversion/libsvn_fs_fs/tree.c (revision 1628514) > +++ subversion/libsvn_fs_fs/tree.c (working copy) > @@ -1534,13 +1534,13 @@ fs_change_node_prop(svn_fs_root_t *root, > const svn_string_t *value, > apr_pool_t *pool) > { > parent_path_t *parent_path; > apr_hash_t *proplist; > const svn_fs_fs__id_part_t *txn_id; > - svn_boolean_t modeinfo_mod = FALSE; > + svn_boolean_t modeinfo_mod = FALSE; /* ### "mergeinfo_mod"? */ > Yep, modeinfo_mod would be one level too meta. Fixed in r1628764. > if (! root->is_txn_root) > return SVN_FS__NOT_TXN(root); > txn_id = root_txn_id(root); > > path = svn_fs__canonicalize_abspath(path, pool); > ]]] > -- Stefan^2. --20cf30223ce9bba7340506031abb Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hey Julian,

Belated thanks for the code review!
=

On Wed, Oct 1, 2014 at 5:35 PM, Julian Foad <jul= ianfoad@btopenworld.com> wrote:
I took a look through the FSFS code that deals with= the changed-path lists, with a special interest in how it reports no-op ch= anges. In the course of this code inspection I found a number of issues tha= t want clarification and a few possible bugs. I have not tried to produce a= ctual buggy behaviour from these observations.

Some of this code is new or changed for format 7, and some is old.

Some of this seems to have been broken for a lo= ng time
and on all backends. Most of it is masked by the way = we
drive our editors and the fact the virtually nobody uses t= he
FS API directly. Until r1631047, we didn't even test t= he
svn_fs_paths_changed2 function.

I'm = thinking of changing the paths_changed API in 1.10
to fix a f= ew conceptual issues. E.g. removing the noderev
ID (it is invalid outsid= e the transactions anyway) and
using an array instead of a ha= sh since this is an ordered
list of operations.
=C2=A0
=
Comments, concerns and suggested code updates together in the form of a dif= f:

Feedback is inline:
=C2=A0
[[[
Index: subversion/include/svn_fs.h
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- subversion/include/svn_fs.h=C2=A0=C2=A0=C2=A0 (revision 1628514)
+++ subversion/include/svn_fs.h=C2=A0=C2=A0=C2=A0 (working copy)
@@ -1427,38 +1427,61 @@ typedef enum svn_fs_path_change_kind_t
=C2=A0/** Change descriptor.
=C2=A0 *
=C2=A0 * @note Fields may be added to the end of this structure in future =C2=A0 * versions.=C2=A0 Therefore, to preserve binary compatibility, users=
=C2=A0 * should not directly allocate structures of this type.
=C2=A0 *
+ * @note The @c text_mod, @c prop_mod and @c mergeinfo_mod flags mean the<= br> + * text, properties and mergeinfo property (respectively) were "touch= ed"
+ * by the commit API; this does not mean the new value is different from + * the old value.
+ *
=C2=A0 * @since New in 1.6. */
=C2=A0typedef struct svn_fs_path_change2_t
=C2=A0{
=C2=A0=C2=A0 /** node revision id of changed path */
=C2=A0=C2=A0 const svn_fs_id_t *node_rev_id;

=C2=A0=C2=A0 /** kind of change */
=C2=A0=C2=A0 svn_fs_path_change_kind_t change_kind;

-=C2=A0 /** were there text mods? */
+=C2=A0 /** was the text touched?
+=C2=A0=C2=A0 * For node_kind=3Ddir: always false. For node_kind=3Dfile: +=C2=A0=C2=A0 *=C2=A0=C2=A0 modify:=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 true iff = text touched.
+=C2=A0=C2=A0 *=C2=A0=C2=A0 add (copy):=C2=A0 true iff text touched.
+=C2=A0=C2=A0 *=C2=A0=C2=A0 add (plain): always true.
+=C2=A0=C2=A0 *=C2=A0=C2=A0 delete:=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 always fa= lse.
+=C2=A0=C2=A0 *=C2=A0=C2=A0 replace:=C2=A0=C2=A0=C2=A0=C2=A0 as for the add= /copy part of the replacement.
+=C2=A0=C2=A0 */
=C2=A0=C2=A0 svn_boolean_t text_mod;

-=C2=A0 /** were there property mods? */
+=C2=A0 /** were the properties touched?
+=C2=A0=C2=A0 *=C2=A0=C2=A0 modify:=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 true iff = props touched.
+=C2=A0=C2=A0 *=C2=A0=C2=A0 add (copy):=C2=A0 true iff props touched.
+=C2=A0=C2=A0 *=C2=A0=C2=A0 add (plain): true iff props touched.
+=C2=A0=C2=A0 *=C2=A0=C2=A0 delete:=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 always fa= lse.
+=C2=A0=C2=A0 *=C2=A0=C2=A0 replace:=C2=A0=C2=A0=C2=A0=C2=A0 as for the add= /copy part of the replacement.
+=C2=A0=C2=A0 */
=C2=A0=C2=A0 svn_boolean_t prop_mod;

=C2=A0=C2=A0 /** what node kind is the path?
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (Note: it is legal for this to be #svn= _node_unknown.) */
=C2=A0=C2=A0 svn_node_kind_t node_kind;

=C2=A0=C2=A0 /** Copyfrom revision and path; this is only valid if copyfrom= _known
=C2=A0=C2=A0=C2=A0 * is true. */
=C2=A0=C2=A0 svn_boolean_t copyfrom_known;
=C2=A0=C2=A0 svn_revnum_t copyfrom_rev;
=C2=A0=C2=A0 const char *copyfrom_path;

-=C2=A0 /** were there mergeinfo mods?
+=C2=A0 /** was the mergeinfo property touched?
+=C2=A0=C2=A0 *=C2=A0=C2=A0 modify:=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } true if= f svn:mergeinfo property add/del/mod
+=C2=A0=C2=A0 *=C2=A0=C2=A0 add (copy):=C2=A0 }=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 and fs format supports mergeinfo.

Minor correction: The format must support the mergei= nfo_mod
flag (1.9) not just plain mergeinfo (1.5).
<= div>=C2=A0
+=C2=A0=C2=A0 *=C2=A0=C2=A0 add (plain): }
+=C2=A0=C2=A0 *=C2=A0=C2=A0 delete:=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 always fa= lse.
+=C2=A0=C2=A0 *=C2=A0=C2=A0 replace:=C2=A0=C2=A0=C2=A0=C2=A0 as for the add= /copy part of the replacement.
=C2=A0=C2=A0=C2=A0 * (Note: Pre-1.9 repositories will report #svn_tristate_= unknown.)
=C2=A0=C2=A0=C2=A0 * @since New in 1.9. */
=C2=A0=C2=A0 svn_tristate_t mergeinfo_mod;
=C2=A0=C2=A0 /* NOTE! Please update svn_fs_path_change2_create() when addin= g new
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fields here. */
=C2=A0} svn_fs_path_change2_t;=C2=A0

Docstr= ing changes committed as r1628759.
=C2=A0
Index: subversion/libsvn_fs_fs/low_level.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- subversion/libsvn_fs_fs/low_level.c=C2=A0=C2=A0=C2=A0 (revision 1628514= )
+++ subversion/libsvn_fs_fs/low_level.c=C2=A0=C2=A0=C2=A0 (working copy) @@ -353,12 +353,15 @@ read_change(change_t **change_p,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return svn_error_create(SVN_ERR_FS_COR= RUPT, NULL,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 _("Invalid prop-mod flag in rev-file&qu= ot;));
=C2=A0=C2=A0=C2=A0=C2=A0 }

=C2=A0=C2=A0 /* Get the mergeinfo-mod flag if given.=C2=A0 Otherwise, the n= ext thing
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 is the path starting with a slash. */
+=C2=A0 /* ### Must initialize info->mergeinfo_mod to 'unknown' = rather than 0,
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 else write_change_entry()= would assume it's definitely false. */
+=C2=A0 /* info->mergeinfo_mod =3D svn_tristate_unknown; */

Nasty one, masked by the fact that higher level
= libs did not test for "unknown" but defaulted to it.
Fixed in = r1628762.
=C2=A0
=C2=A0=C2=A0 if (*last_str !=3D '/')
=C2=A0=C2=A0=C2=A0=C2=A0 {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 str =3D svn_cstring_tokenize(" &q= uot;, &last_str);
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (str =3D=3D NULL)
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return svn_error_create(SV= N_ERR_FS_CORRUPT, NULL,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 _("Invalid changes line in = rev-file"));
@@ -470,9 +473,10 @@

-/* Write a single change entry, path PATH, change CHANGE, and copyfrom
-=C2=A0=C2=A0 string COPYFROM, into the file specified by FILE.=C2=A0 Only = include the
+/* Write a single change entry, path PATH, change CHANGE, to STREAM.
+
+=C2=A0=C2=A0 Only include the

Correcte= d in r1631049.
=C2=A0
=C2=A0=C2=A0=C2=A0 node kind field if INCLUDE_NODE_KIND is true.=C2=A0 Only= include the
=C2=A0=C2=A0=C2=A0 mergeinfo-mod field if INCLUDE_MERGEINFO_MODS is true.= =C2=A0 All temporary
=C2=A0=C2=A0=C2=A0 allocations are in SCRATCH_POOL. */
=C2=A0static svn_error_t *
=C2=A0write_change_entry(svn_stream_t *stream,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const char *path,
@@ -563,18 +567,34 @@ svn_fs_fs__write_changes(svn_stream_t *s
=C2=A0=C2=A0 apr_pool_t *iterpool =3D svn_pool_create(scratch_pool);
=C2=A0=C2=A0 fs_fs_data_t *ffd =3D fs->fsap_data;
=C2=A0=C2=A0 svn_boolean_t include_node_kinds =3D
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ffd->format >=3D SVN_FS_FS__MIN_= KIND_IN_CHANGED_FORMAT;
=C2=A0=C2=A0 svn_boolean_t include_mergeinfo_mods =3D
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ffd->format >=3D SVN_FS_FS__MIN_= MERGEINFO_IN_CHANGES_FORMAT;
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* ### Rename s/_CHANGES_/_CHANGED_/ to mat= ch MIN_KIND_...? */

Renamed for consist= ency in r1631185.
=C2=A0
=C2=A0=C2=A0 apr_array_header_t *sorted_changed_paths;
=C2=A0=C2=A0 int i;

=C2=A0=C2=A0 /* For the sake of the repository administrator sort the chang= es so
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 that the final file is deterministic and rep= eatable, however the
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 rest of the FSFS code doesn't require an= y particular order here. */
+
+=C2=A0 /* ### This sorting would not be effective if the caller writes cha= nges
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 in multiple batches. This= sorting would not be safe if CHANGES
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 included multiple changes= for the same path.
+
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 While building up the txn= we call this from svn_fs_fs__add_change(),
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 multiple changes per path= overall, but only one change at a time.
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 While writing the final p= roto-rev file we call this from
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 write_final_changed_path_= info() with all the changes at once, but
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 only one change per path.=
+
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Both of the current calle= rs get safe and effective behaviour, but
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 consider sorting in write= _final_changed_path_info() instead to
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 make all this clearer. (T= his internal API would have to change.)
+=C2=A0=C2=A0 */

It turns out that leav= ing the sorting part to the caller
is difficult since the exi= sting changes array and hash
representation use different structs. We wo= uld need
to copy & realloc quite a bit, which means a lot of
extr= a code and peak memory usage.

In r1631186, I added a comm= ent that explains that
this ordering is only partial and why = that is the way
we want it to be.
=C2=A0
=C2=A0=C2=A0 sorted_changed_paths =3D svn_sort__hash(changes,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 svn_sort_compare_items_lexically,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 scratch_pool);

=C2=A0=C2=A0 /* Write all items to disk in the new order. */
=C2=A0=C2=A0 for (i =3D 0; i < sorted_changed_paths->nelts; ++i)
Index: subversion/libsvn_fs_fs/transaction.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- subversion/libsvn_fs_fs/transaction.c=C2=A0=C2=A0=C2=A0 (revision 16286= 76)
+++ subversion/libsvn_fs_fs/transaction.c=C2=A0=C2=A0=C2=A0 (working copy)<= br> @@ -624,20 +624,29 @@ replace_change
=C2=A0/* Copy the contents of NEW_CHANGE into OLD_CHANGE assuming that both=
-=C2=A0=C2=A0 belong to the same path.=C2=A0 Allocate copies in POOL.
+=C2=A0=C2=A0 belong to the same path.=C2=A0 Do not copy the change_kind fi= eld.
+
+=C2=A0=C2=A0 ### For semantic simplicity, I suggest making this copy the *= whole*
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 contents, and renaming it to 'cop= y_change' or 'svn_fs_path_change2_dup'
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if it also allocates a new object (ca= ller adjustment needed). It
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 could call memcpy and then duplicate = the sub-objects, simplifying
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 the implementation too.
+
+=C2=A0=C2=A0 Allocate copies in POOL.

r1631115 re= places this function with path_change_dup().
=C2=A0
=C2=A0 */
=C2=A0static void
=C2=A0replace_change(svn_fs_path_change2_t *old_change,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 const svn_fs_path_change2_t *new_change,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 apr_pool_t *pool)
=C2=A0{
=C2=A0=C2=A0 old_change->node_kind =3D new_change->node_kind;
=C2=A0=C2=A0 old_change->node_rev_id =3D svn_fs_fs__id_copy(new_change-&= gt;node_rev_id,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pool);
=C2=A0=C2=A0 old_change->text_mod =3D new_change->text_mod;
=C2=A0=C2=A0 old_change->prop_mod =3D new_change->prop_mod;
=C2=A0=C2=A0 old_change->mergeinfo_mod =3D new_change->mergeinfo_mod;=
+=C2=A0 /* ### What about copyfrom_known? (Is it *always* TRUE?) */

For FSFS, it implicitly is. Also, see below.
=C2=A0
=C2=A0=C2=A0 if (new_change->copyfrom_rev =3D=3D SVN_INVALID_REVNUM)
=C2=A0=C2=A0=C2=A0=C2=A0 {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 old_change->copyfrom_rev =3D SVN_IN= VALID_REVNUM;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 old_change->copyfrom_path =3D NULL;=
=C2=A0=C2=A0=C2=A0=C2=A0 }
=C2=A0=C2=A0 else
@@ -714,18 +723,22 @@ fold_change(apr_hash_t *changed_paths,

=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 case svn_fs_path_change_de= lete:
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (old_change= ->change_kind =3D=3D svn_fs_path_change_add)
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 {<= br> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 /* If the path was introduced in this transaction via an
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 add, and we are deleting it, just remove the pa= th
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 altogether. */
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 altogether.=C2=A0 (The caller will delete any chil= d paths.) */

Part of r1631115.

T= here was another problem lurking here which got
fixed in 1631171. Deleti= ons of replacements would
report the wrong noderev ID and node type.
=
=C2=A0
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 old_change =3D NULL;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }<= br> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 else
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 {<= br> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 /* A deletion overrules all previous changes. */
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 /* A deletion overrules a previous change (modify/replace). */
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 /* ### Why not call replace_change(old_change, info, pool); ? */
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 /* ### What about node_rev_id? Would be wrong after a replace? */ +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 /* ### What about node_kind? Could be wrong after a replace? */
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 /* ### What about copyfrom_known? (Is it *always* TRUE?) */
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 old_change->change_kind =3D svn_fs_path_change_delete;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 old_change->text_mod =3D info->text_mod;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 old_change->prop_mod =3D info->prop_mod;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 old_change->mergeinfo_mod =3D info->mergeinfo_mod;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 old_change->copyfrom_rev =3D SVN_INVALID_REVNUM;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 old_change->copyfrom_path =3D NULL;

<= /div>
Rewritten as part of r1631115.
=C2=A0
@@ -739,12 +752,15 @@ fold_change(apr_hash_t *changed_paths,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 replace_change= (old_change, info, pool);
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 old_change->= ;change_kind =3D svn_fs_path_change_replace;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 break;

=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 case svn_fs_path_change_mo= dify:
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 default:
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* If the new chang= e modifies some attribute of the node, set
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 t= he corresponding flag, whether it already was set or not.
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 N= ote: We do not reset a flag to FALSE if a change is undone. */

Committed as r1631180.
=C2=A0
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (info->t= ext_mod)
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ol= d_change->text_mod =3D TRUE;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (info->p= rop_mod)
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ol= d_change->prop_mod =3D TRUE;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (info->m= ergeinfo_mod =3D=3D svn_tristate_true)
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ol= d_change->mergeinfo_mod =3D svn_tristate_true;
@@ -761,12 +777,13 @@ fold_change(apr_hash_t *changed_paths,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 structure from the i= nternal one (in the hash's pool), and dup
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 the path into the ha= sh's pool, too. */
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 new_change =3D apr_pmemdup(pool, info,= sizeof(*new_change));
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 new_change->node_rev_id =3D svn_fs_= fs__id_copy(info->node_rev_id, pool);
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (info->copyfrom_path)
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 new_change->copyfrom_pa= th =3D apr_pstrdup(pool, info->copyfrom_path);
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* ### Use copy_change() or svn_fs_path_cha= nge2_dup() instead. */

Done as part of r16311= 15.
=C2=A0
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* Add this path.=C2=A0 The API makes = no guarantees that this (new) key
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 will not be retained.=C2= =A0 Thus, we copy the key into the target pool
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 to ensure a proper lifetim= e.=C2=A0 */
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 apr_hash_set(changed_paths,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 apr_pstrmemdup(pool, path->data,= path->len), path->len,
@@ -1585,16 +1602,24 @@ svn_fs_fs__add_change(svn_fs_t *fs,
=C2=A0=C2=A0 change->text_mod =3D text_mod;
=C2=A0=C2=A0 change->prop_mod =3D prop_mod;
=C2=A0=C2=A0 change->mergeinfo_mod =3D mergeinfo_mod
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ? svn= _tristate_true
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 : svn= _tristate_false;
=C2=A0=C2=A0 change->node_kind =3D node_kind;
+=C2=A0 /* ### copyfrom_known remains FALSE, but svn_fs_fs__write_changes()=
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ignores it. Should set TR= UE for clarity, and also document
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 svn_fs_fs__write_changes(= ) as ignoring it? */

It turns out to be= a backend-specific invariant.
As of r1631050, we initialize = it correctly.
=C2=A0
=C2=A0=C2=A0 change->copyfrom_rev =3D copyfrom_rev;
=C2=A0=C2=A0 change->copyfrom_path =3D apr_pstrdup(pool, copyfrom_path);=
+=C2=A0 /* ### COPYFROM_PATH may be NULL. apr_pstrdup < 1.5 does not pro= mise
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 null-safety, although the= implementation since 0.9.0 is null-safe.
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 For consistency, should c= onditionalize it. */

Changed in r1631051.
=
=C2=A0
=C2=A0=C2=A0 svn_hash_sets(changes, path, change);
=C2=A0=C2=A0 SVN_ERR(svn_fs_fs__write_changes(svn_stream_from_aprfile2(file= , TRUE, pool),
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fs, changes, F= ALSE, pool));
+=C2=A0 /* ### There is no call (in this context) that sets the 'termin= ate_list'
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 parameter true as require= d by the doc string. */

That is o.k. We= need to set that flag only if the file does
not end with the= list. Docstring corrected in r1631049.
=C2=A0
=C2=A0=C2=A0 return svn_io_file_close(file, pool);
=C2=A0}

Index: subversion/libsvn_fs_fs/tree.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- subversion/libsvn_fs_fs/tree.c=C2=A0=C2=A0=C2=A0 (revision 1628514)
+++ subversion/libsvn_fs_fs/tree.c=C2=A0=C2=A0=C2=A0 (working copy)
@@ -1534,13 +1534,13 @@ fs_change_node_prop(svn_fs_root_t *root,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const svn_string_t *value, =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 apr_pool_t *pool)
=C2=A0{
=C2=A0=C2=A0 parent_path_t *parent_path;
=C2=A0=C2=A0 apr_hash_t *proplist;
=C2=A0=C2=A0 const svn_fs_fs__id_part_t *txn_id;
-=C2=A0 svn_boolean_t modeinfo_mod =3D FALSE;
+=C2=A0 svn_boolean_t modeinfo_mod =3D FALSE; /* ### "mergeinfo_mod&qu= ot;? */

Yep, modeinfo_mod would be one = level too meta.
Fixed in r1628764.
=C2=A0
=C2=A0=C2=A0 if (! root->is_txn_root)
=C2=A0=C2=A0=C2=A0=C2=A0 return SVN_FS__NOT_TXN(root);
=C2=A0=C2=A0 txn_id =3D root_txn_id(root);

=C2=A0=C2=A0 path =3D svn_fs__canonicalize_abspath(path, pool);
]]]

-- Stefan^2.

--20cf30223ce9bba7340506031abb--