Re: svn commit: r1068169 - /subversion/trunk/tools/server-side/svn-populate-node-origins-index.c

Hyrum K Wright wrote on Tue, Feb 08, 2011 at 16:59:13 +0000:
> On Tue, Feb 8, 2011 at 3:03 AM, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
> > hwright_at_apache.org wrote on Mon, Feb 07, 2011 at 22:09:15 -0000:
> >> Author: hwright
> >> Date: Mon Feb  7 22:09:15 2011
> >> New Revision: 1068169
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1068169&view=rev
> >> Log:
> >> * tools/server-side/svn-populate-node-origins-index.c
> >>   (index_revision_adds): Update a deprecated function call, simplifying the
> >>     code a bit.
> >>
> >> Modified:
> >>     subversion/trunk/tools/server-side/svn-populate-node-origins-index.c
> >>
> >> Modified: subversion/trunk/tools/server-side/svn-populate-node-origins-index.c
> >> URL: http://svn.apache.org/viewvc/subversion/trunk/tools/server-side/svn-populate-node-origins-index.c?rev=1068169&r1=1068168&r2=1068169&view=diff
> >> ==============================================================================
> >> --- subversion/trunk/tools/server-side/svn-populate-node-origins-index.c (original)
> >> +++ subversion/trunk/tools/server-side/svn-populate-node-origins-index.c Mon Feb  7 22:09:15 2011
> >> @@ -77,7 +77,7 @@ index_revision_adds(int *count, svn_fs_t
> >>
> >>    *count = 0;
> >>    SVN_ERR(svn_fs_revision_root(&root, fs, revision, pool));
> >> -  SVN_ERR(svn_fs_paths_changed(&changes, root, pool));
> >> +  SVN_ERR(svn_fs_paths_changed2(&changes, root, pool));
> >>
> >>    /* No paths changed in this revision?  Nothing to do.  */
> >>    if (apr_hash_count(changes) == 0)
> >> @@ -88,7 +88,7 @@ index_revision_adds(int *count, svn_fs_t
> >>      {
> >>        const void *path;
> >>        void *val;
> >> -      svn_fs_path_change_t *change;
> >> +      svn_fs_path_change2_t *change;
> >>
> >>        svn_pool_clear(subpool);
> >>        apr_hash_this(hi, &path, NULL, &val);
> >> @@ -96,12 +96,8 @@ index_revision_adds(int *count, svn_fs_t
> >>        if ((change->change_kind == svn_fs_path_change_add)
> >>            || (change->change_kind == svn_fs_path_change_replace))
> >>          {
> >> -          const char *copyfrom_path;
> >> -          svn_revnum_t copyfrom_rev;
> >> -
> >> -          SVN_ERR(svn_fs_copied_from(&copyfrom_rev, &copyfrom_path,
> >> -                                     root, path, subpool));
> >> -          if (! (copyfrom_path && SVN_IS_VALID_REVNUM(copyfrom_rev)))
> >> +          if (! (change->copyfrom_path
> >
> > The doc string for that parameter says:
> >
> >  /** Copyfrom revision and path; this is only valid if copyfrom_known
> >   * is true. */
>
> Since we're not actually using the copyfrom_path or copyfrom_rev, just
> checking their validity, would the following patch then make sense?
>

IMO, no. This is the idiom in libsvn_repos:

      if (! change->copyfrom_known)
        {
          SVN_ERR(svn_fs_copied_from(&(change->copyfrom_rev),
                                     &(change->copyfrom_path),
                                     root, edit_path, pool));
          change->copyfrom_known = TRUE;
        }

In other words, I believe that the _known flag is "Did I fetch the
information", not "Does the FS have non-NULL values for this
information".

Daniel
(who learnt this the hard way)

> [[[
>
> Index: tools/server-side/svn-populate-node-origins-index.c
> ===================================================================
> --- tools/server-side/svn-populate-node-origins-index.c (revision 1068475)
> +++ tools/server-side/svn-populate-node-origins-index.c (working copy)
> @@ -96,8 +96,7 @@
> if ((change->change_kind == svn_fs_path_change_add)
> || (change->change_kind == svn_fs_path_change_replace))
> {
> - if (! (change->copyfrom_path
> - && SVN_IS_VALID_REVNUM(change->copyfrom_rev)))
> + if (!change->copyfrom_known)
> {
> svn_revnum_t origin;
> SVN_ERR(svn_fs_node_origin_rev(&origin, root, path, subpool));
> ]]]
>
> -Hyrum
Received on 2011-02-09 04:49:32 CET