Bert Huijben <b...@qqmail.nl> writes:
> Noting everything as a change even if there is no actual delta... still
> looks like a bug we should fix (and we did) instead of something that we
> should restore.
>
> For the reasoning on why:
>
> For blame we are really interested if there are deltas... and if there
> aren't any changes the revision is not 'interesting'. (See docs of the api).
The place where we define "interesting" revisions is svn_fs_history_prev2():
/** Set @a *prev_history_p to an opaque node history object which
* represents the previous (or "next oldest") interesting history
* location for the filesystem node represented by @a history, or @c
* NULL if no such previous history exists. If @a cross_copies is @c
* FALSE, also return @c NULL if stepping backwards in history to @a
* *prev_history_p would cross a filesystem copy operation.
*
* @note If this is the first call to svn_fs_history_prev() for the @a
* history object, it could return a history object whose location is
* the same as the original. This will happen if the original
* location was an interesting one (where the node was modified, or
* took place in a copy event). This behavior allows looping callers
* to avoid the calling svn_fs_history_location() on the object
* returned by svn_fs_node_history(), and instead go ahead and begin
* calling svn_fs_history_prev().
...
Functions like svn_ra_get_file_revs2() that you mentioned just link to it:
/**
* Retrieve a subset of the interesting revisions of a file @a path
* as seen in revision @a end (see svn_fs_history_prev() for a
* definition of "interesting revisions").
...
The svn_fs_history_prev2() function reports revisions with empty deltas as
interesting, and has been doing that for years (see the attached patch with
a test). This behavior did not change in 1.9.
This means that after r1572363 and r1573111, svn_ra_get_file_revs2() and
svn_repos_get_file_revs2() were skipping some of the "interesting" revisions,
according to the FS API defining the concept. Moreover, this behavior could
be inconsistent even within a single function like svn_ra_get_file_revs2()
that calls svn_ra_get_log2() for old servers, as get-log notices revisions
with empty deltas.
I think that it's another example of where r1572363 and r1573111 introduce an
inconsistent and unwanted behavior change.
Stefan Fuhrmann <stefan.fuhrm...@wandisco.com> writes:
>> As for /trunk, I think that we could do that, so I sketched this option in
>> the attached patch.
>
> The patch looks o.k.
Thanks for giving this patch a look.
I examined the differences between these functions, svn_repos__compare_files()
and svn_fs_contents_different(), and I think that we also have a problem in
FSFS's implementation of the new svn_fs_contents_different() API in 1.9.x.
The implementation relies on representation_t.expanded_size value when doing
the comparison. If this value is zero, a representation is considered empty,
and two empty representations are considered equal.
The problem is that the expanded_size can be zero for empty files and also for
some of the PLAIN representations, which are not empty. If I am not mistaken,
we have a workaround for this in /trunk — r1673875 [1] and follow-ups, but not
in Subversion 1.9. Hence, the new svn_fs_contents_different() API can fail to
distinguish different contents in 1.9.x under the circumstances described in
issue 4554 [2].
So, we can use it in trunk, but this API may produce invalid results in 1.9.x.
I'll commit the patch, and then I am going to prepare the nomination consisting
of r1709388 and the new dump_no_op_change() test.
[1] https://svn.apache.org/r1673875
[2] https://issues.apache.org/jira/browse/SVN-4554
Regards,
Evgeny Kotkov
Index: subversion/tests/libsvn_fs/fs-test.c
===================================================================
--- subversion/tests/libsvn_fs/fs-test.c (revision 1710620)
+++ subversion/tests/libsvn_fs/fs-test.c (working copy)
@@ -6971,6 +6971,47 @@ freeze_and_commit(const svn_test_opts_t *opts,
return SVN_NO_ERROR;
}
+static svn_error_t *
+history_with_empty_delta(const svn_test_opts_t *opts,
+ apr_pool_t *pool)
+{
+ svn_fs_t *fs;
+ svn_revnum_t head_rev = 0;
+ svn_fs_root_t *root;
+ svn_fs_txn_t *txn;
+ svn_fs_history_t *history;
+ const char *path;
+ svn_revnum_t revision;
+
+ SVN_ERR(svn_test__create_fs(&fs, "test-history-with-empty-delta",
+ opts, pool));
+
+ /* r1: Create the greek tree. */
+ SVN_ERR(svn_fs_begin_txn(&txn, fs, head_rev, pool));
+ SVN_ERR(svn_fs_txn_root(&root, txn, pool));
+ SVN_ERR(svn_test__create_greek_tree(root, pool));
+ SVN_ERR(test_commit_txn(&head_rev, txn, NULL, pool));
+
+ /* r2: Commit an empty delta for /iota. */
+ SVN_ERR(svn_fs_begin_txn(&txn, fs, head_rev, pool));
+ SVN_ERR(svn_fs_txn_root(&root, txn, pool));
+ SVN_ERR(svn_test__set_file_contents(root, "/iota",
+ "This is the file 'iota'.\n", pool));
+ SVN_ERR(test_commit_txn(&head_rev, txn, NULL, pool));
+
+ /* Get the first real point of interesting history. */
+ SVN_ERR(svn_fs_revision_root(&root, fs, head_rev, pool));
+ SVN_ERR(svn_fs_node_history2(&history, root, "/iota", pool, pool));
+ SVN_ERR(svn_fs_history_prev2(&history, history, FALSE, pool, pool));
+
+ /* The revision with empty delta is reported as "interesting". */
+ SVN_ERR(svn_fs_history_location(&path, &revision, history, pool));
+ SVN_TEST_STRING_ASSERT(path, "/iota");
+ SVN_TEST_ASSERT(revision == 2);
+
+ return SVN_NO_ERROR;
+}
+
/* ------------------------------------------------------------------------ */
/* The test table. */
@@ -7105,6 +7146,8 @@ static struct svn_test_descriptor_t test_funcs[] =
"test svn_fs_check_related for transactions"),
SVN_TEST_OPTS_PASS(freeze_and_commit,
"freeze and commit"),
+ SVN_TEST_OPTS_PASS(history_with_empty_delta,
+ "test node history with empty delta"),
SVN_TEST_NULL
};
