Daniel Shahaf <d.s_at_daniel.shahaf.co.il> writes:
> Patch manager hat on.
>
> [...]
>
> Numerous stylistic violations here in the log message (four different
> issues, at least). Please pay more attention to adhering to our
> log message format; badly formatted log messages are as much of an
> obstacle to our processing a patch as any issue in the code itself.
> Thank you.
True... but these sorts of things are easy for us to fix as we read.
Bert, here's a new version of your log message, followed by some
comments on the code itself. My edits to the log message were:
- standardize line lengths
- put colons where they ought to be
- avoid repeating what new doc strings in the code say anyway
- fixed a few misspellings fixed
- describe implementation details less, describe behaviors more
- put periods put on the ends of sentences
- correct a few actual errors (see my "###" comments)
- put many sentences into the active voice (my personal crusade)
[[[
Move the creation of final display paths to the CLI notification
handler, to make sure notifications always receive a valid path in the
path field.
* subversion/include/svn_wc.h
(svn_wc_notify_t): New field path_prefix. Unrelatedly, note versions
in which other new fields were added.
### If possible, it's best to submit those other cleanups as a
### separate patch. I know it's slightly more work, but it really
### helps keep things simple for reviewers.
* subversion/libsvn_wc/util.c
(svn_wc_create_notify, svn_wc_dup_notify): Handle new path_prefix field.
* subversion/libsvn_client/client.h
(svn_client__do_commit): Document the new handling of notify_path_prefix.
* subversion/libsvn_client/commit_util.c
(svn_client__do_commit, do_item_commit) Remove local handling of
notify_path_prefix. Instead pass path_prefix to the notification
handler.
* subversion/libsvn_client/commit.c
(svn_client_commit4): Calculate a new-style notify prefix to pass to
svn_client__do_commit.
### Your original log message here said "Document the calculation of
### the notify prefix by renaming and adding a local variable." But
### there is no documentation change here, only a code change!
* subversion/svn/notify.c
(notify): Use the new path_prefix to split the first part of the path
if it matches the specified prefix. Don't bother testing for urls
as the path is documented to be a local path.
Patch by: Bert Huijben <b.huijben_at_competence.biz>
]]]
Okay, now comments on the code:
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 31025)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -903,12 +903,16 @@
> * In all other cases, it is @c SVN_INVALID_REVNUM. */
> svn_revnum_t revision;
> /** When @c action is @c svn_wc_notify_changelist_add or name. In all other
> - * cases, it is @c NULL. */
> + * cases, it is @c NULL. @since New in 1.5 */
> const char *changelist_name;
> /** When @c action is @c svn_wc_notify_merge_begin, and both the
> - left and right sides of the merge are from the same URL. In all
> - other cases, it is @c NULL. */
> + * left and right sides of the merge are from the same URL. In all
> + * other cases, it is @c NULL. @since New in 1.5 */
> svn_merge_range_t *merge_range;
> + /** If non-NULL, specifies an absolute path prefix that can be subtracted
> + * from the start of the absolute path in @c path to print paths without a
> + * common prefix. @since New in 1.6 */
> + const char *path_prefix;
> /* NOTE: Add new fields at the end to preserve binary compatibility.
> Also, if you add fields here, you have to update svn_wc_create_notify
> and svn_wc_dup_notify. */
The wording of the doc string is ambiguous: it suggest that we are
printing paths that fall into the class of "paths without a common
prefix" -- that is, the phrase "without a common prefix" sounds like
it's a adjectival phrase modifying "path", but you meant it to be an
adverbial phrase modifying "print". How about:
/** If non-NULL, specifies an absolute path prefix that can be subtracted
* from the start of the absolute path in @c path. Its purpose is to
* allow notification to remove a common prefix from all the paths
* displayed for an operation. @since New in 1.6 */
...instead, or something like that?
> --- subversion/libsvn_wc/util.c (revision 31025)
> +++ subversion/libsvn_wc/util.c (working copy)
> @@ -128,6 +128,7 @@
> ret->revision = SVN_INVALID_REVNUM;
> ret->changelist_name = NULL;
> ret->merge_range = NULL;
> + ret->path_prefix = NULL;
>
> return ret;
> }
> @@ -164,6 +165,8 @@
> ret->changelist_name = apr_pstrdup(pool, ret->changelist_name);
> if (ret->merge_range)
> ret->merge_range = svn_merge_range_dup(ret->merge_range, pool);
> + if (ret->path_prefix)
> + ret->path_prefix = apr_pstrdup(pool, ret->path_prefix);
>
> return ret;
> }
Glad you read the comment at the end of svn_wc_notify_t :-).
> --- subversion/libsvn_client/client.h (revision 31025)
> +++ subversion/libsvn_client/client.h (working copy)
> @@ -915,9 +915,9 @@
> CTX->NOTIFY_FUNC/CTX->BATON will be called as the commit progresses, as
> a way of describing actions to the application layer (if non NULL).
>
> - NOTIFY_PATH_PREFIX is used to send shorter, relative paths to the
> - notify_func (it's a prefix that will be subtracted from the front
> - of the paths.)
> + NOTIFY_PATH_PREFIX will be passed to the notification handler to
> + print shorter paths (it's a prefix that will be subtracted from the
> + front of absolute paths.)
>
> If the caller wants to keep track of any outstanding temporary
> files left after the transmission of text and property mods,
Hmmm. This isn't really your fault, but we've sort of mis-documented
NOTIFY_PATH_PREFIX here. We should just say that it is passed to
CTX->notify_func2(). Maybe we can say what we *think* the notify func
is likely to do with it, but we should have svn_wc_notify_func2_t
formally document the behavior, since svn_client__do_commit() actually
has no control over that.
> --- subversion/svn/notify.c (revision 31025)
> +++ subversion/svn/notify.c (working copy)
> @@ -55,13 +55,21 @@
> {
> struct notify_baton *nb = baton;
> char statchar_buf[5] = " ";
> - const char *path_local;
> + const char *path_local = n->path;
> svn_error_t *err;
> +
> + if (n->path_prefix)
> + {
> + if (strcmp(n->path->prefix, path_local))
> + path_local = svn_path_is_child(n->path_prefix, path_local, pool);
> + else
> + path_local = ".";
> +
> + if (!path_local)
> + path_local = n->path; /* if path is not below path_prefix */
> + }
Why check strcmp() before calling svn_path_is_child()? You could just
call svn_path_is_child() directly -- you'll either get NULL or the
remainder you're looking for, right?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-05-06 03:04:16 CEST