[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] RE: Commit strips common prefix instead of current directory (Issue #3168)

From: Karl Fogel <kfogel_at_red-bean.com>
Date: Mon, 05 May 2008 21:03:35 -0400

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

This is an archived mail posted to the Subversion Dev mailing list.