Karl Fogel wrote:
> 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 :-) ).
>
>
Sounds Fine.
> 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
>
Looks good. Thanks.
>
>> 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).
>
>
Other 2 public functions also was having such a duplicate DocString.
Fixed in r28114.
> 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! :-)
>
>
Fixed in r28115.
>> --- 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.
>
Yes it seems like an oversight in neon, serf uses the macros.
>
> Okay, in order to avoid wasting turnaround time (due to timezones),
> I'm going to send this now, and continue reviewing. More to come...
>
>
Thanks.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Nov 28 14:01:18 2007