Instead of trying to review the whole change at once, this mail will
concentrate on some simple issues (I'm still grokking the change
overall :-) ).
kameshj@tigris.org writes:
> Log:
> On issue-2897 branch: Fix 2897 Reflective merges are faulty.
Yay!
I propedited the log message to fix various formatting issues, by the
way. See the propchange diff email for details:
http://subversion.tigris.org/servlets/ReadMsg?list=svn&msgNo=33099
> Fix invloves 3 changes.
> - Schema change to track what revisions of merge source got merged in a
> given commit in 'mergeinfo_changed' table.
> - implement 'get_commit_revs_for_merge_ranges' API to get the commit revs
> for the reflective merge.
> - negate the reflective revision from the requested merge range.
>
> * subversion/libsvn_fs_util/sqlite-util.c
> (schema_create_sql): Make the following change to the schema of
> 'mergeinfo_changed' table.
> Rename column 'path' -> 'mergedto'
> New column 'mergedfrom' to track the 'merge source canonical path'.
> New columns 'mergedrevstart', 'mergedrevend' and 'inheritable' to track
> the merge revision range for this commit.
> Drop the indices mi_c_revpath_idx, mi_c_path_idx, mi_c_revision_idx.
> New index 'mi_c_merge_source_target_revstart_end_commit_rev_idx'.
> * subversion/libsvn_fs_util/mergeinfo-sqlite-index.c
> (): include svn_sorts.h
> (get_mergeinfo): Forward declaration, so that 'index_txn_mergeinfo' can also
> make use of this function.
> (index_path_mergeinfo): Change signature to accept hashes 'curr_mergeinfo',
> 'orig_mergeinfo', 'added_mergeinfo' instead of 'mergeinfo_str'(which is a
> mergeinfo as at this commit). Record full details about the merge for the
> given commit in 'mergeinfo_changed' table.
> (index_txn_mergeinfo): Call 'index_path_mergeinfo' with necessary arguments
> as per new signature.
> (get_mergeinfo_for_path, get_mergeinfo_for_children): Adjust the query for
> the new schema change.
> (get_parent_target_path_having_mergeinfo,
> get_commit_revs_for_merge_ranges,
> svn_fs_mergeinfo__get_commit_revs_for_merge_ranges): New function.
In the code, svn_fs_mergeinfo__get_commit_revs_for_merge_ranges() is
documented twice: the first time where it is declared in
include/private/svn_fs_mergeinfo.h, and again where it is defined in
subversion/libsvn_fs_util/mergeinfo-sqlite-index.c.
We shouldn't write the same doc string in two places. The API
declaration should document the full behavior, and that's the only doc
string it needs (otherwise, the two can get out of sync; or someone
might read only the nearest doc string and miss important information
that had been added later to the other one).
get_parent_target_path_having_mergeinfo()'s doc string doesn't mention
every parameter. (As I page through the diff, I'll look for other
functions that have this problem.) A doc string should leave no
questions unanswered -- at a minimum, that usually means documenting
exactly what each parameter does. I can't stress this enough: most
likely, whoever reads your code will not understand the problem space
as well as you do. We need those doc strings to fill in the gaps in
our knowledge! :-)
> --- branches/issue-2897/subversion/include/private/svn_dav_protocol.h (original)
> +++ branches/issue-2897/subversion/include/private/svn_dav_protocol.h Thu Nov 22 11:35:07 2007
> @@ -28,6 +28,8 @@
> /** Names for the custom HTTP REPORTs understood by mod_dav_svn, sans
> namespace. */
> #define SVN_DAV__MERGEINFO_REPORT "mergeinfo-report"
> +#define SVN_DAV__COMMIT_REVS_FOR_MERGE_RANGES_REPORT \
> + "commit-revs-for-merge-ranges-report"
This isn't really about your change, but I just noticed that in
subversion/libsvn_ra_neon/mergeinfo.c:mergeinfo_report_elements, we
have:
static const svn_ra_neon__xml_elm_t mergeinfo_report_elements[] =
{
{ SVN_XML_NAMESPACE, SVN_DAV__MERGEINFO_REPORT, ELEM_mergeinfo_report,
0 },
{ SVN_XML_NAMESPACE, SVN_DAV__COMMIT_REVS_FOR_MERGE_RANGES_REPORT,
ELEM_mergeinfo_report, 0 },
{ SVN_XML_NAMESPACE, "mergeinfo-item", ELEM_mergeinfo_item, 0 },
{ SVN_XML_NAMESPACE, "mergeinfo-path", ELEM_mergeinfo_path,
SVN_RA_NEON__XML_CDATA },
{ SVN_XML_NAMESPACE, "mergeinfo-info", ELEM_mergeinfo_info,
SVN_RA_NEON__XML_CDATA },
{ NULL }
};
...which surprisingly uses hardcoded strings, instead of the #defines
provided by subversion/include/private/svn_dav_protocol.h:
/** Names for XML child elements of the custom HTTP REPORTs understood
by mod_dav_svn, sans namespace. */
#define SVN_DAV__CREATIONDATE "creationdate"
#define SVN_DAV__MERGEINFO_ITEM "mergeinfo-item"
#define SVN_DAV__MERGEINFO_PATH "mergeinfo-path"
#define SVN_DAV__MERGEINFO_INFO "mergeinfo-info"
#define SVN_DAV__PATH "path"
#define SVN_DAV__INHERIT "inherit"
#define SVN_DAV__REVISION "revision"
#define SVN_DAV__MAX_COMMIT_REVISION "max-commit-revision"
#define SVN_DAV__MIN_COMMIT_REVISION "min-commit-revision"
#define SVN_DAV__MERGE_SOURCE "merge-source"
#define SVN_DAV__MERGE_TARGET "merge-target"
#define SVN_DAV__MERGE_RANGES "merge-ranges"
#define SVN_DAV__VERSION_NAME "version-name"
Do you know why this is? Is it just an oversight? If so, we should
probably correct it on trunk and port that change over to the branch.
Okay, in order to avoid wasting turnaround time (due to timezones),
I'm going to send this now, and continue reviewing. More to come...
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Nov 28 08:24:35 2007