Index: subversion/include/svn_ra.h =================================================================== --- subversion/include/svn_ra.h (revision 1708565) +++ subversion/include/svn_ra.h (working copy) @@ -1797,12 +1797,6 @@ svn_ra_get_location_segments(svn_ra_session_t *ses * to support reversion of the revision range for @a include_merged_revision * @c FALSE reporting by switching @a end with @a start. * - * @note Prior to Subversion 1.9, this function may request delta handlers - * from @a handler even for empty text deltas. Starting with 1.9, the - * delta handler / baton return arguments passed to @a handler will be - * #NULL unless there is an actual difference in the file contents between - * the current and the previous call. - * * @since New in 1.5. */ svn_error_t * Index: subversion/include/svn_repos.h =================================================================== --- subversion/include/svn_repos.h (revision 1708565) +++ subversion/include/svn_repos.h (working copy) @@ -2093,12 +2093,6 @@ svn_repos_fs_get_mergeinfo(svn_mergeinfo_catalog_t * the revision range for @a include_merged_revision @c FALSE reporting by * switching @a start with @a end. * - * @note Prior to Subversion 1.9, this function may request delta handlers - * from @a handler even for empty text deltas. Starting with 1.9, the - * delta handler / baton return arguments passed to @a handler will be - * #NULL unless there is an actual difference in the file contents between - * the current and the previous call. - * * @since New in 1.5. */ svn_error_t * Index: subversion/libsvn_fs_base/dag.c =================================================================== --- subversion/libsvn_fs_base/dag.c (revision 1708565) +++ subversion/libsvn_fs_base/dag.c (working copy) @@ -1657,8 +1657,14 @@ svn_fs_base__things_different(svn_boolean_t *props /* Compare contents keys and their (optional) uniquifiers. */ if (contents_changed != NULL) - *contents_changed = (! svn_fs_base__same_keys(noderev1->data_key, - noderev2->data_key)); + *contents_changed = + (! (svn_fs_base__same_keys(noderev1->data_key, + noderev2->data_key) + /* Technically, these uniquifiers aren't used and "keys", + but keys are base-36 stringified numbers, so we'll take + this liberty. */ + && (svn_fs_base__same_keys(noderev1->data_key_uniquifier, + noderev2->data_key_uniquifier)))); return SVN_NO_ERROR; } Index: subversion/libsvn_fs_base/fs.h =================================================================== --- subversion/libsvn_fs_base/fs.h (revision 1708565) +++ subversion/libsvn_fs_base/fs.h (working copy) @@ -195,11 +195,7 @@ typedef struct node_revision_t only because one or both of us decided to pick up a shared representation after-the-fact." May be NULL (if this node revision isn't using a shared rep, or isn't the original - "assignee" of a shared rep). - - This is no longer used by the 1.9 code but we have to keep - reading and writing it to remain compatible with 1.8, and - earlier, that require it. */ + "assignee" of a shared rep). */ const char *data_key_uniquifier; /* representation key for this node's text-data-in-progess (files Index: subversion/libsvn_fs_fs/dag.c =================================================================== --- subversion/libsvn_fs_fs/dag.c (revision 1708565) +++ subversion/libsvn_fs_fs/dag.c (working copy) @@ -1305,8 +1305,6 @@ svn_fs_fs__dag_things_different(svn_boolean_t *pro apr_pool_t *pool) { node_revision_t *noderev1, *noderev2; - svn_fs_t *fs; - svn_boolean_t same; /* If we have no place to store our results, don't bother doing anything. */ @@ -1313,26 +1311,55 @@ svn_fs_fs__dag_things_different(svn_boolean_t *pro if (! props_changed && ! contents_changed) return SVN_NO_ERROR; - fs = svn_fs_fs__dag_get_fs(node1); - /* The node revision skels for these two nodes. */ SVN_ERR(get_node_revision(&noderev1, node1)); SVN_ERR(get_node_revision(&noderev2, node2)); - /* Compare property keys. */ - if (props_changed != NULL) + if (strict) { - SVN_ERR(svn_fs_fs__prop_rep_equal(&same, fs, noderev1, noderev2, - strict, pool)); - *props_changed = !same; + /* In strict mode, compare text and property representations in the + svn_fs_contents_different() / svn_fs_props_different() manner. + These functions are currently not being used in our codebase, but + we released them as a part of 1.9, and keep them for compatibility + reasons. + + See the "No-op changes no longer dumped by 'svnadmin dump' in 1.9" + discussion (http://svn.haxx.se/dev/archive-2015-09/0269.shtml) and + issue #4598 (https://issues.apache.org/jira/browse/SVN-4598). */ + svn_fs_t *fs = svn_fs_fs__dag_get_fs(node1); + svn_boolean_t same; + + /* Compare property keys. */ + if (props_changed != NULL) + { + SVN_ERR(svn_fs_fs__prop_rep_equal(&same, fs, noderev1, + noderev2, pool)); + *props_changed = !same; + } + + /* Compare contents keys. */ + if (contents_changed != NULL) + { + SVN_ERR(svn_fs_fs__file_text_rep_equal(&same, fs, noderev1, + noderev2, pool)); + *contents_changed = !same; + } } + else + { + /* Otherwise, compare representation keys -- as in Subversion 1.8. */ - /* Compare contents keys. */ - if (contents_changed != NULL) - { - SVN_ERR(svn_fs_fs__file_text_rep_equal(&same, fs, noderev1, noderev2, - strict, pool)); - *contents_changed = !same; + /* Compare property keys. */ + if (props_changed != NULL) + *props_changed = + !svn_fs_fs__noderev_same_rep_key(noderev1->prop_rep, + noderev2->prop_rep); + + /* Compare contents keys. */ + if (contents_changed != NULL) + *contents_changed = + !svn_fs_fs__noderev_same_rep_key(noderev1->data_rep, + noderev2->data_rep); } return SVN_NO_ERROR; Index: subversion/libsvn_fs_fs/fs.h =================================================================== --- subversion/libsvn_fs_fs/fs.h (revision 1708565) +++ subversion/libsvn_fs_fs/fs.h (working copy) @@ -550,13 +550,7 @@ typedef struct representation_t /* For rep-sharing, we need a way of uniquifying node-revs which share the same representation (see svn_fs_fs__noderev_same_rep_key() ). So, we store the original txn of the node rev (not the rep!), along with some - intra-node uniqification content. - - This is no longer used by the 1.9 code but we have to keep - reading and writing it for old formats to remain compatible with - 1.8, and earlier, that require it. We also read/write it in - format 7 even though it is not currently required by any code - that handles that format. */ + intra-node uniqification content. */ struct { /* unique context, i.e. txn ID, in which the noderev (!) got created */ Index: subversion/libsvn_fs_fs/fs_fs.c =================================================================== --- subversion/libsvn_fs_fs/fs_fs.c (revision 1708565) +++ subversion/libsvn_fs_fs/fs_fs.c (working copy) @@ -1387,12 +1387,30 @@ svn_fs_fs__file_length(svn_filesize_t *length, return SVN_NO_ERROR; } +svn_boolean_t +svn_fs_fs__noderev_same_rep_key(representation_t *a, + representation_t *b) +{ + if (a == b) + return TRUE; + + if (a == NULL || b == NULL) + return FALSE; + + if (a->item_index != b->item_index) + return FALSE; + + if (a->revision != b->revision) + return FALSE; + + return memcmp(&a->uniquifier, &b->uniquifier, sizeof(a->uniquifier)) == 0; +} + svn_error_t * svn_fs_fs__file_text_rep_equal(svn_boolean_t *equal, svn_fs_t *fs, node_revision_t *a, node_revision_t *b, - svn_boolean_t strict, apr_pool_t *scratch_pool) { svn_stream_t *contents_a, *contents_b; @@ -1437,19 +1455,6 @@ svn_fs_fs__file_text_rep_equal(svn_boolean_t *equa return SVN_NO_ERROR; } - /* Old repositories may not have the SHA1 checksum handy. - This check becomes expensive. Skip it unless explicitly required. - - We already have seen that the ID is different, so produce a likely - false negative as allowed by the API description - even though the - MD5 matched, there is an extremely slim chance that the SHA1 wouldn't. - */ - if (!strict) - { - *equal = FALSE; - return SVN_NO_ERROR; - } - SVN_ERR(svn_fs_fs__get_contents(&contents_a, fs, rep_a, TRUE, scratch_pool)); SVN_ERR(svn_fs_fs__get_contents(&contents_b, fs, rep_b, TRUE, @@ -1465,7 +1470,6 @@ svn_fs_fs__prop_rep_equal(svn_boolean_t *equal, svn_fs_t *fs, node_revision_t *a, node_revision_t *b, - svn_boolean_t strict, apr_pool_t *scratch_pool) { representation_t *rep_a = a->prop_rep; @@ -1512,14 +1516,6 @@ svn_fs_fs__prop_rep_equal(svn_boolean_t *equal, return SVN_NO_ERROR; } - /* Skip the expensive bits unless we are in strict mode. - Simply assume that there is a difference. */ - if (!strict) - { - *equal = FALSE; - return SVN_NO_ERROR; - } - /* At least one of the reps has been modified in a txn. Fetch and compare them. */ SVN_ERR(svn_fs_fs__get_proplist(&proplist_a, fs, a, scratch_pool)); Index: subversion/libsvn_fs_fs/fs_fs.h =================================================================== --- subversion/libsvn_fs_fs/fs_fs.h (revision 1708565) +++ subversion/libsvn_fs_fs/fs_fs.h (working copy) @@ -90,9 +90,13 @@ svn_error_t *svn_fs_fs__file_length(svn_filesize_t node_revision_t *noderev, apr_pool_t *pool); +/* Return TRUE if the representation keys in A and B both point to the + same representation, else return FALSE. */ +svn_boolean_t svn_fs_fs__noderev_same_rep_key(representation_t *a, + representation_t *b); + /* Set *EQUAL to TRUE if the text representations in A and B within FS - have equal contents, else set it to FALSE. If STRICT is not set, allow - for false negatives. + have equal contents, else set it to FALSE. Use SCRATCH_POOL for temporary allocations. */ svn_error_t * svn_fs_fs__file_text_rep_equal(svn_boolean_t *equal, @@ -99,12 +103,10 @@ svn_fs_fs__file_text_rep_equal(svn_boolean_t *equa svn_fs_t *fs, node_revision_t *a, node_revision_t *b, - svn_boolean_t strict, apr_pool_t *scratch_pool); /* Set *EQUAL to TRUE if the property representations in A and B within FS - have equal contents, else set it to FALSE. If STRICT is not set, allow - for false negatives. + have equal contents, else set it to FALSE. Use SCRATCH_POOL for temporary allocations. */ svn_error_t * svn_fs_fs__prop_rep_equal(svn_boolean_t *equal, @@ -111,7 +113,6 @@ svn_fs_fs__prop_rep_equal(svn_boolean_t *equal, svn_fs_t *fs, node_revision_t *a, node_revision_t *b, - svn_boolean_t strict, apr_pool_t *scratch_pool); Index: subversion/libsvn_fs_fs/tree.c =================================================================== --- subversion/libsvn_fs_fs/tree.c (revision 1708565) +++ subversion/libsvn_fs_fs/tree.c (working copy) @@ -1882,7 +1882,6 @@ merge(svn_stringbuf_t *conflict_p, */ { node_revision_t *tgt_nr, *anc_nr, *src_nr; - svn_boolean_t same; apr_pool_t *scratch_pool; /* Get node revisions for our id's. */ @@ -1899,15 +1898,16 @@ merge(svn_stringbuf_t *conflict_p, /* Now compare the prop-keys of the skels. Note that just because the keys are different -doesn't- mean the proplists have - different contents. */ - SVN_ERR(svn_fs_fs__prop_rep_equal(&same, fs, src_nr, anc_nr, TRUE, pool)); - if (! same) + different contents. But merge() isn't concerned with contents; + it doesn't do a brute-force comparison on textual contents, so + it won't do that here either. Checking to see if the propkey + atoms are `equal' is enough. */ + if (! svn_fs_fs__noderev_same_rep_key(src_nr->prop_rep, anc_nr->prop_rep)) return conflict_err(conflict_p, target_path); /* The directory entries got changed in the repository but the directory properties did not. */ - SVN_ERR(svn_fs_fs__prop_rep_equal(&same, fs, tgt_nr, anc_nr, TRUE, pool)); - if (! same) + if (! svn_fs_fs__noderev_same_rep_key(tgt_nr->prop_rep, anc_nr->prop_rep)) { /* There is an incoming prop change for this directory. We will accept it only if the directory changes were mere updates Index: subversion/libsvn_repos/delta.c =================================================================== --- subversion/libsvn_repos/delta.c (revision 1708565) +++ subversion/libsvn_repos/delta.c (working copy) @@ -530,8 +530,8 @@ delta_proplists(struct context *c, svn_boolean_t changed; /* Is this deltification worth our time? */ - SVN_ERR(svn_fs_props_different(&changed, c->target_root, target_path, - c->source_root, source_path, subpool)); + SVN_ERR(svn_fs_props_changed(&changed, c->target_root, target_path, + c->source_root, source_path, subpool)); if (! changed) goto cleanup; @@ -611,8 +611,62 @@ svn_repos__compare_files(svn_boolean_t *changed_p, const char *path2, apr_pool_t *pool) { - return svn_error_trace(svn_fs_contents_different(changed_p, root1, path1, - root2, path2, pool)); + svn_filesize_t size1, size2; + svn_checksum_t *checksum1, *checksum2; + svn_stream_t *stream1, *stream2; + svn_boolean_t same; + + /* If the filesystem claims the things haven't changed, then they + haven't changed. */ + SVN_ERR(svn_fs_contents_changed(changed_p, root1, path1, + root2, path2, pool)); + if (!*changed_p) + return SVN_NO_ERROR; + + /* If the SHA1 checksums match for these things, we'll claim they + have the same contents. (We don't give quite as much weight to + MD5 checksums.) */ + SVN_ERR(svn_fs_file_checksum(&checksum1, svn_checksum_sha1, + root1, path1, FALSE, pool)); + SVN_ERR(svn_fs_file_checksum(&checksum2, svn_checksum_sha1, + root2, path2, FALSE, pool)); + if (checksum1 && checksum2) + { + *changed_p = !svn_checksum_match(checksum1, checksum2); + return SVN_NO_ERROR; + } + + /* From this point on, our default answer is "Nothing's changed". */ + *changed_p = FALSE; + + /* Different filesizes means the contents are different. */ + SVN_ERR(svn_fs_file_length(&size1, root1, path1, pool)); + SVN_ERR(svn_fs_file_length(&size2, root2, path2, pool)); + if (size1 != size2) + { + *changed_p = TRUE; + return SVN_NO_ERROR; + } + + /* Different MD5 checksums means the contents are different. */ + SVN_ERR(svn_fs_file_checksum(&checksum1, svn_checksum_md5, root1, path1, + FALSE, pool)); + SVN_ERR(svn_fs_file_checksum(&checksum2, svn_checksum_md5, root2, path2, + FALSE, pool)); + if (!svn_checksum_match(checksum1, checksum2)) + { + *changed_p = TRUE; + return SVN_NO_ERROR; + } + + /* And finally, different contents means the ... uh ... contents are + different. */ + SVN_ERR(svn_fs_file_contents(&stream1, root1, path1, pool)); + SVN_ERR(svn_fs_file_contents(&stream2, root2, path2, pool)); + SVN_ERR(svn_stream_contents_same2(&same, stream1, stream2, pool)); + *changed_p = !same; + + return SVN_NO_ERROR; } @@ -639,7 +693,19 @@ delta_files(struct context *c, if (source_path) { - SVN_ERR(svn_fs_contents_different(&changed, + /* Is this delta calculation worth our time? If we are ignoring + ancestry, then our editor implementor isn't concerned by the + theoretical differences between "has contents which have not + changed with respect to" and "has the same actual contents + as". We'll do everything we can to avoid transmitting even + an empty text-delta in that case. */ + if (c->ignore_ancestry) + SVN_ERR(svn_repos__compare_files(&changed, + c->target_root, target_path, + c->source_root, source_path, + subpool)); + else + SVN_ERR(svn_fs_contents_changed(&changed, c->target_root, target_path, c->source_root, source_path, subpool)); Index: subversion/libsvn_repos/dump.c =================================================================== --- subversion/libsvn_repos/dump.c (revision 1708565) +++ subversion/libsvn_repos/dump.c (working copy) @@ -1164,13 +1164,13 @@ dump_node(struct edit_baton *eb, svn_fs_root_fs(eb->fs_root), compare_rev, pool)); - SVN_ERR(svn_fs_props_different(&must_dump_props, - compare_root, compare_path, - eb->fs_root, path, pool)); + SVN_ERR(svn_fs_props_changed(&must_dump_props, + compare_root, compare_path, + eb->fs_root, path, pool)); if (kind == svn_node_file) - SVN_ERR(svn_fs_contents_different(&must_dump_text, - compare_root, compare_path, - eb->fs_root, path, pool)); + SVN_ERR(svn_fs_contents_changed(&must_dump_text, + compare_root, compare_path, + eb->fs_root, path, pool)); break; case svn_node_action_delete: @@ -1293,16 +1293,16 @@ dump_node(struct edit_baton *eb, /* Need to decide if the copied node had any extra textual or property mods as well. */ - SVN_ERR(svn_fs_props_different(&must_dump_props, - compare_root, compare_path, - eb->fs_root, path, pool)); + SVN_ERR(svn_fs_props_changed(&must_dump_props, + compare_root, compare_path, + eb->fs_root, path, pool)); if (kind == svn_node_file) { svn_checksum_t *checksum; const char *hex_digest; - SVN_ERR(svn_fs_contents_different(&must_dump_text, - compare_root, compare_path, - eb->fs_root, path, pool)); + SVN_ERR(svn_fs_contents_changed(&must_dump_text, + compare_root, compare_path, + eb->fs_root, path, pool)); SVN_ERR(svn_fs_file_checksum(&checksum, svn_checksum_md5, compare_root, compare_path, Index: subversion/libsvn_repos/reporter.c =================================================================== --- subversion/libsvn_repos/reporter.c (revision 1708565) +++ subversion/libsvn_repos/reporter.c (working copy) @@ -578,8 +578,8 @@ delta_proplists(report_baton_t *b, svn_revnum_t s_ SVN_ERR(get_source_root(b, &s_root, s_rev)); /* Is this deltification worth our time? */ - SVN_ERR(svn_fs_props_different(&changed, b->t_root, t_path, s_root, - s_path, pool)); + SVN_ERR(svn_fs_props_changed(&changed, b->t_root, t_path, s_root, + s_path, pool)); if (! changed) return SVN_NO_ERROR; Index: subversion/libsvn_repos/rev_hunt.c =================================================================== --- subversion/libsvn_repos/rev_hunt.c (revision 1708565) +++ subversion/libsvn_repos/rev_hunt.c (working copy) @@ -1374,31 +1374,17 @@ send_path_revision(struct path_revision *path_rev, SVN_ERR(svn_prop_diffs(&prop_diffs, props, sb->last_props, sb->iterpool)); - /* Check if the contents *may* have changed. */ + /* Check if the contents changed. */ if (! sb->last_root) { /* Special case: In the first revision, we always provide a delta. */ contents_changed = TRUE; } - else if (sb->include_merged_revisions - && strcmp(sb->last_path, path_rev->path)) - { - /* ### This is a HACK!!! - * Blame -g, in older clients anyways, relies on getting a notification - * whenever the path changes - even if there was no content change. - * - * TODO: A future release should take an extra parameter and depending - * on that either always send a text delta or only send it if there - * is a difference. */ - contents_changed = TRUE; - } else { - /* Did the file contents actually change? - * It could e.g. be a property-only change. */ - SVN_ERR(svn_fs_contents_different(&contents_changed, sb->last_root, - sb->last_path, root, path_rev->path, - sb->iterpool)); + SVN_ERR(svn_fs_contents_changed(&contents_changed, sb->last_root, + sb->last_path, root, path_rev->path, + sb->iterpool)); } /* We have all we need, give to the handler. */ Index: subversion/tests/cmdline/svnadmin_tests.py =================================================================== --- subversion/tests/cmdline/svnadmin_tests.py (revision 1708565) +++ subversion/tests/cmdline/svnadmin_tests.py (working copy) @@ -3202,7 +3202,7 @@ def dump_revprops(sbox): svntest.actions.run_and_verify_svnlook(log_msg, [], 'log', '-r1', sbox.repo_dir) -@XFail() +@XFail(svntest.main.is_fs_type_fsx) @Issue(4598) def dump_no_op_change(sbox): "svnadmin dump with no-op changes"