AUDIT: r15205 - gnucash/trunk - Load and store a commodity's KVP-frame (IFF it's non-empty).

[Chris Shoemaker]

On Mon, Dec 11, 2006 at 11:01:55PM -0500, Derek Atkins wrote:
> I'll just point out a couple of things:
> 
> 1) This patch doesn't actually change any data formats, because there's
>   no data in those fields.  So without additional changes (which I agree
>   should not get backported), there's no chance of a data format change.

That requires us to be careful not to make any changes that use
commodity KVPs.

> 2) Your proposed change makes it WORSE, because it no longer preserves
>   data!  It would happly read in the KVP data but then NOT SAVE IT BACK
>   OUT!   I think that would be even worse than tossing chunks.
> 
> Keep in mind that just making the XML parser not blow chunks on
> unrecognized tags isn't sufficient -- we also need to re-write out
> those unrecognized tags or we destroy data.   Reading and silently
> discarding is worse than erroring out.

That's an ideal, but I'm pragmatic.  Preserving unrecognized data is
best; Preventing me from opening the file at all is worst.  Discarding
unrecognized data is somewhere inbetween.  (And it doesn't have to be
silent.)

> So, I argue that this change still is safe because it's effectively
> unused code, but it would let us add additional information in a 2.2
> release and not destroy that data in 2.0, which is why I think we should
> backport this change.

Just to be clear, in my opinion, this "feature" (which we have yet to
actually see) does not by itself justify breaking backward
compatibility, even 2.0 -> 2.2.

When we _do_ implement new features, I want to be intentional about
breaking compatibility, not just accept that, well if you're trying to
use your new file with 2.0.4 you're fine, but if you've got 2.0.3,
sorry you can't even open it.  That's lousy.

-chris

> -derek
> 
> Quoting Chris Shoemaker <c.shoemaker at cox.net>:
> 
> >
> >I don't think this should be back-ported.  At least not the whole
> >thing.  I don't mind backporting the first and last hunks, but I'm
> >against breaking backward compatibility of the datafile within a
> >stable branch.
> >
> >In fact, even between major releases I think these types of changes
> >should only be made if there's enough of them to warrant it.
> >
> >Also, I'd prefer to see the proposed feature implemented _before_
> >breaking compatibilty, not the other way around.
> >
> >Now, if someone wanted to make it so that our xml reader didn't croak
> >on unrecognized tags, that would make this kind of change easier.
> >
> >So, can we please comment out hunks 2 and 3 with a comment about
> >re-enabling them when we're willing to break backward compatibility?
> >
> >-chris
> >
> >>Property changes on: gnucash/trunk
> >>___________________________________________________________________
> >>Name: svk:merge
> >>   - 
> >>3889ce50-311e-0410-a464-f059747ec5d1:/local/gnucash/branches/swig-redo:802
> >>3889ce50-311e-0410-a464-f059747ec5d1:/local/gnucash/trunk:943
> >>d2ab10a8-8a95-4986-baff-8d511d9f15b2:/local/gnucash/trunk:13679
> >>d2ab10a8-8a95-4986-baff-8d511d9f15b2:/local/gnucash/trunk2:13366
> >>   + 
> >>3889ce50-311e-0410-a464-f059747ec5d1:/local/gnucash/branches/swig-redo:802
> >>3889ce50-311e-0410-a464-f059747ec5d1:/local/gnucash/trunk:943
> >>d2ab10a8-8a95-4986-baff-8d511d9f15b2:/local/gnucash/trunk:13699
> >>d2ab10a8-8a95-4986-baff-8d511d9f15b2:/local/gnucash/trunk2:13366
> >>
> >>Modified: gnucash/trunk/src/backend/file/gnc-commodity-xml-v2.c
> >>===================================================================
> >>--- gnucash/trunk/src/backend/file/gnc-commodity-xml-v2.c	2006-12-11 
> >>22:28:36 UTC (rev 15204)
> >>+++ gnucash/trunk/src/backend/file/gnc-commodity-xml-v2.c	2006-12-12 
> >>02:51:37 UTC (rev 15205)
> >>@@ -58,6 +58,7 @@
> >> #define cmdty_get_quotes     "cmdty:get_quotes"
> >> #define cmdty_quote_source   "cmdty:quote_source"
> >> #define cmdty_quote_tz       "cmdty:quote_tz"
> >>+#define cmdty_slots          "cmdty:slots"
> >>
> >> xmlNodePtr
> >> gnc_commodity_dom_tree_create(const gnc_commodity *com)
> >>@@ -66,8 +67,11 @@
> >>     const char *string;
> >>     xmlNodePtr ret;
> >>     gboolean currency = gnc_commodity_is_iso(com);
> >>+    xmlNodePtr kvpnode =
> >>+      kvp_frame_to_dom_tree(cmdty_slots,
> >>+			    qof_instance_get_slots(QOF_INSTANCE(com)));
> >>
> >>-    if (currency && !gnc_commodity_get_quote_flag(com))
> >>+    if (currency && !gnc_commodity_get_quote_flag(com) && !kvpnode)
> >>       return NULL;
> >>
> >>     ret = xmlNewNode(NULL, BAD_CAST gnc_commodity_string);
> >>@@ -108,6 +112,10 @@
> >>       if (string)
> >> 	xmlAddChild(ret, text_to_dom_tree(cmdty_quote_tz, string));
> >>     }
> >>+
> >>+    if (kvpnode)
> >>+      xmlAddChild(ret, kvpnode);
> >>+
> >>     return ret;
> >> }
> >>
> >>@@ -159,6 +167,12 @@
> >> 	gnc_commodity_set_quote_source(com, source);
> >>         xmlFree (string);
> >>     }
> >>+    else if(safe_strcmp((char*)node->name, cmdty_slots) == 0)
> >>+    {
> >>+      /* We ignore the results here */
> >>+      dom_tree_to_kvp_frame_given(node,
> >>+				  qof_instance_get_slots(QOF_INSTANCE(com)));
> >>+    }
> >>     else
> >>     {
> >>         struct com_char_handler *mark;
> >>
> >>_______________________________________________
> >>gnucash-changes mailing list
> >>gnucash-changes at gnucash.org
> >>https://lists.gnucash.org/mailman/listinfo/gnucash-changes
> >_______________________________________________
> >gnucash-devel mailing list
> >gnucash-devel at gnucash.org
> >https://lists.gnucash.org/mailman/listinfo/gnucash-devel
> >
> 
> 
> 
> -- 
>       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
>       warlord at MIT.EDU                        PGP key available
>