Re: [PATCH] Was: Enhancement needed in svn status -u

Hello All,

Thanks for taking some time to look at this Philip. A new version of the
patch is attached based on your (and cmpilato's) comments.

Disclaimer: I'm running the tests on this patch now, plus I need to do
some manual testing. Come Monday if everything passes I'll resubmit a
formal patch and updated log message, I just wanted to get my initial
reply out.

For brevity I've removed the comments regarding indent formatting, these
are all fixed in the new patch.

Philip Martin <philip@codematters.co.uk> wrote on 09/22/2005 12:14:05 PM:

> Mark Phippard <MarkP@softlanding.com> writes:
>
> > Index: subversion/include/svn_wc.h
> > ===================================================================
> > --- subversion/include/svn_wc.h (revision 15823)
> > +++ subversion/include/svn_wc.h (working copy)
> > @@ -1516,6 +1516,29 @@
> > /** The entry's lock in the repository, if any. */
> > svn_lock_t *repos_lock;
> >
> > + /** Out of Date Info.
> > + *
> > + * If the working copy item is out of date compared to the
repository
> > + * the following five fields represent the HEAD state of the item
in
> > + * the repository. If not out of date, the items are set as
described
> > + * below.
> > + */
> > +
> > + /* If not out of date set to SVN_INVALID_REVNUM. */
> > + svn_revnum_t ood_last_cmt_rev;
> > +
> > + /* If not out of date set to 0. */
> > + apr_time_t ood_last_cmt_date;
> > +
> > + /* If not out of date set to svn_wc_status_none. */
> > + svn_node_kind_t ood_kind;
> > +
> > + /* If not out of date set to NULL. */
> > + const char *ood_url;
> > +
> > + /* If not out of date set to NULL. */
> > + const char *ood_last_cmt_author;
> > +
>
> You need to mark these fields as "@since New in 1.3".

Is this format appropriate?

  /** If not out of date set to SVN_INVALID_REVNUM.
   * @since New in 1.3
   */
  svn_revnum_t ood_last_cmt_rev;

> > } svn_wc_status2_t;
> >
> >
> > Index: subversion/libsvn_wc/status.c
> > ===================================================================
> > --- subversion/libsvn_wc/status.c (revision 15823)
> > +++ subversion/libsvn_wc/status.c (working copy)
> > @@ -33,6 +33,7 @@
> > #include "svn_io.h"
> > #include "svn_wc.h"
> > #include "svn_config.h"
> > +#include "svn_time.h"
> > #include "svn_private_config.h"
> >
> > #include "wc.h"
> > @@ -132,6 +133,13 @@
> >
> > /* The pool in which this baton itself is allocated. */
> > apr_pool_t *pool;
> > +
> > + /* Out of Date info corresponding to ood_* fields in
svn_wc_status2_t. */
> > + svn_revnum_t ood_last_cmt_rev;
> > + apr_time_t ood_last_cmt_date;
> > + svn_node_kind_t ood_kind;
> > + const char *ood_url;
> > + const char *ood_last_cmt_author;
> > };
> >
> >
> > @@ -166,6 +174,12 @@
> > the code that syncs up the adm dir and working copy. */
> > svn_boolean_t prop_changed;
> >
> > + /* Out of Date info corresponding to ood_* fields in
svn_wc_status2_t. */
> > + svn_revnum_t ood_last_cmt_rev;
> > + apr_time_t ood_last_cmt_date;
> > + svn_node_kind_t ood_kind;
> > + const char *ood_url;
> > + const char *ood_last_cmt_author;
> > };
> >
> >
> > @@ -282,6 +296,11 @@
> > }
> >
> > stat->repos_lock = repos_lock;
> > + stat->ood_last_cmt_rev = SVN_INVALID_REVNUM;
> > + stat->ood_last_cmt_date = 0;
> > + stat->ood_kind = svn_wc_status_none;
> > + stat->ood_url = NULL;
> > + stat->ood_last_cmt_author = NULL;
> >
> > *status = stat;
> > return SVN_NO_ERROR;
> > @@ -466,6 +485,11 @@
> > stat->switched = switched_p;
> > stat->copied = entry->copied;
> > stat->repos_lock = repos_lock;
> > + stat->ood_last_cmt_rev = SVN_INVALID_REVNUM;
> > + stat->ood_last_cmt_date = 0;
> > + stat->ood_kind = svn_wc_status_none;
> > + stat->ood_url = NULL;
> > + stat->ood_last_cmt_author = NULL;
> >
> > *status = stat;
> >
> > @@ -1038,12 +1062,17 @@
> > full_path = apr_pstrdup (pool, eb->anchor);
> >
> > /* Finish populating the baton members. */
> > - d->path = full_path;
> > - d->name = path ? (svn_path_basename (path, pool)) : NULL;
> > - d->edit_baton = edit_baton;
> > - d->parent_baton = parent_baton;
> > - d->pool = pool;
> > - d->statii = apr_hash_make (pool);
> > + d->path = full_path;
> > + d->name = path ? (svn_path_basename (path, pool)) :
NULL;
> > + d->edit_baton = edit_baton;
> > + d->parent_baton = parent_baton;
> > + d->pool = pool;
> > + d->statii = apr_hash_make (pool);
> > + d->ood_last_cmt_rev = SVN_INVALID_REVNUM;
> > + d->ood_last_cmt_date = 0;
> > + d->ood_kind = svn_wc_status_none;
> > + d->ood_url = NULL;
> > + d->ood_last_cmt_author = NULL;
> >
> > /* Get the status for this path's children. Of course, we only
want
> > to do this if the path is versioned as a directory. */
> > @@ -1098,12 +1127,16 @@
> > full_path = apr_pstrdup (pool, eb->anchor);
> >
> > /* Finish populating the baton members. */
> > - f->path = full_path;
> > - f->name = svn_path_basename (path, pool);
> > - f->pool = pool;
> > - f->dir_baton = pb;
> > - f->edit_baton = eb;
> > -
> > + f->path = full_path;
> > + f->name = svn_path_basename (path, pool);
> > + f->pool = pool;
> > + f->dir_baton = pb;
> > + f->edit_baton = eb;
> > + f->ood_last_cmt_rev = SVN_INVALID_REVNUM;
> > + f->ood_last_cmt_date = 0;
> > + f->ood_kind = svn_wc_status_none;
> > + f->ood_url = NULL;
> > + f->ood_last_cmt_author = NULL;
> > return f;
> > }
> >
> > @@ -1276,6 +1309,33 @@
> > }
> >
> >
> > +/* Copy out of date info from dir_baton or file_baton
> > + to svn_wc_status2 structure. */
> > +static void
> > +copy_ood_info(void *baton, svn_wc_status2_t *status, svn_boolean_t
is_dir)
> > +{
> > + if (baton && status)
> > + if (is_dir)
> > + {
> > + struct dir_baton *b = baton;
> > + status->ood_last_cmt_rev = b->ood_last_cmt_rev;
> > + status->ood_last_cmt_date = b->ood_last_cmt_date;
> > + status->ood_kind = b->ood_kind;
> > + status->ood_url = b->ood_url;
> > + status->ood_last_cmt_author = b->ood_last_cmt_author;
> > + }
> > + else
> > + {
> > + struct file_baton *b = baton;
> > + status->ood_last_cmt_rev = b->ood_last_cmt_rev;
> > + status->ood_last_cmt_date = b->ood_last_cmt_date;
> > + status->ood_kind = b->ood_kind;
> > + status->ood_url = b->ood_url;
> > + status->ood_last_cmt_author = b->ood_last_cmt_author;
> > + }
> > +}
> > +
> > +
> >
/*----------------------------------------------------------------------*/
> > 
> > /*** The callbacks we'll plug into an svn_delta_editor_t structure.
***/
> > @@ -1369,9 +1429,19 @@
> >
> > SVN_ERR (svn_wc_entries_read (&entries, adm_access, FALSE, pool));
> > if (apr_hash_get (entries, hash_key, APR_HASH_KEY_STRING))
> > + {
> > + struct svn_wc_status2_t *sb = apr_hash_get (db->statii,
full_path,
> > + APR_HASH_KEY_STRING);
>
> Is full_path guaranteed to be present in the hash?

This functionality is now in tweak_statushash, see below.
 
> > SVN_ERR (tweak_statushash (db->statii, eb->adm_access,
> > full_path, kind == svn_node_dir,
> > svn_wc_status_deleted, 0, NULL));
> > + /* When a directory loses a file entry, close_file is
> > + not called. So copy the available out of date info now.
> > + TODO: Obtain the last committed date and author. */
>
> What about that TODO?

In my original posting of this patch I mentioned there was, "One known
problem with the patch: It doesn't pick up the last committed author and
date for a WC item that has been deleted in the repository."

Unfortunately I haven't come up with any new ideas on this.

Also, in looking at this again I realize that setting ood_last_cmt_rev to
revision is incorrect, and that like last_cmt_date and last_cmt_author,
this information is not readily available for deleted items.

Having said all that, this change is gone in the new patch. I pursued
your idea to move copy_ood_info's functionality into tweak_statushash. The
last commit rev, author, and date still are still not obtained for deleted
items there, so this "TODO" lives on implicitly...but fortunately this
missing info isn't a serious problem as regards the original
Subclipse/JavaHL problem this patch aims to address; per Mark Phippard
this info isn't essential for deleted items, it qualified more as a "nice
to have".

> > + sb->ood_last_cmt_rev = revision;
> > + sb->ood_kind = sb->entry->kind;
> > + sb->ood_url = sb->entry->url;
>
> Why not call copy_ood_info?

Pursuing your idea to move copy_ood_info into tweak_statushash (see below)
this is now N/A.

> > + }
> >
> > /* Mark the parent dir -- it lost an entry (unless that parent dir
> > is the root node and we're not supposed to report on the root
> > @@ -1431,6 +1501,26 @@
> > struct dir_baton *db = dir_baton;
> > if (svn_wc_is_normal_prop (name))
> > db->prop_changed = TRUE;
> > +
> > + /* Store out of date info. */
> > + db->ood_kind = svn_node_dir;
> > + if (!db->ood_url)
> > + db->ood_url = apr_pstrdup(db->pool, find_dir_url (db, pool));
> > + if ( strcmp (name, SVN_PROP_ENTRY_COMMITTED_REV) == 0)
> > + db->ood_last_cmt_rev = SVN_STR_TO_REV(value->data);
> > + else if ( strcmp (name, SVN_PROP_ENTRY_LAST_AUTHOR) == 0)
> > + {
> > + db->ood_last_cmt_author = apr_pstrdup(db->pool, value->data);
> > + }
> > + else if ( strcmp (name, SVN_PROP_ENTRY_COMMITTED_DATE) == 0)
> > + {
> > + const char *time_str;
> > + apr_time_t time;
> > + SVN_ERR (svn_time_from_cstring (&time, value->data, db->pool));
> > + time_str = svn_time_to_cstring (time, db->pool);
> > + db->ood_last_cmt_date = time;
> > + }
> > +
> > return SVN_NO_ERROR;
> > }
> >
> > @@ -1474,6 +1564,12 @@
> > db->path, TRUE,
> > repos_text_status,
> > repos_prop_status, NULL));
> > +
> > + /* Something has changed so copy the out of date info. */
> > + if(pb)
> > + copy_ood_info(db,
> > + apr_hash_get (pb->statii, db->path,
> APR_HASH_KEY_STRING),
>
> Perhaps copy_ood_info should be part of tweak_statushash?

Yes, this was doable, though I didn't see any way around making some minor
changes to the args passed to tweak_statushash.
 
> > + TRUE);
> > }
> >
> > /* Handle this directory's statuses, and then note in the parent
> > @@ -1608,6 +1704,31 @@
> > struct file_baton *fb = file_baton;
> > if (svn_wc_is_normal_prop (name))
> > fb->prop_changed = TRUE;
> > +
> > + /* Store out of date info. */
> > + fb->ood_kind = svn_node_file;
> > + if (!fb->ood_url)
> > + {
> > + const char *base
> > + = svn_path_uri_encode (svn_path_basename (fb->path, pool),
pool);
> > + const char *url
> > + = svn_path_join(find_dir_url (fb->dir_baton, pool), base,
pool);
>
> svn_path_url_add_component perhaps?

Agreed, done.

> > + /* Copy into file_baton with a safe pool. */
> > + fb->ood_url = apr_pstrdup(fb->dir_baton->pool, url);
> > + }
> > + if ( strcmp (name, SVN_PROP_ENTRY_COMMITTED_REV) == 0)
> > + fb->ood_last_cmt_rev = SVN_STR_TO_REV(value->data);
> > + else if ( strcmp (name, SVN_PROP_ENTRY_LAST_AUTHOR) == 0)
> > + {
> > + fb->ood_last_cmt_author = apr_pstrdup(fb->dir_baton->pool,
> value->data);
> > + }
> > + else if ( strcmp (name, SVN_PROP_ENTRY_COMMITTED_DATE) == 0)
> > + {
> > + apr_time_t time;
> > + SVN_ERR (svn_time_from_cstring (&time, value->data,
> fb->dir_baton->pool));
> > + fb->ood_last_cmt_date = time;
> > + }
> > +
> > return SVN_NO_ERROR;
> > }
> >
> > @@ -1659,6 +1780,11 @@
> > repos_text_status,
> > repos_prop_status,
> > repos_lock));
> > +
> > + /* Add out of date info for this file in it's
> > + parent directory's statti hash. */
> > + copy_ood_info(fb, apr_hash_get (fb->dir_baton->statii, fb->path,
> > + APR_HASH_KEY_STRING), FALSE);
>
> Again, perhaps it should be part of tweak_statushash.
> >
> > return SVN_NO_ERROR;
> > }
> > Index: subversion/libsvn_repos/reporter.c
> > ===================================================================
> > --- subversion/libsvn_repos/reporter.c (revision 15823)
> > +++ subversion/libsvn_repos/reporter.c (working copy)
> > @@ -770,7 +770,7 @@
> > e_fullpath = svn_path_join (e_path, s_entry->name,
subpool);
> > if (b->recurse || s_entry->kind != svn_node_dir)
> > SVN_ERR (b->editor->delete_entry (e_fullpath,
> > - SVN_INVALID_REVNUM,
> > + s_rev,
>
> That looks like it could have quite far reaching implications, does it
> change the data sent over ra_dav and ra_svn? Will new clients work
> with old servers, will old clients work with new servers?

As I mentioned above I was mistaken that I could get the last committed
rev number for deletes, so this change is gone.

> --
> Philip Martin

Thanks again,

Paul B.

_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org