Re: [Tracker] Merging Turtle branch to Trunk

[Philip Van Hoof <spam pvanhoof be>]

On Tue, 2008-12-16 at 12:54 +0000, Martyn Russell wrote:

I will attach a new patch as soon as Ivan fixed issues #13 and #15

> #1, Didn't we decide to depend on Raptor? If so, we don't need the 
> AC_DEFINE right?
>
> +# Check for Raptor
> +PKG_CHECK_MODULES(RAPTOR, [raptor], have_raptor=yes, have_raptor=no)
> +AC_SUBST(RAPTOR_CFLAGS)
> +AC_SUBST(RAPTOR_LIBS)
> +
> +if test x$have_raptor == "xyes"; then
> +  AC_DEFINE(HAVE_RAPTOR, 1, [Raptor RDF parsers])
> +fi

It's not decided yet if raptor should become a mandatory dependency in
TRUNK just yet.


> #2. This definitely looks like a leak to me, since the "break" will 
> leave the while() loop and the volume is freed ONLY in the opposite 
> condition. Or am I missing something here?
>
> +     while (g_hash_table_iter_next (&iter, &key, &value)) {
> +             LibHalVolume  *volume;
> +             const gchar   *udi;
> +             const gchar   *mp;
> +
> +             udi = key;
> +
> +             volume = libhal_volume_from_udi (priv->context, udi);
> +
> +             if (!volume) {
> +                     g_message ("HAL device with udi:'%s' has no volume, "
> +                                "should we delete?",
> +                                udi);
> +                     continue;
> +             }
> +
> +             mp = libhal_volume_get_mount_point (volume);
> +
> +             if (g_strcmp0 (mp, path) != 0) {
> +                     if (g_strrstr (path, mp)) {
> +                             found = TRUE;
> +
> +                             if (mount_point)
> +                                     *mount_point = g_strdup (mp);
> +
> +                             if (available)
> +                                     *available = libhal_volume_is_mounted (volume);
> +                             break;
> +                     }
> +             }
> +
> +             libhal_volume_free (volume);

Ah yes I see it now. Fixed.

> #3. We are checking the same thing twice here:
>
>       field = tracker_ontology_get_field_by_name (field_name);
>
> +     if (!field) {
> +             g_warning ("Field name '%s' has isn't described in the ontology", 
> field_name);
> +             return;
> +     }
> +
> +     field = tracker_ontology_get_field_by_name (field_name);
> +
>       g_return_if_fail (TRACKER_IS_FIELD (field));
>       g_return_if_fail (tracker_field_get_multiple_values (field) == TRUE);


Fixed


> #4. I still don't like the Config and Language being static here. We 
> already initialise the data module with them in 
> tracker_data_manager_init (), why not just call 
> tracker_data_manager_get_config() and 
> tracker_data_manager_get_language() respectively?
>
> +typedef struct {
> +     TrackerService *service;
> +     guint32 iid_value;
> +     TrackerLanguage *language;
> +     TrackerConfig *config;
> +} ForeachInMetadataInfo;
> +
> +static TrackerConfig   *config = NULL;
> +static TrackerLanguage *language = NULL;
> +

Fixed (first time I hear about those methods, but they are indeed
exactly what I needed for this)


> #5. Shouldn't we be adding the Service type == Folder check here before 
> doing anything recursive?
>
> +     if (service_id != 0) {
> +             tracker_data_update_delete_service (service, service_id);
> +             tracker_data_update_delete_service_recursively (service, (gchar *) path);
> +             tracker_data_update_delete_all_metadata (service, service_id);
> +     }

Fixed


> #6. Extra spaces here, there are further down in the patch for this file 
> too:
>
> +             GList *list;
> +
> +             for (list = value; list; list = list->next)
> +                     set_metadata (field, list->data, user_data);
> +
> +     }

Fixed

> #7. Can we remove these preprocessor operatives? and/or the debug 
> statement itself? If not, we can always improve the debugging to 
> actually mean something in the context of all the other logging.
>
> +#ifdef TURTLE_DEBUG
> +             g_debug ("Q ERROR: %s\n", error->message);
> +#endif /* TURTLE_DEBUG */

Fixed

> #8. Since we decided to depend on Raptor, we can remove the #ifdefs in 
> the tracker-turtle.[ch] I think. There are also a lot of code block 
> blank lines which are not necessary.

This is undecided

> #9. Include order should be system, highest dependency -> lowest 
> dependency. i.e. these should be the opposite way round in the 
> tracker-turtle.h:
>
> +#include <libtracker-data/tracker-data-metadata.h>
> +#include <stdio.h>
>
>
> #10. Coding style, indentation, and again, stmt isn't a good name for a 
> public typedef struct, can we call it something sensible like 
> TrackerTurtleStatement? We shouldn't need the #idefs there too if we 
> depend on Raptor.
>
> +typedef struct {
> +  gconstpointer subject;
> +  gint subject_type;
> +  gconstpointer predicate;
> +  gint predicate_type;
> +  gconstpointer object;
> +  gint object_type;
> +  gpointer object_literal_datatype;
> +  const guchar *object_literal_language;
> +} stmt;
> +#endif

Renamed to TrackerRaptorStatement

> #11. Can we avoid comments in line like this, it looks ugly, it is 
> better to have it above the function or not at all.
>
> +void        tracker_turtle_add_metadatas   (TurtleFile          *turtle,
> +                                         GPtrArray /* <TrackerTurtleMetadataItem> */ *metadata_items);

Fixed


> #12. There are some serious coding style issues in 
> tracker-removable-device.c. We could probably also use g_strcmp0() in a 
> lot of places to reduce the code down a bit and make it easier to read. 
> That's really not completely necessary though for this patch to get 
> committed.

All strcmp()s are now g_strcmp0()s.


> #13. Is this a leak, looks like only str is freed, not service_type in 
> the if (!field):
>
> +             gchar *str = NULL;
> +             gchar *uri;
> +             gchar *service_type;
> +
> +             g_value_init (&transform, G_TYPE_STRING);
> +
> +             tracker_db_result_set_get (result_set, 0, &uri, -1);
> +             tracker_db_result_set_get (result_set, 1, &service_type, -1);
> +             tracker_db_result_set_get (result_set, 2, &metadata_id, -1);
> +             tracker_db_result_set_get (result_set, 3, &str, -1);
> +
> +             field = tracker_ontology_get_field_by_id (metadata_id);
> +             if (!field) {
> +                     g_critical ("Field id %d in database but not in tracker-ontology",
> +                                 metadata_id);
> +                     g_free (str);
> +                     return;
> +             }

Ivan will fix this

> #14. There is no need to cast here in trackerd/tracker-main.c for 
> callback_id:
>
>
> +     gulong *callback_id = (gulong *)user_data;
> +     GError *error;
> +     static gint counter = 0;

Fixed


> #15. The || condition should be on the right here, also, this whole code 
> block would be nicer in another function I think - which I commented on 
> in my previous mail.
>
> +     if (flags & TRACKER_DB_MANAGER_FORCE_REINDEX
> +         || g_file_test (get_ttl_backup_filename, G_FILE_TEST_EXISTS)) {

Ivan will fix this


> Other than that, good work guys. I think we should try to get this 
> commit this week!


-- 
Philip Van Hoof, freelance software developer
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org 
http://pvanhoof.be/blog
http://codeminded.be