Hi,
I've been looking at the second half of the open issue 571
There is a patch posted at the end that filters the output of the
command line client, but doesn't use a hash table.
I'm trying to decide if this is the best method.
- or -
Looking into the internals of how the update method(s) work to add a new
enumeration to svn_wc_notify_t such as:
svn_wc_notify_update_replace
I am having some problems understanding the notification handlers, but
am slowly plugging away.
The new type could easily be handled by
just like all the other types of
notifications are.
Thoughts, comments, ideas?
Chris

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.

I'm helping a friend build a shed tomorrow so I won't be around for most
of the day.

Cheers,
Chris

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

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.

I'm helping a friend build a shed tomorrow so I won't be around for
most
of the day.

Cheers,
Chris

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 NT READY T BE CMMITTED.

Closure of the second half of issue 571, "svn update: D+A should say R"

Patch By: Chris Ness <chris (AT) nesser (DOT) org>
Design By: eh
Review By:

* subversion/include/svn_wc.h
(svn_wc_notify_action_t): New enumerated type
svn_wc_notify_update_replace
*
(): 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.
*
(notify): New case to handle svn_wc_notify_update_replace
]]]

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

Cheers,
Chris

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.

Christopher Ness <chris (AT) nesser (DOT) orgwrites:

* subversion/include/svn_wc.h
(svn_wc_notify_action_t): New enumerated type
svn_wc_notify_update_replace

It's not a new enumerated type, it's a new value in an existing type.

Such a change may well contravene Subversion's ABI compatability
guidelines; it's likely to break existing notification callbacks. I
see that we added locking values to svn_wc_notify_action_t in 1.2, but
that didn't break things in quite the same way. A 1.1 client using
1.2 libraries would continue to work so long as no locks were
involved, whereas your proposed change is likely to break 1.2 clients
using 1.3 libraries. You may need to rev the notification API.

Tue, 2005-09-13 at 16:47 +0100, Philip Martin wrote:
Christopher Ness <chris (AT) nesser (DOT) orgwrites:

* subversion/include/svn_wc.h
(svn_wc_notify_action_t): New enumerated type
svn_wc_notify_update_replace

It's not a new enumerated type, it's a new value in an existing type.

Your correct, my mistake.

Such a change may well contravene Subversion's ABI compatability
guidelines; it's likely to break existing notification callbacks. I
see that we added locking values to svn_wc_notify_action_t in 1.2, but
that didn't break things in quite the same way. A 1.1 client using
1.2 libraries would continue to work so long as no locks were
involved, whereas your proposed change is likely to break 1.2 clients
using 1.3 libraries. You may need to rev the notification API.

At first I had the new value in the middle of the enumerated list which
caused the values after to be n+1 in the binary. Eric mentioned that I
was breaking binary compatibility.

In this patch the new value comes at the end of the list and therefore
all values for 1.{1,2} shall be the same as this.

Cheers,
Chris

"Peter N. Lundblad" <peter (AT) famlundblad (DOT) sewrites:

Tue, 13 Sep 2005, Philip Martin wrote:
>
>Christopher Ness <chris (AT) nesser (DOT) orgwrites:
>>
>Such a change may well contravene Subversion's ABI compatability
>guidelines; it's likely to break existing notification callbacks. I
>see that we added locking values to svn_wc_notify_action_t in 1.2, but
>that didn't break things in quite the same way. A 1.1 client using
>1.2 libraries would continue to work so long as no locks were
>involved, whereas your proposed change is likely to break 1.2 clients
>using 1.3 libraries. You may need to rev the notification API.
>>
>>
! N again, please! I think you're a bit extreme when you claim that
we break ABI by just adding an enumeration value (K, in a very strict
sense, but we're a library, not a standards body:-). But in this case,
when you *replace* some values with a new value, we would break ABI in a
way that isn't acceptable.

Perhaps my point wasn't clear.

Adding the enum value does not break the ABI, it's the rest of the
patch that does that: when it stops sending 'D'+'A' and sends the
newly introduced 'R' instead. The patch modifies the svn client to
cope, but obviously third-party clients will still get broken.

To be acceptable the patch needs to provide an interface that causes
unmodified clients to continue to receive 'D'+'A' while allowing more
up to date clients to receive 'R'. That probably means new fields in
svn_client_ctx_t.

But, why can't this stuff be kept in the CLI
code. We could leave it up to each notifier implementation to coelsce D/A
into R.

That would be another way to do it, although it would mean every
individual client has to implement it separately.

Good evening,

After reading some of the comments and thinking this through again I
have to agree with Max that queuing up the DELETE messages is a bad
idea.

So I think this patch is dead in the water. I put a link to the ML in
the issue, but some more design would be needed.
#desc12

I think the best solution is to get the server to generate the replaced
message (somehow) but that knowledge is a bit beyond me right now.

I do suggest that the new enumeration value be added for the 1.3 release
if that is an appropriate place for binary compatibility.

Cheers,
Chris