Forgotten patch: qofid.diff

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

Forgotten patch: qofid.diff

Chris Shoemaker
Subject: [qofid.diff] Minor tweaks to Qof Code in src/engine.

Neil, I don't know if this patch messes up your plans.  The only
essential thing here is qof_collection_get_list().  If we have to, I
suppose we could pull this into the budget namespace until the next
qof release.  What do you think?

 * src/engine/qof-be-utils.h
   - avoid declare-after-statement
   - minor tweaks to ugly macros

 * src/engine/qofid.[ch]:
   - Collection's hash of entities was using guid->data as lookup key.
     The key is actually just guid everywhere else.
   - Introduced qof_collection_get_list() for getting GList of entity GUIDs.
   - line-wrap and whitespace fixes
   - add comment about problems with get/set_data interface.

 * src/engine/qofclass.c:
   - Catch uses of qof_class system without qof_class_init and throw error.
   - Comment that 3 qof_class functions are actually private.

 * src/engine/qofquery.c:
   - ENTER/LEAVE balance.


 src/engine/qof-be-utils.h |   22 +++++++-------
 src/engine/qofclass.c     |   69 +++++++++++++++++++++++++++-------------------
 src/engine/qofid.c        |   46 ++++++++++++++++++++----------
 src/engine/qofid.h        |   30 +++++++++++++++-----
 src/engine/qofquery.c     |    1
 5 files changed, 106 insertions(+), 62 deletions(-)

Index: gnucash/src/engine/qof-be-utils.h
===================================================================
--- gnucash.orig/src/engine/qof-be-utils.h
+++ gnucash/src/engine/qof-be-utils.h
@@ -52,7 +52,6 @@
  */
 
 #define QOF_BEGIN_EDIT(inst)                                        \
-  QofBackend * be;                                                  \
   if (!(inst)) return;                                              \
                                                                     \
   (inst)->editlevel++;                                              \
@@ -66,12 +65,13 @@
   ENTER ("(inst=%p)", (inst));                                      \
                                                                     \
   /* See if there's a backend.  If there is, invoke it. */          \
