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

Re: [PATCH] partial fix for issue 571

From: Branko Čibej <brane_at_xbc.nu>
Date: 2002-02-25 19:21:13 CET

Garrett Rooney wrote:

>Here's a patch that fixes half of issue 571. The issue is about
>printing out notification that a file has been replaced, rather than
>having a notification that it was deleted followed by a notification
>that the same file was added. This patch fixes it for commit, and if
>people think this is the right approach, I'll do something similar for
>update.
>
>-garrett
>
>Index: ./subversion/clients/cmdline/trace-commit.c
>===================================================================
>--- ./subversion/clients/cmdline/trace-commit.c
>+++ ./subversion/clients/cmdline/trace-commit.c Sun Feb 24 20:46:35 2002
>@@ -44,6 +44,22 @@
> };
>
>
>+/* When we add and delete files, we don't immediately print out that info,
>+ because if a file is both added and deleted, we want to say it was
>+ replaced. So instead, we have 2 hashes in the dir baton, one for adds
>+ and one for deletes. For deletes, the key is all that matters, since
>+ we just need to know the entity's name. For adds, the val is used to
>+ tell us if the file was binary or not. (We use the pointer as a flag,
>+ it doesn't actually point to anything) We can't just store the value of
>+ the file's binary flag, because FALSE is equal to NULL, and that means
>+ 'erase this entry' in apr_hash_set. So instead we use this enum. */
>+enum file_type
>+{
>+ binary = 1,
>+ text = 2
>+};
>
I don't think you need this at all, see below.

>struct dir_baton
> {
> struct edit_baton *edit_baton;
>@@ -51,6 +67,8 @@
> svn_stringbuf_t *path;
> svn_boolean_t added;
> svn_boolean_t prop_changed;
>+ apr_hash_t *entries_added;
>+ apr_hash_t *entries_deleted;
>
You don't need two hashes. You need only one. What you do is, every time
you get an "add" or "delete", you:

    * Check the hash. If the entry is already there, change its state to
      "replaced".
    * Otherwise just add the entry to the hash.

>
> apr_pool_t *subpool;
> int ref_count;
> };
>@@ -80,6 +98,34 @@
> if (db->ref_count == 0)
> {
> struct dir_baton *dbparent = db->parent_dir_baton;
>+ apr_hash_index_t *hi;
>+ const void *key;
>+ void *val;
>+
>+ for (hi = apr_hash_first(db->subpool, db->entries_added);
>+ hi;
>+ hi = apr_hash_next(hi))
>+ {
>+ apr_hash_this(hi, &key, NULL, &val);
>+ if (apr_hash_get(db->entries_deleted, key, APR_HASH_KEY_STRING)
>+ != NULL)
>+ {
>+ printf("Replacing %s\n", (const char *)key);
>+ apr_hash_set(db->entries_deleted, key, APR_HASH_KEY_STRING, NULL);
>+ }
>+ else
>+ printf("Adding %s %s\n",
>+ ((enum file_type)val) == binary ? "(bin)" : " ",
>+ (const char *)key);
>+ }
>+
>+ for (hi = apr_hash_first(db->subpool, db->entries_deleted);
>+ hi;
>+ hi = apr_hash_next(hi))
>+ {
>+ apr_hash_this(hi, &key, NULL, NULL);
>+ printf("Deleting %s\n", (const char *)key);
>+ }
>
It's much simpler to just add the whole entry to the hash, see below ...

>
> /* Destroy all memory used by this baton, including the baton
> itself! */
>@@ -107,6 +153,9 @@
> rb->subpool = subpool;
> rb->ref_count = 1;
>
>+ rb->entries_added = apr_hash_make(subpool);
>+ rb->entries_deleted = apr_hash_make(subpool);
>+
> *root_baton = rb;
>
> return SVN_NO_ERROR;
>@@ -122,7 +171,9 @@
>
> svn_path_add_component (printable_name, name);
>
>- printf ("Deleting %s\n", printable_name->data);
>+ apr_hash_set(d->entries_deleted, printable_name->data, APR_HASH_KEY_STRING,
>+ printable_name->data);
>+
> return SVN_NO_ERROR;
> }
>
>@@ -150,7 +201,12 @@
> child_d->ref_count = 1;
> parent_d->ref_count++;
>
>- printf ("Adding %s\n", child_d->path->data);
>+ child_d->entries_added = apr_hash_make(subpool);
>+ child_d->entries_deleted = apr_hash_make(subpool);
>+
>+ apr_hash_set(parent_d->entries_added, child_d->path->data,
>+ APR_HASH_KEY_STRING, child_d->path->data);
>+
> *child_baton = child_d;
>
> return SVN_NO_ERROR;
>@@ -178,6 +234,9 @@
> child_d->ref_count = 1;
> parent_d->ref_count++;
>
>+ child_d->entries_added = apr_hash_make(subpool);
>+ child_d->entries_deleted = apr_hash_make(subpool);
>+
> *child_baton = child_d;
>
> /* Don't print anything for a directory open -- this event is
>@@ -206,9 +265,9 @@
> struct file_baton *fb = file_baton;
>
> if (fb->added)
>- printf ("Adding %s %s\n",
>- fb->binary ? "(bin)" : " ",
>- fb->path->data);
>+ apr_hash_set(fb->parent_dir_baton->entries_added, fb->path->data,
>+ APR_HASH_KEY_STRING,
>+ fb->binary ? (void *)binary : (void *)text);
> else
> printf ("Sending %s\n", fb->path->data);
>

Right here. It you add 'fb', keyed off it's name, to the hash instead of
the binary/text flag, you can later: a) check fb->binary directly
instead of muching around with the enums; b) fix up 'added' to
'replaced', as I suggested above.

This may mean a bit more work up front, but the code would definitely be
cleaner. Not to mention that you'd have only one additional hash table
instead of two.

-- 
Brane Čibej   <brane_at_xbc.nu>   http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:37:09 2006

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.