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

> -----Original Message-----
> From: Karl Fogel [mailto:kfogel_at_red-bean.com]
> Sent: dinsdag 6 mei 2008 3:04
> To: Daniel Shahaf
> Cc: Bert Huijben; dev_at_subversion.tigris.org; Mark Phippard
> Subject: Re: [PATCH] RE: Commit strips common prefix instead of
current
> directory (Issue #3168)
>
> 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.

I would have expected only a functional review at this time (Not one of
the actual implementation ;-).

> 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)

Thanks..

> [[[
> 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.

If I had used two patches they would not apply cleanly if used
separately (and once applied I merge this patch in the SharpSvn builds
to fix some AnkhSvn issues).
(Didn't change this for the updated patch)

> * 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!

I added an extra variable and renamed another; no functional changes
here..
(And copied your text into the new log message)

> * 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>
> ]]]

New version attached

>
> 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?

Copied literally in the new patch

>
> > --- 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 :-).

I have to know this code for the SharpSvn bindings and the AnkhSVN
status cache updating ;)

> > --- 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.

The new patch contains:
+ NOTIFY_PATH_PREFIX will be passed to CTX->notify_func2() as common
absolute
+ path prefix of the committed absolute paths.

Your suggested text for svn_wc_notify_t should fix the other part.

> > --- 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?

I wrote this code at least three times in different forms. Finally I
concluded to use the original form from the commit code to make sure no
unnecessary allocations were made.

After making sure that svn_path_is_child not do any unneeded allocations
when returning NULL I updated the code to check the exact match after
the svn_path_is_child call. (That code path should be only accessed once
during a commit; but using "." in all cases where path_local is NULL
might obsure future bugs in other places that use this new field)

> -Karl

Thanks for your review.

(New log message and new patch attached to stop Outlook updating the
layout)

        Bert

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org