-  be = qof_book_get_backend ((inst)->book);                         \
-    if (be && qof_backend_begin_exists((be))) {                     \
-     qof_backend_run_begin((be), (inst));                           \
-  } else {                                                          \
-     /* We tried and failed to start transaction! */                \
-     (inst)->dirty = TRUE;                                          \
+  {                                                                 \
+      QofBackend * be;                                              \
+      be = qof_book_get_backend ((inst)->book);                     \
+      if (be && qof_backend_begin_exists(be))                       \
+          qof_backend_run_begin(be, (inst));                        \
+      else /* We tried and failed to start transaction! */          \
+          (inst)->dirty = TRUE;                                     \
   }                                                                 \
   LEAVE (" ");
 
@@ -109,8 +109,8 @@ gboolean qof_begin_edit(QofInstance *ins
   {                                                              \
     QofBackend * be;                                             \
     be = qof_book_get_backend ((inst)->book);                    \
-    if (be && qof_backend_begin_exists((be))) {                  \
-     qof_backend_run_begin((be), (inst));                        \
+    if (be && qof_backend_begin_exists(be)) {                    \
+        qof_backend_run_begin(be, (inst));                       \
     }                                                            \
     (inst)->editlevel = 0;                                       \
   }                                                              \
@@ -148,7 +148,7 @@ gboolean qof_commit_edit(QofInstance *in
                                                                  \
   /* See if there's a backend.  If there is, invoke it. */       \
   be = qof_book_get_backend ((inst)->book);                      \
-  if (be && qof_backend_commit_exists((be)))                     \
+  if (be && qof_backend_commit_exists(be))                       \
   {                                                              \
     QofBackendError errcode;                                     \
                                                                  \
@@ -157,7 +157,7 @@ gboolean qof_commit_edit(QofInstance *in
       errcode = qof_backend_get_error (be);                      \
     } while (ERR_BACKEND_NO_ERR != errcode);                     \
                                                                  \
-    qof_backend_run_commit((be), (inst));                        \
+    qof_backend_run_commit(be, (inst));                          \
     errcode = qof_backend_get_error (be);                        \
     if (ERR_BACKEND_NO_ERR != errcode)                           \
     {                                                            \
Index: gnucash/src/engine/qofclass.c
===================================================================
--- gnucash.orig/src/engine/qofclass.c
+++ gnucash/src/engine/qofclass.c
@@ -43,6 +43,44 @@ static gboolean clear_table (gpointer ke
   return TRUE;
 }
 
+static gboolean check_init (void)
+{
+    if (initialized) return TRUE;
+
+    PERR("You must call qof_class_init() before using qof_class.");
+    return FALSE;
+}
+
+/********************************************************************/
+/* PRIVATE PUBLISHED API FUNCTIONS */
+void
+qof_class_init(void)
+{
+  if (initialized) return;
+  initialized = TRUE;
+
+  classTable = g_hash_table_new (g_str_hash, g_str_equal);
+  sortTable = g_hash_table_new (g_str_hash, g_str_equal);
+}
+
+void
+qof_class_shutdown (void)
+{
+  if (!initialized) return;
+  initialized = FALSE;
+
+  g_hash_table_foreach_remove (classTable, clear_table, NULL);
+  g_hash_table_destroy (classTable);
+  g_hash_table_destroy (sortTable);
+}
+
+QofSortFunc
+qof_class_get_default_sort (QofIdTypeConst obj_name)
+{
+  if (!obj_name) return NULL;
+  return g_hash_table_lookup (sortTable, obj_name);
+}
+
 /********************************************************************/
 /* PUBLISHED API FUNCTIONS */
 
@@ -55,6 +93,7 @@ qof_class_register (QofIdTypeConst obj_n
   int i;
 
   if (!obj_name) return;
+  if (!check_init()) return;
 
   if (default_sort_function)
   {
@@ -83,31 +122,11 @@ qof_class_register (QofIdTypeConst obj_n
   }
 }
 
-void
-qof_class_init(void)
-{
-  if (initialized) return;
-  initialized = TRUE;
-
-  classTable = g_hash_table_new (g_str_hash, g_str_equal);
-  sortTable = g_hash_table_new (g_str_hash, g_str_equal);
-}
-
-void
-qof_class_shutdown (void)
-{
-  if (!initialized) return;
-  initialized = FALSE;
-
-  g_hash_table_foreach_remove (classTable, clear_table, NULL);
-  g_hash_table_destroy (classTable);
-  g_hash_table_destroy (sortTable);
-}
-
 gboolean
 qof_class_is_registered (QofIdTypeConst obj_name)
 {
   if (!obj_name) return FALSE;
+  if (!check_init()) return FALSE;
 
   if (g_hash_table_lookup (classTable, obj_name)) return TRUE;
 
@@ -122,6 +141,7 @@ qof_class_get_parameter (QofIdTypeConst
 
   g_return_val_if_fail (obj_name, NULL);
   g_return_val_if_fail (parameter, NULL);
+  if (!check_init()) return NULL;
 
   ht = g_hash_table_lookup (classTable, obj_name);
   if (!ht)
@@ -179,13 +199,6 @@ qof_class_get_parameter_type (QofIdTypeC
   return (prm->param_type);
 }
 
-QofSortFunc
-qof_class_get_default_sort (QofIdTypeConst obj_name)
-{
-  if (!obj_name) return NULL;
-  return g_hash_table_lookup (sortTable, obj_name);
-}
-
 /* ================================================================ */
 
 struct class_iterate {
Index: gnucash/src/engine/qofid.c
===================================================================
--- gnucash.orig/src/engine/qofid.c
+++ gnucash/src/engine/qofid.c
@@ -53,11 +53,11 @@ qof_entity_init (QofEntity *ent, QofIdTy
   g_return_if_fail (NULL != tab);
   
   /* XXX We passed redundant info to this routine ... but I think that's
- * OK, it might eliminate programming errors. */
+   * OK, it might eliminate programming errors. */
   if (safe_strcmp(tab->e_type, type))
   {
     PERR ("attempt to insert \"%s\" into \"%s\"", type, tab->e_type);
- return;
+    return;
   }
   ent->e_type = CACHE_INSERT (type);
 
@@ -158,8 +158,8 @@ void
 qof_collection_destroy (QofCollection *col)
 {
   CACHE_REMOVE (col->e_type);
-  g_hash_table_destroy(col->hash_of_entities);
   col->e_type = NULL;
+  g_hash_table_destroy(col->hash_of_entities);
   col->hash_of_entities = NULL;
   col->data = NULL;   /** XXX there should be a destroy notifier for this */
   g_free (col);
@@ -289,7 +289,7 @@ qof_collection_lookup_entity (QofCollect
   QofEntity *ent;
   g_return_val_if_fail (col, NULL);
   if (guid == NULL) return NULL;
-  ent = g_hash_table_lookup (col->hash_of_entities, guid->data);
+  ent = g_hash_table_lookup (col->hash_of_entities, guid);
   return ent;
 }
 
@@ -326,22 +326,21 @@ qof_collection_count (QofCollection *col
 gboolean
 qof_collection_is_dirty (QofCollection *col)
 {
-   if (!col) return FALSE;
-   return col->is_dirty;
+    return col ? col->is_dirty : FALSE;
 }
 
 void
 qof_collection_mark_clean (QofCollection *col)
 {
-   if (!col) return;
-   col->is_dirty = FALSE;
+    if (col)
+        col->is_dirty = FALSE;
 }
 
 void
 qof_collection_mark_dirty (QofCollection *col)
 {
-   if (!col) return;
-   col->is_dirty = TRUE;
+    if (col)
+        col->is_dirty = TRUE;
 }
 
 /* =============================================================== */
@@ -349,15 +348,14 @@ qof_collection_mark_dirty (QofCollection
 gpointer
 qof_collection_get_data (QofCollection *col)
 {
-   if (!col) return NULL;
-   return col->data;
+    return col ? col->data : NULL;
 }
 
 void
 qof_collection_set_data (QofCollection *col, gpointer user_data)
 {
-   if (!col) return;
-   col->data = user_data;
+    if (col)
+        col->data = user_data;
 }
 
 /* =============================================================== */
@@ -376,8 +374,8 @@ static void foreach_cb (gpointer key, gp
 }
 
 void
-qof_collection_foreach (QofCollection *col,
-                   QofEntityForeachCB cb_func, gpointer user_data)
+qof_collection_foreach (QofCollection *col, QofEntityForeachCB cb_func,
+                        gpointer user_data)
 {
   struct _iterate iter;
 
@@ -390,4 +388,20 @@ qof_collection_foreach (QofCollection *c
   g_hash_table_foreach (col->hash_of_entities, foreach_cb, &iter);
 }
 
+
+static void
+add_to_list(gpointer key, gpointer val, gpointer data) {
+    GList **list = data;
+    *list = g_list_append(*list, QOF_ENTITY(val));
+}
+
+GList *
+qof_collection_get_list(QofCollection *col) {
+    GList *list = NULL;
+    g_return_val_if_fail (col, NULL);
+
+    g_hash_table_foreach(col->hash_of_entities, add_to_list, &list);
+    return list;
+}
+
 /* =============================================================== */
Index: gnucash/src/engine/qofid.h
===================================================================
--- gnucash.orig/src/engine/qofid.h
+++ gnucash/src/engine/qofid.h
@@ -105,7 +105,8 @@ typedef const gchar* QofLogModule;
 })
 
 /** return TRUE if object is of the given type */
-#define QOF_CHECK_TYPE(obj,type) (0 == QSTRCMP((type),(((QofEntity *)(obj))->e_type)))
+#define QOF_CHECK_TYPE(obj,type) ( ((obj) != NULL) && \
+   (0 == QSTRCMP((type),(((QofEntity *)(obj))->e_type))))
 
 /** cast object to the indicated type, print error message if its bad  */
 #define QOF_CHECK_CAST(obj,e_type,c_type) (                   \
@@ -138,9 +139,9 @@ typedef struct QofCollection_s QofCollec
 
 struct QofEntity_s
 {
-   QofIdType        e_type;
- GUID             guid;
- QofCollection  * collection;
+  QofIdType        e_type;
+  GUID             guid;
+  QofCollection  * collection;
 };
 
 /** @name QOF Entity Initialization & Shutdown
@@ -178,14 +179,29 @@ QofEntity * qof_collection_lookup_entity
 typedef void (*QofEntityForeachCB) (QofEntity *, gpointer user_data);
 
 /** Call the callback for each entity in the collection. */
-void qof_collection_foreach (QofCollection *,
-                       QofEntityForeachCB, gpointer user_data);
+void qof_collection_foreach (QofCollection *, QofEntityForeachCB,
+                             gpointer user_data);
 
-/** Store and retreive arbitrary object-defined data
+/** Get a list of entities in the collection.  Caller owns the list. */
+GList * qof_collection_get_list(QofCollection *col);
+
+/** Store and retrieve arbitrary object-defined data
  *
  * XXX We need to add a callback for when the collection is being
  * destroyed, so that the user has a chance to clean up anything
  * that was put in the 'data' member here.
+ *
+ * CAS: Besides the potential leak, this interface is kind of bogus in
+ * other ways, too.  For example, using collection data is risky
+ * because even though the collection data is very public, its use
+ * must be universally consistent.  That's a visibility violation.
+ *
+ * Here's what I think is needed: Object implementations need to be
+ * allowed a say in how their collection is managed.  Maybe they can
+ * register functions that control what happens to the collection when
+ * an object is added to or removed from the book.  Then we can remove
+ * public access to the collection's data via these two functions here.
+ *
  */
 gpointer qof_collection_get_data (QofCollection *col);
 void qof_collection_set_data (QofCollection *col, gpointer user_data);
Index: gnucash/src/engine/qofquery.c
===================================================================
--- gnucash.orig/src/engine/qofquery.c
+++ gnucash/src/engine/qofquery.c
@@ -1273,6 +1273,7 @@ void qof_query_init (void)
   ENTER (" ");
   qof_query_core_init ();
   qof_class_init ();
+  LEAVE ("Completed initialization of QofQuery");
 }
 
 void qof_query_shutdown (void)
_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Neil Williams-2
On Tuesday 25 October 2005 3:18 pm, Chris Shoemaker wrote:
> Subject: [qofid.diff] Minor tweaks to Qof Code in src/engine.
>
> Neil, I don't know if this patch messes up your plans.

No, but I am curious about why some of it is needed.

Paid employment commitments prevented me replying earlier.

> The only
> essential thing here is qof_collection_get_list().  If we have to, I
> suppose we could pull this into the budget namespace until the next
> qof release.  What do you think?

I'm not sure about the GList - I'd like to know how you want to use it and why
qof_collection_foreach isn't adequate.

>    - Introduced qof_collection_get_list() for getting GList of entity
> GUIDs.

It's QofEntity that you are actually storing in the list rather than the GUID.
In which case, I'm not sure I see the advantage over qof_collection_foreach.

> @@ -158,8 +158,8 @@ void
>  qof_collection_destroy (QofCollection *col)
>  {
>    CACHE_REMOVE (col->e_type);
> -  g_hash_table_destroy(col->hash_of_entities);
>    col->e_type = NULL;
> +  g_hash_table_destroy(col->hash_of_entities);

(separate minor point):
Why move the g_hash_table_destroy call?

> +static void
> +add_to_list(gpointer key, gpointer val, gpointer data) {
> +    GList **list = data;
> +    *list = g_list_append(*list, QOF_ENTITY(val));
> +}
> +
> +GList *
> +qof_collection_get_list(QofCollection *col) {
> +    GList *list = NULL;
> +    g_return_val_if_fail (col, NULL);
> +
> +    g_hash_table_foreach(col->hash_of_entities, add_to_list, &list);
> +    return list;
> +}
Why do you need this GList call at all? I'm not sure I see why you need to
convert the QofCollection hashtable into a list. What does it gain compared
to qof_collection_foreach?

> +/** Get a list of entities in the collection.  Caller owns the list. */
> +GList * qof_collection_get_list(QofCollection *col);

To me, this defeats the main purposes of a primary collection - to keep the
e_type identifier and the GUID hash table key. You can't look up entities
from the converted list and the list isn't limited to only one e_type.

--

Neil Williams
=============
http://www.data-freedom.org/
http://www.nosoftwarepatents.com/
http://www.linux.codehelp.co.uk/


_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches

attachment0 (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Neil Williams-2
In reply to this post by Chris Shoemaker
On Tuesday 25 October 2005 3:18 pm, Chris Shoemaker wrote:
> Index: gnucash/src/engine/qofclass.c
> ===================================================================
> --- gnucash.orig/src/engine/qofclass.c
> +++ gnucash/src/engine/qofclass.c
> @@ -43,6 +43,44 @@ static gboolean clear_table (gpointer ke
>    return TRUE;
>  }

...

> +/********************************************************************/
> +/* PRIVATE PUBLISHED API FUNCTIONS */

These are private and it is useful to highlight the distinction, but
qofclass-p.h is not public and is not distributed/installed so I'll make this
simply
/* PRIVATE FUNCTIONS */


--

Neil Williams
=============
http://www.data-freedom.org/
http://www.nosoftwarepatents.com/
http://www.linux.codehelp.co.uk/


_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches

attachment0 (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Chris Shoemaker
In reply to this post by Neil Williams-2
On Wed, Oct 26, 2005 at 07:14:08PM +0100, Neil Williams wrote:

> On Tuesday 25 October 2005 3:18 pm, Chris Shoemaker wrote:
> > Subject: [qofid.diff] Minor tweaks to Qof Code in src/engine.
> >
> > Neil, I don't know if this patch messes up your plans.
>
> No, but I am curious about why some of it is needed.
>
> Paid employment commitments prevented me replying earlier.
>
> > The only
> > essential thing here is qof_collection_get_list().  If we have to, I
> > suppose we could pull this into the budget namespace until the next
> > qof release.  What do you think?
>
> I'm not sure about the GList - I'd like to know how you want to use it and why
> qof_collection_foreach isn't adequate.

It's certainly not strictly necessary, since I could always use
qof_collection_foreach to generate a GList.  I just noticed that other
objects were jumping through various hoops to obtain a GList like
this.  (For example, when they want to get a count of how many such
objects there are, or when writing them to file.)  I also use it to
populate a temporary ListStore for the Budget Selection Dialog.

In every place I used qof_collection_get_list, I could have just
written a static add_to_list callback, and used qof_collection_foreach
to get a GList, but I'm lazy and I didn't want to redo this every
place I needed it, nor even once in Budget's API.  Since other objects
were doing similar things, or worse (like managing a GList stored in
the collection's generic data field) I thought it might be convenient
for everybody.

Also, I didn't want to rely on known relationships letting me get a
GList of Budgets from some other object (like the book) as some
objects do.  And, I certainly didn't want to store a GList in the
generic book data ala commodity_table.

All said, it's just a convenience function.  If you don't like it, we
can just put it in the Budget API.  (Although there /is/ a weird
naming issue: other objects have a GList *
gnc_book_get_{thisobject}(QofBook *book) or something stranger.  That
just seems awkward and not too difficult to avoid if you have
something like qof_collection_get_list.)

>
> >    - Introduced qof_collection_get_list() for getting GList of entity
> > GUIDs.
>
> It's QofEntity that you are actually storing in the list rather than the GUID.
> In which case, I'm not sure I see the advantage over qof_collection_foreach.

Yeah, that description is outdated and irrelavant.  It just so happens
I use get GUIDs, among other things, from the entity.  I think an
earlier implementation actually returned a GList of GUIDs, but I
decided entities was more useful.

>
> > @@ -158,8 +158,8 @@ void
> >  qof_collection_destroy (QofCollection *col)
> >  {
> >    CACHE_REMOVE (col->e_type);
> > -  g_hash_table_destroy(col->hash_of_entities);
> >    col->e_type = NULL;
> > +  g_hash_table_destroy(col->hash_of_entities);
>
> (separate minor point):
> Why move the g_hash_table_destroy call?

Heh, actually, I moved the col->e_type = NULL.  :)  It's invalid
immediately, so it's best to drop the pointer immediately.

>
> > +static void
> > +add_to_list(gpointer key, gpointer val, gpointer data) {
> > +    GList **list = data;
> > +    *list = g_list_append(*list, QOF_ENTITY(val));
> > +}
> > +
> > +GList *
> > +qof_collection_get_list(QofCollection *col) {
> > +    GList *list = NULL;
> > +    g_return_val_if_fail (col, NULL);
> > +
> > +    g_hash_table_foreach(col->hash_of_entities, add_to_list, &list);
> > +    return list;
> > +}
>
> Why do you need this GList call at all? I'm not sure I see why you need to
> convert the QofCollection hashtable into a list. What does it gain compared
> to qof_collection_foreach?
>
> > +/** Get a list of entities in the collection.  Caller owns the list. */
> > +GList * qof_collection_get_list(QofCollection *col);
>
> To me, this defeats the main purposes of a primary collection - to keep the
> e_type identifier and the GUID hash table key. You can't look up entities
> from the converted list and the list isn't limited to only one e_type.

Well, you can, but that's not what it's for.  It's just another type
of iteration.  And sometimes, you *do* need a list.  It's just a
question of who should make it.

-chris
_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Neil Williams-2
On Wednesday 26 October 2005 8:48 pm, Chris Shoemaker wrote:

> On Wed, Oct 26, 2005 at 07:14:08PM +0100, Neil Williams wrote:
> > On Tuesday 25 October 2005 3:18 pm, Chris Shoemaker wrote:
> > > The only
> > > essential thing here is qof_collection_get_list().  If we have to, I
> > > suppose we could pull this into the budget namespace until the next
> > > qof release.  What do you think?
> >
> > I'm not sure about the GList - I'd like to know how you want to use it
> > and why qof_collection_foreach isn't adequate.
>
> It's certainly not strictly necessary, since I could always use
> qof_collection_foreach to generate a GList.
BTW: qof_collection_count already exists.

If we can avoid iterating over a hash_table to append to a GList which we then
iterate over again, I'd prefer that! (Especially as ALL the entities involved
are exactly the same pointers!)

Wherever we put this list function, it's going to mean repeated iteration over
the same entities.

Do you generate this list just once (on book load) or do you keep it in sync
with changing entities in the book? What about the collection?

Can you use the QofCollection for your object ID directly? Let that be your
GList and do all the iteration with qof_collection_foreach instead of using a
second structure in memory?

I can't help with specifics at the moment, I can't see your code (but that
reflects another, different, story).

Do you create budgets as QofEntities with qof_class_register and
qof_object_register? If you use qof_instance_init, the book already contains
a QofCollection and you wouldn't need to store a GList anywhere.

> I just noticed that other
> objects were jumping through various hoops to obtain a GList like
> this.  (For example, when they want to get a count of how many such
> objects there are,

use qof_collection_count.

It's relatively new, so older objects haven't been changed to use it yet but
new code should use it.

> or when writing them to file.)  I also use it to
> populate a temporary ListStore for the Budget Selection Dialog.

The file example is a little unclear. Certainly in QSF I use
qof_object_foreach_type and qof_object_foreach - that's the basis of the
generic structure. If you know the object type in advance (which you would
have to in order to get the collection) maybe qof_object_foreach would be
suitable? I don't see the example you mention about writing to a file in
gnc-backend-file.

> In every place I used qof_collection_get_list, I could have just
> written a static add_to_list callback, and used qof_collection_foreach
> to get a GList, but I'm lazy and I didn't want to redo this every
> place I needed it, nor even once in Budget's API.

I understand that. I'm just concerned that we would be iterating over the same
entities again and again in two different forms. It seems wasteful to me.

qof_collection_get_list may look convenient but I'm concerned that it's doing
more than it needs behind the simple call.

Can you adapt the functions that call qof_collection_get_list to call a
different callback using qof_collection_foreach - but without building a
GList in between?

Presumably, you get the GList then use an internal for loop or an external
g_list_foreach routine?

The external g_list_foreach callback is the easiest one to adapt. Change the
gpointer value to QofEntity *ent and remove the cast from gpointer to
QofEntity.

e.g. using a g_list_foreach and callback:

my_func_cb (gpointer value, gpointer data)
{
  // internal func using a context for other data, cast from data
}

my_func()
{
  QofCollection *col;
  GList *list;

  list = qof_collection_get_list(col);  /**< duplicate the hash_table as a
                                    list without changing any content */

  g_list_foreach(list, my_func_cb, context);
  g_list_free(list);
}

Or if you have internal for or while loops:

my_func ()
{
  QofCollection *col;
  GList *list;

  list = qof_collection_get_list(col);  /**< duplicate the hash_table as a
                                    list without changing any content */
  while (list)
  {
        // internal func
        list = g_list_next(list);
  }
  g_list_free(list);
}

The QofCollection version could be:

my_func_cb (QofEntity *ent, gpointer data)
{
  // internal func using a context for other data, cast from data
}

my_func()
{
  QofCollection *col;

  qof_collection_foreach(col, my_func_cb, context);
}

> Since other objects
> were doing similar things, or worse (like managing a GList stored in
> the collection's generic data field) I thought it might be convenient
> for everybody.

Maybe. I think we can exclude SX-book from the examples of similar problems as
the file itself declares it does not use entities in an efficient manner.

qof_collection_foreach uses the same kind of callback that you would use for
g_list_foreach - you just don't have to cast the gpointer back to
(QofEntity*).

I know it's handy to iterate using lists using a simple for loop but I'd
rather not have the QOF API encourage others to use double iteration over
other objects that may have thousands of entities per book.

Just imagine if qof_collection_get_list was called for GNC_ID_TRANS each time
a new one was added or modified.

> Also, I didn't want to rely on known relationships letting me get a
> GList of Budgets from some other object (like the book) as some
> objects do.  And, I certainly didn't want to store a GList in the
> generic book data ala commodity_table.

I can understand that too. However, a QofCollection of budgets is always being
maintained on your behalf, within the book.

I realise this is getting quite low level in your budget structure but I would
prefer not to have QOF providing a standard API that actually provides
duplicate iteration.

If you can use qof_instance_init, you have the collection created, updated and
maintained for you. I'm not sure you'd need to store anything in the book
data, it's already in the book as a QofCollection. (And, by definition, you
can write your budgets out in QSF).

In order to call qof_collection_get_list, you must already have such a
collection. I don't see why you want to iterate over that to append to a list
that you then iterate over again.

> All said, it's just a convenience function.  If you don't like it, we
> can just put it in the Budget API.

For now, that may be preferable but, IMHO, it would be better to use the
QofCollection directly and not create a duplicate structure.

> (Although there /is/ a weird
> naming issue:

If you need to use GList for budgets, maybe use this routine locally
temporarily but I think it should use QofCollection instead.

I can't see that qof_collection_get_list can be part of QOF.

> other objects have a GList *
> gnc_book_get_{thisobject}(QofBook *book) or something stranger.

I can find only one - SX-book. We already know that isn't using entities
properly, I don't think it's a good model for other objects.

Part of libqof1 and cashutil is to encourage better object design, improving
the API to deprecate methods that cause trouble later on.

> That
> just seems awkward and not too difficult to avoid if you have
> something like qof_collection_get_list.)

The basic problems, to me, are:

1. It's adding iteration on top of other iteration. We take a
g_hash_table_foreach to build a list using a callback and then iterate over
that list. If this was a QofCollection for GNC_ID_TRANS, it could take a
while! That's the risk with making it part of QOF - it gets used out of
context on objects that have nothing to do with budgets.

2. I'm not that happy about passing the GList back to the caller - there's
enough confusion about KVP and GLists because a GList cannot be relied upon
to identify or maintain any particular conventions about entries in the list.

3. I don't follow some of your examples.

> > > +static void
> > > +add_to_list(gpointer key, gpointer val, gpointer data) {
> > > +    GList **list = data;
> > > +    *list = g_list_append(*list, QOF_ENTITY(val));
> > > +}
> > > +
> > > +GList *
> > > +qof_collection_get_list(QofCollection *col) {
> > > +    GList *list = NULL;
> > > +    g_return_val_if_fail (col, NULL);
> > > +
> > > +    g_hash_table_foreach(col->hash_of_entities, add_to_list, &list);
> > > +    return list;
> > > +}
> >
> > Why do you need this GList call at all? I'm not sure I see why you need
> > to convert the QofCollection hashtable into a list. What does it gain
> > compared to qof_collection_foreach?
> >
> Well, you can, but that's not what it's for.  It's just another type
> of iteration.
I disagree, it's another *layer* of iteration on top of an existing layer.
It's not an alternative iteration, it's not equivalent - it is supplementary
and dependent on the first iteration without modifying any of the pointers
contained within either iteration.

> And sometimes, you *do* need a list.

Sorry, I can't see when that is necessary. Any examples from your own code?
It's hard to see the merit when I cannot appreciate your reasons for your
initial examples.

> It's just a
> question of who should make it.

Maybe. Or maybe it's a question of whether there is a method that only
involves a single layer of iteration.

--

Neil Williams
=============
http://www.data-freedom.org/
http://www.nosoftwarepatents.com/
http://www.linux.codehelp.co.uk/


_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches

attachment0 (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Chris Shoemaker

Wow!  You had me at double iteration!  :)

I'll convert to use qof_collection_foreach.

-chris
_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Neil Williams-2
On Wednesday 26 October 2005 11:27 pm, Chris Shoemaker wrote:
> Wow!  You had me at double iteration!  :)

:-))

As clear an example as I can imagine of how sharing code BEFORE it is "final
quality" is a wise choice for free software development! (Sorry it was your
code that got used for the illustration, Chris!)  ;-)

I should have been able to see your early code and you should be able to see
cashutil. Let's not let other new developers make the same mistakes next
time? All this work for a lack of documentation, easy to update webpages and
a little consensus on code management conventions.

We need quick and open ways to publish *early* code - pseudo code if you will.
Designs, plans, ideas. Rough hacks, concepts, logic. Code that is certain to
break the sacrosanct CoreBuild. Branches are part of the answer but, as Derek
pointed out, technology isn't the answer to sociological problems. The core
advance has to be that we leave our paranoia behind, kill the sacred cow of
"TheCoreBuild" and accept that you can't make an omelette without breaking
eggs. CVS is NOT guaranteed to always build, there's no rule that says so and
we mislead ourselves and prospective new developers if we pursue such a goal
*to the exclusion of all else*. Fine, have a HEAD branch that is the current
release and maybe a main development branch but sometimes that build will
break. It's a necessary step. It's a GOOD sign, it is to be managed not
eradicated as if it was some strain of avian flu.

We all look at our own code and think it's good - the whole point of free
software is that the code should be available for peer review BEFORE it gets
set in stone. It's about getting more peer review at earlier stages and if
that means committing code that is incomplete then that is a good thing. It
means it's easier to change, it's easier to solve fundamental problems that
do not appear to the sole developer because they are too close to the code.

If we stay fixated on "never break the build, no matter what" and "break the
build and you'll be flamed until you fix it", then we are not free.

Fear of making a commit undermines the very freedom to modify code that we so
eagerly protect in our collective fight against software patents.

Patent lawsuits and monopolists trade on fear. Proprietary software develops
in secret, hidden from view.

How is it that this part of the free software community have being doing the
same by hiding our most volatile code out-of-tree!!!

Can we *please* drop the pretence that breaking the build is a crime on a par
with treason? OK, it's not the best thing to do and we try to avoid it
whenever we can but that's as far as it should go. It's development code,
sometimes it will simply break. Live with it and be free.

> I'll convert to use qof_collection_foreach.

Glad I could help, Chris!
:-)

I'll happily put the rest of your diff into QOF and update gnucash in due
course. There's nothing there that desperately needs changing imminently.

I hope it's not too awkward to convert your code and I look forward to seeing
it in G2.

Let me know if I can help with the conversion.

--

Neil Williams
=============
http://www.data-freedom.org/
http://www.nosoftwarepatents.com/
http://www.linux.codehelp.co.uk/


_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches

attachment0 (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Chris Shoemaker
[WARNING, my response to Neil might get polemic.  If you've already
exceeded your daily tolerance for my suggestions, PLEASE JUST SKIP to
the patch.  I'm serious, really.]

On Thu, Oct 27, 2005 at 12:51:54AM +0100, Neil Williams wrote:

> On Wednesday 26 October 2005 11:27 pm, Chris Shoemaker wrote:
> > Wow!  You had me at double iteration!  :)
>
> :-))
>
> As clear an example as I can imagine of how sharing code BEFORE it is "final
> quality" is a wise choice for free software development! (Sorry it was your
> code that got used for the illustration, Chris!)  ;-)
>
> I should have been able to see your early code and you should be able to see
> cashutil. Let's not let other new developers make the same mistakes next
> time? All this work for a lack of documentation, easy to update webpages and
> a little consensus on code management conventions.
>
> We need quick and open ways to publish *early* code - pseudo code if you will.
> Designs, plans, ideas. Rough hacks, concepts, logic. Code that is certain to
> break the sacrosanct CoreBuild. Branches are part of the answer but, as Derek
> pointed out, technology isn't the answer to sociological problems.
I know nobody liked my ideas about branching, but maybe I'm just using
the term differently than others.  There's a interesting case-study
right here.  In my view, *every* time you're writing code, you're
implicitly branching whether you realize it or not.  For example, the
code was at point (A) when I last refreshed by budget patches (B) and
mailed them.  David continued to improve general GUI code (C).  Then
David merged (B) to (C), fixing the conflicts and also improving on my
code, producing (D).  Now, I've improved (B) from Neil's suggestion,
producing (E).

      /=B==E
     /   \
   -A     \
     \==C==D

The goal now is to merge E->D.  I think the SCM question isn't so much
about branching as it is about merging.  Any merging approach based on
versioned trees will involve repeating the work done to merge B->C in
order to merge E->D.  Of course, what we *want* to do is not really
merge E->D, but only merge the delta from B to E into D.  (BTW, that
delta is exactly what I'm attaching.)  IOW, we're merging changesets,
whether we do it manually or our tools do it for us.  (Of course, they
can only do that if they *have* B delta E and *know* that B was merged
into C.)

The requirements are the same for the manual operation.  In this case,
we're not so bad off, because we do have B -- it was sent to the list.
And the only reason I'm sending B delta E is because I do know that B
was merged into C.

Now, I've seen this pattern probably 20 times (not with gnucash) and
it can get pretty hairy.  The only time this process doesn't really
suck is when I know somebody's been merging my code, and exactly what
state they were merging from, (like now).

IMO, this is a PITA, because I *REALLY* DON'T WANT TO CARE who's been
doing whatever with whatever I've written.  I'm *sharing* my code for
a reason: I want other people to benefit from it.  That benefit is
severely diminished if it's a pain for other people to independently
improve my code and still retain my further improvements, too, or if I
have to keep tabs on everything that happens to every revision of my
code just to make it *not* a pain to incorporate further improvements.

To be frank, I'm convinced that for me to continue in that code
development pattern is irresponsible and wasteful.

> The core
> advance has to be that we leave our paranoia behind, kill the sacred cow of
> "TheCoreBuild" and accept that you can't make an omelette without breaking
> eggs. CVS is NOT guaranteed to always build, there's no rule that says so and
> we mislead ourselves and prospective new developers if we pursue such a goal
> *to the exclusion of all else*. Fine, have a HEAD branch that is the current
> release and maybe a main development branch but sometimes that build will
> break. It's a necessary step. It's a GOOD sign, it is to be managed not
> eradicated as if it was some strain of avian flu.

I actually don't have any problem with there being some branch
(whatever it's called) that has a very high standard for merging.
Now, I certainly don't want to be the one responsible for maintaining
it, but I'd sure have a healthy amount of respect for anyone who did.
OTOH, I'm really doing my best to make sure that that's not the de
facto standard for *sharing* code, which I think is just idiotic.

****

Just so I can keep all my rant in one mail ... This code sharing idea
is not some hypothetical.  I've got a fair chuck of register rewrite
code and Didier expressed an interest in collaborating.  We've seen
him put up and code so I'm inclined to take his offer seriously.  

Right now, my options are:

1) stream a bunch of patches to -patches that don't always compile,
are obviously incomplete and ugly, have dependencies higher than our
target release, probably break the build, and probably shouldn't be
applied by anyone not working on the register rewrite; and hope
Didier's tools are handy enough to autosuck patches from email (oooh
distributed development 1980's style)

or 2) take this into a back-alley and get some real collaboration
going, even though it'll be in relative isolation from the people who
have the most to offer in terms of assistance.

*sigh* Ok, I'm done.  :( Please include the word FLAME in the subject
of your response if I'll need my asbestos underwear.

-chris



_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches

undo-qof-get-list.diff (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Neil Williams-2
On Thursday 27 October 2005 4:36 am, Chris Shoemaker wrote:
> > We need quick and open ways to publish *early* code - pseudo code if you
> > will. Designs, plans, ideas. Rough hacks, concepts, logic. Code that is
> > certain to break the sacrosanct CoreBuild. Branches are part of the
> > answer but, as Derek pointed out, technology isn't the answer to
> > sociological problems.
>
> I know nobody liked my ideas about branching, but maybe I'm just using
> the term differently than others.

It's certainly how I understand the term. I use makepatch to handle my
changesets but my process is long winded:

1. Ensure all changes are in local CVS from /opt/working but do NOT update my
own pristine source in /opt/devel
2. cvs update in /opt/forge that contains the pristine gnucash source.
3. makepatch with /opt/devel as old and /opt/forge as new. This gives me a
single patch that covers all changes since my last commit to remote CVS.
4. View the patch and check for conflicts with files I've been changing
locally. This is a time consuming stage.
5. Copy files around if I haven't changed them myself and hack remote changes
into my modified files. Resolve conflicts and post to the list!!!
6. Now update /opt/devel with my local changes and the remote changes and my
hacks to combine the two.
7. Remake the makepatch to see what I might have missed.
8. Set the scripts running to build various trees (6 at present count) while I
go off and do something different.

> There's a interesting case-study
> right here.  In my view, *every* time you're writing code, you're
> implicitly branching whether you realize it or not.

Yes, a local, working copy that contains files that you are actively modifying
is just a local, uncommitted and unreviewed branch. It is precisely at this
early stage that others can be most useful in the peer review process and
where interventions are easiest to incorporate into new code.

> order to merge E->D.  Of course, what we *want* to do is not really
> merge E->D, but only merge the delta from B to E into D.

Yes, what I want is not the makepatch from forge to old local, I want a
makepatch that synchronises forge with working WITHOUT losing the changes in
the working tree. It's complicated and not easily automated with current
tools. Current patching tools simply force the old tree to become the new -
byte by byte, what I need is to merge the *changes* from forge into the
developing code in working/ without losing other changes.

I'm changing files in working/ all the time - the worst time is when I'm
preparing for a commit and someone else also makes a commit. Because of this
single build problem, my commit process has to start all over again. It's
mind-numbingly frustrating.

All I want is to be able to commit my changes, independent of anyone else's
changes, but the current system won't allow that.

CVS patches the remote files to be exact copies of my local files which is
wrong. I want to only send my changes and let them be independent of
unrelated changes in other parts of the tree or other files in the same part
of the tree.

> Now, I've seen this pattern probably 20 times (not with gnucash) and
> it can get pretty hairy.  The only time this process doesn't really
> suck is when I know somebody's been merging my code, and exactly what
> state they were merging from, (like now).

Which is why we NEED more publication of the DEVELOPMENT code, not the
finished "CoreBuild" code.

> IMO, this is a PITA, because I *REALLY* DON'T WANT TO CARE who's been
> doing whatever with whatever I've written.  I'm *sharing* my code for
> a reason: I want other people to benefit from it.  That benefit is
> severely diminished if it's a pain for other people to independently
> improve my code and still retain my further improvements, too, or if I
> have to keep tabs on everything that happens to every revision of my
> code just to make it *not* a pain to incorporate further improvements.

Wholeheartedly agree. It is a severe PITA that unrelated changes in completely
separate folders require a complete merge and rebuild to regenerate my
potential patch for the commit. The ONLY reason for that is the insistence on
not breaking the CoreBuild.

I don't know if my changes will break the build when placed on top of someone
else's changes, I only know that they do NOT break the build when placed over
the code BEFORE that person's changes. To avoid being flamed (again), I have
to delete the entire commit patch and start all over again. WHY? (fear).

> I actually don't have any problem with there being some branch
> (whatever it's called) that has a very high standard for merging.

Neither do I, as long as there is an easier way of creating and reviewing lots
of other branches / series of changesets.

> Now, I certainly don't want to be the one responsible for maintaining
> it, but I'd sure have a healthy amount of respect for anyone who did.

We would have to share that burden - when our own branches DO get to
finished / build-ready code, not before.

> 1) stream a bunch of patches to -patches that don't always compile,
> are obviously incomplete and ugly, have dependencies higher than our
> target release, probably break the build, and probably shouldn't be
> applied by anyone not working on the register rewrite; and hope
> Didier's tools are handy enough to autosuck patches from email (oooh
> distributed development 1980's style)

Not the easiest way for others to review the code - I found it quite hard to
compose that reply to your budget code about the QofClass because reading a
diff in an email client isn't natural.

> or 2) take this into a back-alley and get some real collaboration
> going, even though it'll be in relative isolation from the people who
> have the most to offer in terms of assistance.

Which is why cashutil was mooted as a potential SourceForge project and why it
still has a back-alley feel because the current CVS isn't publicly accessible
(the server wouldn't take the load).

> *sigh* Ok, I'm done.  :( Please include the word FLAME in the subject
> of your response if I'll need my asbestos underwear.

Not from me, I'm right in the dog-house with you, Chris!

--

Neil Williams
=============
http://www.data-freedom.org/
http://www.nosoftwarepatents.com/
http://www.linux.codehelp.co.uk/


_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches

attachment0 (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

David Hampton-2
On Fri, 2005-10-28 at 21:19 +0100, Neil Williams wrote:

> Yes, a local, working copy that contains files that you are actively modifying
> is just a local, uncommitted and unreviewed branch. It is precisely at this
> early stage that others can be most useful in the peer review process and
> where interventions are easiest to incorporate into new code.

So why not create a real explicit branch in the repository.  Commit to
your heart's content, and anyone else can check out your working (or
non-working) code.

> > order to merge E->D.  Of course, what we *want* to do is not really
> > merge E->D, but only merge the delta from B to E into D.
>
> Yes, what I want is not the makepatch from forge to old local, I want a
> makepatch that synchronises forge with working WITHOUT losing the changes in
> the working tree. It's complicated and not easily automated with current
> tools. Current patching tools simply force the old tree to become the new -
> byte by byte, what I need is to merge the *changes* from forge into the
> developing code in working/ without losing other changes.

That would be "cvs update -j rev1 -j rev2".  Its what I do every time I
synchronize the changes from HEAD in to the g2 branch.

> I'm changing files in working/ all the time - the worst time is when I'm
> preparing for a commit and someone else also makes a commit. Because of this
> single build problem, my commit process has to start all over again. It's
> mind-numbingly frustrating.
>
> All I want is to be able to commit my changes, independent of anyone else's
> changes, but the current system won't allow that.

I don't follow this statement at all.  If you've changed
src/engine/qofid.c and I've changed src/gnome/gnc-main-window.c there's
no conflict.  If your process make you restart in this case then the
process is the problem, not the scm system.

> I want to only send my changes and let them be independent of
> unrelated changes in other parts of the tree or other files in the same part
> of the tree.

CVS has no problem with this unless the changes conflict in an
individual file.

> Which is why we NEED more publication of the DEVELOPMENT code, not the
> finished "CoreBuild" code.

So get a branch and publish your code.  This is no different than
committing change sets to a repository.  Someone has to check out the
branch/apply the changeset if they want to see what you're working on.

> Wholeheartedly agree. It is a severe PITA that unrelated changes in completely
> separate folders require a complete merge and rebuild to regenerate my
> potential patch for the commit.

It doesn't.

> The ONLY reason for that is the insistence on not breaking the CoreBuild.

Doesn't matter to me what scm system is used.  I'm going to insist that
the main trunk compiles on a nightly basis.  You want to commit
non-working code to any branch other than the main trunk or a shipping
production branch (like the 1.8 branch)?  Go right ahead.  That's what
branches are for.

> > I actually don't have any problem with there being some branch
> > (whatever it's called) that has a very high standard for merging.
>
> Neither do I,

That's not the impression that I've gotten from the both of you
throughout the whole scm discussion.  Today this is the g2 branch.

> as long as there is an easier way of creating and reviewing lots
> of other branches / series of changesets.

I agree there needs to be an easier way to create branches.

> Which is why cashutil was mooted as a potential SourceForge project and why it
> still has a back-alley feel because the current CVS isn't publicly accessible
> (the server wouldn't take the load).

Is there some reason you're not doing this work in a branch of the
gnucash repository?

David


_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Derek Atkins
In reply to this post by Neil Williams-2
Neil Williams <[hidden email]> writes:

> I should have been able to see your early code and you should be able to see
> cashutil.

I've been saying all along that you should work on cashutil in
the GnuCash CVS tree...  It was /your/ choice to work on it
externally.  8-D

-derek
--
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       [hidden email]                        PGP key available
_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Josh Sled
In reply to this post by David Hampton-2
On Fri, 2005-10-28 at 17:04 -0400, David Hampton wrote:
> > Which is why we NEED more publication of the DEVELOPMENT code, not the
> > finished "CoreBuild" code.
>
> So get a branch and publish your code.  This is no different than
> committing change sets to a repository.  Someone has to check out the
> branch/apply the changeset if they want to see what you're working on.

+1.  This whole "CoreBuild" specter that has been synthesized is a waste
of time.  Create a branch, already.

...jsled
--
http://asynchronous.org/ - `a=jsled; b=asynchronous.org; echo ${a}@${b}`
_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Chris Shoemaker
On Fri, Oct 28, 2005 at 07:08:19PM -0400, Josh Sled wrote:

> On Fri, 2005-10-28 at 17:04 -0400, David Hampton wrote:
> > > Which is why we NEED more publication of the DEVELOPMENT code, not the
> > > finished "CoreBuild" code.
> >
> > So get a branch and publish your code.  This is no different than
> > committing change sets to a repository.  Someone has to check out the
> > branch/apply the changeset if they want to see what you're working on.
>
> +1.  This whole "CoreBuild" specter that has been synthesized is a waste
> of time.  Create a branch, already.

Guys, it's really not about the branching.  If it was, CVS wouldn't
suck; CVS supports branching just fine.  It's about the MERGING of
branches.  *That's* why CVS sucks.  It's the convergence/divergence
balance.  CVS works fine for diverging.  But for good convergence,
branch-merging has to be so easy it's trivial.  And it has to easily
support all types of merge operations - cherry-picking, single-commit
out-of-order reverts, rebasing of entire commit chains, etc.  It's all
the stuff we dread (and avoid) doing now because it's so tedious and
time-consuming.

But there's kind of a conflation of (albeit, related) issues here.
This SCM issue is not the foundational one -- it's only a consequence
of the foundational issue: our development process.  Our process just
doesn't support the case where:
   1. not everybody wants to work on the same thing,
   2. they can't produce almost perfect code the first time out, and
   3. they want to collaborate.

If only 2 of the 3 are true, maybe the current process works just
fine, but, for better or worse, all 3 are true.  And it's NOT working.
At least not well.

-chris

>
> ...jsled
> --
> http://asynchronous.org/ - `a=jsled; b=asynchronous.org; echo ${a}@${b}`
> _______________________________________________
> gnucash-patches mailing list
> [hidden email]
> https://lists.gnucash.org/mailman/listinfo/gnucash-patches
_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Chris Shoemaker
In reply to this post by David Hampton-2
On Fri, Oct 28, 2005 at 05:04:05PM -0400, David Hampton wrote:
> That would be "cvs update -j rev1 -j rev2".  Its what I do every time I
> synchronize the changes from HEAD in to the g2 branch.

And it's enough of a pain that we don't do it /every day/, which is
exactly how often I sync my development with mainline for other
projects.  And if you're tool has the full history for both branches,
merging every day is easy, even when the development branch becomes
quite heavy.  OTOH, refreshing a patch stack the size of budgetting
often became so burdonsome that if I got a few days behind, I dreaded
picking back up with gnucash development.  I don't want to go back
there.

>
> > I'm changing files in working/ all the time - the worst time is when I'm
> > preparing for a commit and someone else also makes a commit. Because of this
> > single build problem, my commit process has to start all over again. It's
> > mind-numbingly frustrating.
> >
> > All I want is to be able to commit my changes, independent of anyone else's
> > changes, but the current system won't allow that.
>
> I don't follow this statement at all.  If you've changed
> src/engine/qofid.c and I've changed src/gnome/gnc-main-window.c there's
> no conflict.  If your process make you restart in this case then the
> process is the problem, not the scm system.

No way, man.  Changes in untouched files broke my patches all the
time.  Semantically, not syntactically.  Breakage wouldn't show itself
until you tried to use the feature.

>
> > I want to only send my changes and let them be independent of
> > unrelated changes in other parts of the tree or other files in the same part
> > of the tree.
>
> CVS has no problem with this unless the changes conflict in an
> individual file.

Um, yeah, but for any substantial development work, conflicts are
guaranteed.

>
> > Which is why we NEED more publication of the DEVELOPMENT code, not the
> > finished "CoreBuild" code.
>
> So get a branch and publish your code.  This is no different than
> committing change sets to a repository.  Someone has to check out the
> branch/apply the changeset if they want to see what you're working on.

Practically, the difference is the relative frequency of tree-based
branch merges vs. the frequency of changeset merges.  *That* difference
is the consequence of the difficulty difference.

>
> > Wholeheartedly agree. It is a severe PITA that unrelated changes in completely
> > separate folders require a complete merge and rebuild to regenerate my
> > potential patch for the commit.
>
> It doesn't.

Well, you don't *know* that it doesn't unless you either 1) review
every line of changed code in *both* branches, or 2) merge, rebuild,
test.

>
> > The ONLY reason for that is the insistence on not breaking the CoreBuild.
>
> Doesn't matter to me what scm system is used.  I'm going to insist that
> the main trunk compiles on a nightly basis.  You want to commit
> non-working code to any branch other than the main trunk or a shipping
> production branch (like the 1.8 branch)?  Go right ahead.  That's what
> branches are for.

Two points:

1) I think it's good to have a trunk that compiles every
day.  There are *tons* of good reasons for this.

2) Maybe it's too early to get retrospective about this, but... we
have to be smart about branching.  IMO, this didn't work so well with
G2.  There was no reason why there couldn't have been regular releases
of GnuCash that had incrementally more and more use of gtk2, while
still using gtk1.  We know how to solve the multiple library issues.
But it didn't happen that way.  The branches diverged just fine, so
there wasn't development conflict, but they didn't converge well
enough to keep releasing new code.  Let's not do that again.  We need
convergence tools.

>
> > > I actually don't have any problem with there being some branch
> > > (whatever it's called) that has a very high standard for merging.
> >
> > Neither do I,
>
> That's not the impression that I've gotten from the both of you
> throughout the whole scm discussion.  Today this is the g2 branch.

Then you've gotten the wrong impression.  As a matter of fact, if I
had everything /my/ way (a scary thought, I know, you can open your
eyes now) the standard would be far *higher* that it is now.  I would
name you the release manager for g2 and *only* you would decide what
goes into *your* tree (but it's all visible of course).  And it's
ready when you say it's ready and release it.  Then, there have to be
mechanisms in place to cultivate the "cloud" of code changes to swirl
around us, including you, and mechanisms to make it easy to for
everyone, including you, to grab arbitrary selections of those
changes.  You get the idea.  Anyway, I hope that clears that up.

>
> > as long as there is an easier way of creating and reviewing lots
> > of other branches / series of changesets.
>
> I agree there needs to be an easier way to create branches.

!?!  Oh.  I should've read the whole mail before writing.  It looks
like we have some agreement here.  I'll just mention that it's not so
much the *creation* of branches which needs to be easy.  (It's trivial
now.)  It's the branch *management* that needs to be easier, and that
means the constellation of merge operations.

-chris
_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Derek Atkins
In reply to this post by Chris Shoemaker
Quoting Chris Shoemaker <[hidden email]>:

> On Fri, Oct 28, 2005 at 07:08:19PM -0400, Josh Sled wrote:
>> On Fri, 2005-10-28 at 17:04 -0400, David Hampton wrote:
>> > > Which is why we NEED more publication of the DEVELOPMENT code, not the
>> > > finished "CoreBuild" code.
>> >
>> > So get a branch and publish your code.  This is no different than
>> > committing change sets to a repository.  Someone has to check out the
>> > branch/apply the changeset if they want to see what you're working on.
>>
>> +1.  This whole "CoreBuild" specter that has been synthesized is a waste
>> of time.  Create a branch, already.
>
> Guys, it's really not about the branching.  If it was, CVS wouldn't
> suck; CVS supports branching just fine.  It's about the MERGING of
> branches.  *That's* why CVS sucks.

CVS merges branches just fine.  As David said:

  cvs update -j tag1 -j tag2

It'll merge great.  Like /all/ SCMs you need to manually resolve
conflicts.  But the merge works just fine.

 From where I sit, your major complaint is that you don't have CVS
commit access.  Everything else you're saying follows from that one
point.  If you had CVS commit access then EVERYTHING you've been
complaining about would be moot, because, well, even CVS does it all.

The /only/ thing CVS doesn't do well is file moves/renames.

-derek

--
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       [hidden email]                        PGP key available

_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Chris Shoemaker
On Fri, Oct 28, 2005 at 10:39:32PM -0400, Derek Atkins wrote:

> Quoting Chris Shoemaker <[hidden email]>:
>
> >On Fri, Oct 28, 2005 at 07:08:19PM -0400, Josh Sled wrote:
> >>On Fri, 2005-10-28 at 17:04 -0400, David Hampton wrote:
> >>> > Which is why we NEED more publication of the DEVELOPMENT code, not the
> >>> > finished "CoreBuild" code.
> >>>
> >>> So get a branch and publish your code.  This is no different than
> >>> committing change sets to a repository.  Someone has to check out the
> >>> branch/apply the changeset if they want to see what you're working on.
> >>
> >>+1.  This whole "CoreBuild" specter that has been synthesized is a waste
> >>of time.  Create a branch, already.
> >
> >Guys, it's really not about the branching.  If it was, CVS wouldn't
> >suck; CVS supports branching just fine.  It's about the MERGING of
> >branches.  *That's* why CVS sucks.
>
> CVS merges branches just fine.  As David said:
>
>  cvs update -j tag1 -j tag2
>
> It'll merge great.  Like /all/ SCMs you need to manually resolve
> conflicts.  But the merge works just fine.

No Derek.  It doesn't.  If you branch and then make say 250 commits to
your branch (that's about how many I would have right now with a
cs-based scm) and there were 250 commits on the other branch (that's
about how many there've been on G2 since I started the
register-rewrite) and then try to merge them based on the final two
resulting trees, with CVS, or any versioned file-tree system, you very
well might have a HUGE MESS.  OTOH, if have all 500 commits and 250^2
degrees of freedom in merge order, you're much more likely to be able
to merge with NO conflicts.  And any conflicts you do have will be the
conflicts of *individual* commits.

> From where I sit, your major complaint is that you don't have CVS
> commit access.  Everything else you're saying follows from that one
> point.  If you had CVS commit access then EVERYTHING you've been
> complaining about would be moot, because, well, even CVS does it all.

Maybe you missed the email where I said in CAPITAL LETTERS that giving
me commit access WON'T SOLVE THE PROBLEM.  Or maybe you missed my
idealization of having no one but David commit to g2.  Or maybe you
just don't believe me.  Or maybe you just wish that there was an easy
fix.  I know I do.

C'mon, have I ever given any indication of not speaking my mind?  I
don't think so.  I did say that commit access would make my
development easier, but if I thought that the real problem was my
commit access, I'd LET YOU KNOW, trust me.  I don't have time for
games; there're too many beautiful ideas that need coding.

Incidentally, I really think you and I have philosophical differences
here.  I don't WANT the authority to dictate what code is in the
software you use.  I want ONLY YOU to have that authority.  OTOH, I
*do* want the ability to make MY code available to whoever wants it
(and will agree to the license), and to work with others efficiently.
Those are my goals.

You have absolutely no obligation to help me achieve those goals, and
I have no right to demand that you do.  I hope you don't think I would
do that.  I really don't think there's any point of conflict here, so
please don't find one.  Besides, there's plenty of real areas of
disagreement.  :)

> The /only/ thing CVS doesn't do well is file moves/renames.

Interesting...  I can honestly say you're the first person I've ever
heard make that precise assertion.  I think you may have the minority
report there, Derek.  :)

-chris
_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Josh Sled
On Fri, 2005-10-28 at 21:00 -0400, Chris Shoemaker wrote:
> of the foundational issue: our development process.  Our process just
> doesn't support the case where:
>    1. not everybody wants to work on the same thing,
>    2. they can't produce almost perfect code the first time out, and
>    3. they want to collaborate.

Branches can support all three of these goals.

Though, I continue to not understand why it's not reasonable for
everyone to be working primarily towards the same goal/thing.


On Fri, 2005-10-28 at 22:21 -0400, Chris Shoemaker wrote:

> Then you've gotten the wrong impression.  As a matter of fact, if I
> had everything /my/ way (a scary thought, I know, you can open your
> eyes now) the standard would be far *higher* that it is now.  I would
> name you the release manager for g2 and *only* you would decide what
> goes into *your* tree (but it's all visible of course).  And it's
> ready when you say it's ready and release it.  Then, there have to be
> mechanisms in place to cultivate the "cloud" of code changes to swirl
> around us, including you, and mechanisms to make it easy to for
> everyone, including you, to grab arbitrary selections of those
> changes.  You get the idea.  Anyway, I hope that clears that up.

I don't think we need, and personally I don't want, a system that
requires any individual to take arbitrary selections of the "cloud" of
changesets.  I'm quite fine developing software from a single
changeset-based repository.


On Sat, 2005-10-29 at 00:04 -0400, Chris Shoemaker wrote:
> well might have a HUGE MESS.  OTOH, if have all 500 commits and 250^2
> degrees of freedom in merge order, you're much more likely to be able
> to merge with NO conflicts.  And any conflicts you do have will be the
> conflicts of *individual* commits.

250^2 degress of freedom in merge order?  Ignoring the fact that that's
not practically true or useful given the semantic dependencies of the
commits, do we really practically need this capability?  I'm finding
your argument tending toward the absurd ... with respect to GnuCash
especially.

> Incidentally, I really think you and I have philosophical differences
> here.  I don't WANT the authority to dictate what code is in the
> software you use.  I want ONLY YOU to have that authority.  OTOH, I
> *do* want the ability to make MY code available to whoever wants it
> (and will agree to the license), and to work with others efficiently.
> Those are my goals.

Well, you're free to take as much time and effort as you like to
assemble for yourself what code is in the software you use.  I think I
can speak for most of the developers (and users) when I say that the
rest of us are trying to produce 1 piece of software... 1 codebase.
It's not a question of coersion or curtailing individual liberties...
it's just practical development simplicity.  Doing this might take
extended branches from time to time, but I don't think we need or want
this flexability you desire, and its associated cost.


Chris, I really don't care to spend any more time on these threads.  You
assert that the development process is broken, and most of the rest of
us assert the opposite.  You really want some unspecified distributed
SCM/"convergence tools", and most of the rest of us are fine just moving
to SVN.   You claim the problem's in the tools, then you claim it's not
just about the tools... around and around we go.

I don't quite know how to resolve this.  I guess we could make proposals
and vote Apache-style, but I already see a concensus.  What do you want?

...jsled
--
http://asynchronous.org/ - `a=jsled; b=asynchronous.org; echo ${a}@${b}`
_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Josh Sled
On Sat, 2005-10-29 at 16:28 -0400, Josh Sled wrote:
> changesets.  I'm quite fine developing software from a single
> changeset-based repository.

Excuse me, I meant (of course): "from a single versioned-tree-based
repository".

...jsled
--
http://asynchronous.org/ - `a=jsled; b=asynchronous.org; echo ${a}@${b}`
_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches
Reply | Threaded
Open this post in threaded view
|

Re: Forgotten patch: qofid.diff

Chris Shoemaker
In reply to this post by Josh Sled
On Sat, Oct 29, 2005 at 04:28:47PM -0400, Josh Sled wrote:

> On Fri, 2005-10-28 at 21:00 -0400, Chris Shoemaker wrote:
> > of the foundational issue: our development process.  Our process just
> > doesn't support the case where:
> >    1. not everybody wants to work on the same thing,
> >    2. they can't produce almost perfect code the first time out, and
> >    3. they want to collaborate.
>
> Branches can support all three of these goals.
>
> Though, I continue to not understand why it's not reasonable for
> everyone to be working primarily towards the same goal/thing.

I guess we have the same goal in a broader sense.  I just meant the
narrow sense of wanting to do two different things to the same code,
e.g. bug-fix current register, and completely overhaul register.

> On Sat, 2005-10-29 at 00:04 -0400, Chris Shoemaker wrote:
> > well might have a HUGE MESS.  OTOH, if have all 500 commits and 250^2
> > degrees of freedom in merge order, you're much more likely to be able
> > to merge with NO conflicts.  And any conflicts you do have will be the
> > conflicts of *individual* commits.
>
> 250^2 degress of freedom in merge order?  Ignoring the fact that that's
> not practically true or useful given the semantic dependencies of the
> commits, do we really practically need this capability?  

That depends on how you want to use it, of course.  If everybody tends
to work in the same branch, then no, it's probably not a compelling
feature.  OTOH, if you prefer to branch for any non-trivial code
endeavor, then yes, it's useful.

> Chris, I really don't care to spend any more time on these threads.  

Me neither.  I recognize there is a diminishing return value here.

> You
> assert that the development process is broken, and most of the rest of
> us assert the opposite.  You really want some unspecified distributed
> SCM/"convergence tools", and most of the rest of us are fine just moving
> to SVN.  

Re: "unspecified".  I've been light on specification not because there
are not concrete realizations of what I'm talking about but because I
wanted to separate issue of requirements and implementation.

> You claim the problem's in the tools, then you claim it's not
> just about the tools... around and around we go.

I don't think I've been unclear here.  SCM tools are both a _product_
of their development culture and _influence_ their development
culture.  Problems in both are manifest in the other.

> I don't quite know how to resolve this.  I guess we could make proposals
> and vote Apache-style, but I already see a concensus.  What do you want?

I don't think you'd find a consensus unless you limited the vote to
certain people.  I agree with Christian's recommendations: collapse G2
_soon_, move forward with SVN, re-eval next year.

-chris
_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches