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

Re: [INTERM PATCH] Issue 571: A+D should be R in update

From: Christopher Ness <chris_at_nesser.org>
Date: 2005-09-12 14:10:33 CEST

On Sun, 2005-09-11 at 21:42 -0500, Ben Collins-Sussman wrote:
> Can you resend with a log message, as described in our hacking.html
> file?
>
> (You can also run 'svn log' on the svn repository for 16,000 examples.)

I do not want this committed. It isn't ready. There are problems that
I need help solving.

But if it will help with the review process I'll add a log message.

[[[
THIS PATCH IS NOT READY TO BE COMMITTED.

Closure of the second half of issue 571, "svn update: D+A should say R"
  http://subversion.tigris.org/issues/show_bug.cgi?id=571

Patch By: Chris Ness <chris@nesser.org>
Design By: eh
Review By:

* subversion/include/svn_wc.h
  (svn_wc_notify_action_t): New enumerated type
   svn_wc_notify_update_replace
* subversion/libsvn_wc/update_editor.c
  (): New defines
  (struct dir_baton): New hash table deleted_targets
  (make_dir_baton): Initialize hash table when a new dir_baton is
   created.
  (do_entry_deletion): No longer notifies about file deletions, this is
   delayed by use of the hash table.
  (delete_entry): Adds to hash table, and if key exists notifies of
   a replacement of a file and removes that key from the hash.
  (close_directory): Walks hash for leftover keys marked as deleted
   and notifies them as deleted. Ignores added hashes.
   Removes entries from the hash.
  (close_file): Adds targets to the hash as added since reverse merges
   of replaced files can also occur. If target exists it notifies of
   a replacement and removes the target from the hash.
  (hash_get_set): New local function which tries to return the hash
   key. If not found it adds the target to the hash table.
* subversion/clients/cmdline/notify.c
  (notify): New case to handle svn_wc_notify_update_replace
]]]

Reattaching the patch.
For the test repos, please see grandparent post.

Cheers,
Chris

> On Sep 11, 2005, at 7:39 PM, Christopher Ness wrote:
>
> > Hi All,
> >
> > This is my first crack at this issue. I've still got some problems.
> >
> > I'm going to review the parts of the patch I'm having trouble with,
> > and
> > have attached the complete patch for your perusal.
> >
> > I have also attached my test repository with the dump file.
> > r1:2 is a replace of trunk/file.c
> >
> > Much thanks to `eh' on the IRC channel for his patience and guidance!
> >
> >
> > @@ -50,6 +50,12 @@
> > #include "lock.h"
> > #include "props.h"
> >
> > +#define ADD "add"
> > +#define DEL "del"
> > +#define NESS_DEBUG 1
> > +
> > +const char * hash_get_set (apr_hash_t * ht, const void * key,
> > + const char * add_or_del);
> >
> > Not sure what to do with my own private functions and defines.
> > HACKING
> > seems to be quiet on the matter. But I haven't seen any other
> > functions
> > in code - so does everything go into header files?
> >
> > Comment out the NESS_DEBUG define to stop the verbose trace lines.
> >
> > @@ -995,18 +1013,6 @@
> > logfile_path, parent_path, pool));
> > *log_number = 0;
> >
> > - /* The passed-in `path' is relative to the anchor of the edit,
> > so if
> > - * the operation was invoked on something other than ".", then
> > - * `path' will be wrong for purposes of notification. However, we
> > - * can always count on the parent_path being the parent of
> > base_name,
> > - * so we just join them together to get a good notification path.
> > - */
> > - if (eb->notify_func)
> > - (*eb->notify_func)
> > - (eb->notify_baton,
> > - svn_wc_create_notify (svn_path_join (parent_path,
> > base_name, pool),
> > - svn_wc_notify_update_delete, pool),
> > pool);
> >
> > Looking at this comment now I didn't use the svn_path_join() method
> > like
> > it warns against. Does this comment still apply today as it did in
> > 2002?
> >
> > Now to the part that is getting me.
> >
> > + for (hi = apr_hash_first(db->pool, db->deleted_targets); hi; hi
> > = apr_hash_next(hi))
> > + {
> > +#ifdef NESS_DEBUG
> > + i++;
> > +#endif
> > + const void *vkey;
> > + void *vval;
> > + const char *key;
> > + const svn_string_t *val;
> > +
> > + apr_hash_this(hi, &vkey, NULL, &vval);
> > + key = vkey;
> > + val = vval;
> > +#ifdef NESS_DEBUG
> > + fprintf(stderr, "[%d] hash key = '%s' was found with val = '%
> > s'\n", i, key, (char *)val);
> > +#endif
> > + if ((char*)val == DEL && db->edit_baton->notify_func)
> > + {
> > + (*db->edit_baton->notify_func)
> > + (db->edit_baton->notify_baton,
> > + svn_wc_create_notify (key,
> > + svn_wc_notify_update_delete, pool),
> > pool);
> > + }
> > + }
> >
> > I'm thinking the const char * is killing me. In my testing it is
> > giving
> > the same entries (yes, I know what the constant keyword means), and
> > not
> > seeming to loop though the hash table as I expected for the key
> > values.
> > But this is what the function arp_hash_this() returns for the key!
> >
> > How can I get around the const void * above and loop through the hash
> > table with proper keys returned? This seems rather common to me to
> > want
> > to get back the keys you entered.
> >
> > The next loop is bigger and uglier than needed but it tries to delete
> > the entries in the loop and probably fails to do so since the keys do
> > not appear to be updating.
> >
> >
> >
> > Comments and hints on beating this apr_hash_this() problem please.

-- 
PGP Public Key: http://www.nesser.org/pgp-key/
07:42:10 up 21 min, 2 users, load average: 0.70, 0.42, 0.17

Received on Mon Sep 12 14:11:21 2005

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