Merge lp:~manishsinha/zeitgeist-datasources/bzr-improvements-subjects into lp:zeitgeist-datasources/0.8

[Manish Sinha (मनीष सिन्हा)]

Fixed the subjects

* Actor is bzr.desktop just so that we are able to fetch the events
* Apart from folder subject, added subject for each file involved in the event
* Used Gio.File to get the mimetype of the file subject
* Subject Interpretation is not being set as it is decided by the mimetype
* File subjects are set only for commit messages as for push and pull, remote URI are
    used from which mimetype cannot be found out by Gio.File
* In case of Commit the text is "Revision: 23, Message: commit message"
* In case of Push, commit message is like "Pushed branch from 12 to 14"
* In case of Pull, commit message is like "Updated branch from 12 to 14"

[Jelmer Vernooij (jelmer)]

Thanks for hacking on this.

Some comments:

Using "is_push" with three different values is quite confusing to wrap your head around, especially as it can have the values False, True and None. Wouldn't it be better to just have a "change_kind" argument that can be set to "commit", "push" or "pull" ?

The other subjects shouldn't be set if there is no working tree (in that case the files don't exist on disk).

I think the other subjects should only be set in the case of push and pull, and not in the case of commit as commit doesn't actually change those files.

It would make sense to add a post_merge hook too. Merge/revert affects the files in the tree too, although it doesn't have any affect on the branch (unlike push/pull and commit). Perhaps that can wait for another branch though.

[Michal Hruby (mhr3)]

You're using USER_ACTIVITY in all cases, but if I just pull, all the changed files definitely aren't changed by the user, so that's clearly wrong.

+1 on the is_push misusage.

All in all I'm not really sure what to think of this dataprovider, more data is good (I guess), but whenever you really work with any of the files from the repo, you're using an editor. I can't shake off the feeling we're trying to somehow duplicate what the VCS system does itself - keep a commit log.

[Jelmer Vernooij (jelmer)]

> You're using USER_ACTIVITY in all cases, but if I just pull, all the changed files definitely aren't changed by the user, so
> that's clearly wrong.
The action is triggered by the user though, it's not an external action. If the user downloads a file from elsewhere onto the local system, then isn't that still USER_ACTIVTITY, even if the user didn't write the (entire) file?

> All in all I'm not really sure what to think of this dataprovider, more data is good (I guess), but whenever you really work
> with any of the files from the repo, you're using an editor. I can't shake off the feeling we're trying to somehow duplicate
> what the VCS system does itself - keep a commit log.
That's true - though it's more like a tiplog. In other words, this records the changes to the tip of this particular branch, rather than the actual history. If the current branch updates from revision 10 to revision 30, then that's a single change here, not 20.

[Seif Lotfy (seif)]

u can set the interpretation by doing

from zeitgeist.mimetypes import get_interpretation_for_mimetype

then execute the method giving it a mimetype and it will return interpretation accordingly

[Manish Sinha (मनीष सिन्हा) (manishsinha)]

> You're using USER_ACTIVITY in all cases, but if I just pull, all the changed files definitely aren't changed by the user, so
> that's clearly wrong.

No, pull is using WORLD_ACTIVITY. Check line 319

> Wouldn't it be better to just have a "change_kind" argument that can be set to "commit", "push" or "pull"

Will make that change.

> I can't shake off the feeling we're trying to somehow duplicate what the VCS system does itself - keep a commit log.

We are not storing all the information. Just the basic info as jelmer said. We are not worried about 20 commits being pulled. It is just a single event that 20 commits are being pulled.

> u can set the interpretation by doing

I saw that if I leave that field blank then the engine sets it itself.

[Manish Sinha (मनीष सिन्हा) (manishsinha)]

> The other subjects shouldn't be set if there is no working tree (in that case the files don't exist on disk).

Can you give some examples?

> I think the other subjects should only be set in the case of push and pull, and not in the case of commit as commit doesn't actually change those files.

The think the reverse. In a commit there are various files involved. A file can be added/deleted/modified which form a part of an commit (which is an event)

In case of push and pull, we get the remote URI like bzr+shh:// which is not a valid URI for Gio.File due to which mimetype cannot be fetched.

> It would make sense to add a post_merge hook too

It would be good, but right now my plan was to get these three events and try to write a small client trying to understand how the information can be used. I have already trimmed XChat dataprovider as it was logging too much and was spamming the database with useless events. Before going ahead and adding merge/revert hooks, it would nice to know how we can use those events.

[Jelmer Vernooij (jelmer)]

Am 24/03/12 17:54, schrieb Manish Sinha (मनीष सिन्हा):
>> The other subjects shouldn't be set if there is no working tree (in that case the files don't exist on disk).
> Can you give some examples?
You can have a branch without a working tree. E.g.:

$ bzr branch --no-tree -r5 lp:bzr-stats

will create a branch without a working tree. Running "bzr pull" there
later will pull in revisions but won't actually touch any of the
subjects the plugin now adds (as they don't exist).

>
>> I think the other subjects should only be set in the case of push and pull, and not in the case of commit as commit doesn't actually change those files.
> The think the reverse. In a commit there are various files involved. A file can be added/deleted/modified which form a part of an commit (which is an event)
The file itself doesn't change in that case though, only the branch
does. In what way is it the subject?
> In case of push and pull, we get the remote URI like bzr+shh:// which is not a valid URI for Gio.File due to which mimetype cannot be fetched.
That's not a problem - the working tree of remote branches isn't updated
by push and pull, only that of local branches.

>> It would make sense to add a post_merge hook too
> It would be good, but right now my plan was to get these three events and try to write a small client trying to understand how the information can be used. I have already trimmed XChat dataprovider as it was logging too much and was spamming the database with useless events. Before going ahead and adding merge/revert hooks, it would nice to know how we can use those events.
I think the same applies to adding too much subjects though... ?

Cheers,

Jelmer

[Seif Lotfy (seif)]

Every event needs a subject else it does not go into the ZG DB
In case of a commit I think we either log the fact that we committed and also add the modified and created files. Else no need to actually log the commit.

[Seif Lotfy (seif)]

having to many subjects is not an issue because mostly in your case we will
be using the files names as the text entry. those names will be reused
again and again.

2012/3/24 Jelmer Vernooij <email address hidden>

> Am 24/03/12 17:54, schrieb Manish Sinha (मनीष सिन्हा):
> >> The other subjects shouldn't be set if there is no working tree (in
> that case the files don't exist on disk).
> > Can you give some examples?
> You can have a branch without a working tree. E.g.:
>
> $ bzr branch --no-tree -r5 lp:bzr-stats
>
> will create a branch without a working tree. Running "bzr pull" there
> later will pull in revisions but won't actually touch any of the
> subjects the plugin now adds (as they don't exist).
>
> >
> >> I think the other subjects should only be set in the case of push and
> pull, and not in the case of commit as commit doesn't actually change those
> files.
> > The think the reverse. In a commit there are various files involved. A
> file can be added/deleted/modified which form a part of an commit (which is
> an event)
> The file itself doesn't change in that case though, only the branch
> does. In what way is it the subject?
> > In case of push and pull, we get the remote URI like bzr+shh:// which is
> not a valid URI for Gio.File due to which mimetype cannot be fetched.
> That's not a problem - the working tree of remote branches isn't updated
> by push and pull, only that of local branches.
>
> >> It would make sense to add a post_merge hook too
> > It would be good, but right now my plan was to get these three events
> and try to write a small client trying to understand how the information
> can be used. I have already trimmed XChat dataprovider as it was logging
> too much and was spamming the database with useless events. Before going
> ahead and adding merge/revert hooks, it would nice to know how we can use
> those events.
> I think the same applies to adding too much subjects though... ?
>
> Cheers,
>
> Jelmer
>
>
>
> --
>
> https://code.launchpad.net/~manishsinha/zeitgeist-datasources/bzr-improvements-subjects/+merge/99168
> You are requested to review the proposed merge of
> lp:~manishsinha/zeitgeist-datasources/bzr-improvements-subjects into
> lp:zeitgeist-datasources.
>

[Michal Hruby (mhr3)]

> The action is triggered by the user though, it's not an external action. If
> the user downloads a file from elsewhere onto the local system, then isn't
> that still USER_ACTIVTITY, even if the user didn't write the (entire) file?
>

That's questionable, I can have a cron job that calls bzr pull in multiple directories. As for downloading something - of course you don't need to write the file yourself for it to be considered a user activity, but in case of a download the user does in fact request that specific file, unlike bzr where you're actually saying update this repository, you don't care about the individual files.

That's why I also don't really like this approach, you're increasing global relevancy of subjects you could have actually never worked with. (consider kernel-like bzr tree and the thousands of files when you do a single pull).

[Jelmer Vernooij (jelmer)]

Am 24/03/12 23:14, schrieb Michal Hruby:
>> The action is triggered by the user though, it's not an external action. If
>> the user downloads a file from elsewhere onto the local system, then isn't
>> that still USER_ACTIVTITY, even if the user didn't write the (entire) file?
>>
> That's questionable, I can have a cron job that calls bzr pull in multiple directories. As for downloading something - of course you don't need to write the file yourself for it to be considered a user activity, but in case of a download the user does in fact request that specific file, unlike bzr where you're actually saying update this repository, you don't care about the individual files.
What if it's e.g. unpacking a zip file?

Anyway, I think this all depends on the zeitgeist data model and what
you want to do with it.

> That's why I also don't really like this approach, you're increasing global relevancy of subjects you could have actually never worked with. (consider kernel-like bzr tree and the thousands of files when you do a single pull).
Note that it was seif who proposed that. I think just using the branch
as subject is fine, and that's why my original branch did.

Cheers,

Jelmer

[Seif Lotfy (seif)]

> Am 24/03/12 23:14, schrieb Michal Hruby:
> >> The action is triggered by the user though, it's not an external action. If
> >> the user downloads a file from elsewhere onto the local system, then isn't
> >> that still USER_ACTIVTITY, even if the user didn't write the (entire) file?
> >>
> > That's questionable, I can have a cron job that calls bzr pull in multiple
> directories. As for downloading something - of course you don't need to write
> the file yourself for it to be considered a user activity, but in case of a
> download the user does in fact request that specific file, unlike bzr where
> you're actually saying update this repository, you don't care about the
> individual files.
> What if it's e.g. unpacking a zip file?
>
> Anyway, I think this all depends on the zeitgeist data model and what
> you want to do with it.
>
> > That's why I also don't really like this approach, you're increasing global
> relevancy of subjects you could have actually never worked with. (consider
> kernel-like bzr tree and the thousands of files when you do a single pull).
> Note that it was seif who proposed that. I think just using the branch
> as subject is fine, and that's why my original branch did.
>
> Cheers,
>
> Jelmer

OK you got me there. You are right. pulling stuff should not put up the subjects. The only subject should be have the uri to the directory and that is it.

[Seif Lotfy (seif)]

OK to sum it up.
When committing/pushing we add a subject for each changed file. When pulling or branching we just add one subject with the uri of the directory.
Tadaaaaaaaa

[Siegfried Gevatter (rainct)]

2012/3/25 Seif Lotfy <email address hidden>:
>> > That's why I also don't really like this approach, you're increasing global
>> relevancy of subjects you could have actually never worked with.

Not if you're only looking at UserActivity events. Events modified by
others are also relevant (ie. I may want a launcher to give me the
last files modified in a project; this is also relevant for eg.
Dropbox).

[Jelmer Vernooij (jelmer)]

> > I think the other subjects should only be set in the case of push and pull,
> and not in the case of commit as commit doesn't actually change those files.
>
> The think the reverse. In a commit there are various files involved. A file
> can be added/deleted/modified which form a part of an commit (which is an
> event)
The file itself isn't modified as part of commit - only the bzr branch/repo is. Modifying the files themselves was done earlier by the user using their editor.

[Jelmer Vernooij (jelmer)]

> That's why I also don't really like this approach, you're increasing global
> relevancy of subjects you could have actually never worked with. (consider
> kernel-like bzr tree and the thousands of files when you do a single pull).
Note that this scaling issue also applies to commits, as you can commit a merge (which would involve the same amount of changes as a pull or push).

[Manish Sinha (मनीष सिन्हा) (manishsinha)]

I talked with mhr3 and RainCT. We three agreed that we should keep files subjects out for now.

file subjects make sense only in pull request, but let's add them later instead of now

[Jelmer Vernooij (jelmer)]

Manish, are you updating the branch or should I ?

=== modified file 'bzr/Makefile.am'
--- bzr/Makefile.am	2010-11-09 19:33:59 +0000
+++ bzr/Makefile.am	2012-03-24 13:04:20 +0000
@@ -1,7 +1,7 @@
 EXTRA_DIST = bzr-icon-64.png
 
 bzrplugindir = $(datadir)/pyshared/bzrlib/plugins/zeitgeist
-dist_bzrplugin_DATA = __init__.py
+dist_bzrplugin_DATA = __init__.py hooks.py
 
 bzricondir = $(datadir)/pixmaps
 bzricon_DATA = bzr-icon-64.png
@@ -13,6 +13,7 @@
 local-install:
 	mkdir -p $(BZR_PLUGIN_HOME_DIR)
 	cp __init__.py $(BZR_PLUGIN_HOME_DIR)
+	cp hooks.py $(BZR_PLUGIN_HOME_DIR)
 	mkdir -p $(BZR_ICON_DIR)
 	cp bzr-icon-64.png $(BZR_ICON_DIR)
 

=== modified file 'bzr/__init__.py'
--- bzr/__init__.py	2011-08-31 13:13:33 +0000
+++ bzr/__init__.py	2012-03-24 13:04:20 +0000
@@ -30,102 +30,16 @@
 Copy this directory to ~/.bazaar/plugins/zeitgeist/*
 """
 
-import time
-from bzrlib import (
-    branch,
-    trace,
-    )
-
-_client = None
-_client_checked = None
-
-def get_client():
-    global _client_checked, _client
-    if _client_checked:
-        return _client
-    _client_checked = True
-    try:
-        from zeitgeist.client import ZeitgeistClient
-    except ImportError:
-        _client = None
-        return _client
-    import dbus
-    try:
-        _client = ZeitgeistClient()
-    except dbus.DBusException, e:
-        trace.warning("zeitgeist: %s. No events will be sent." % e.message)
-        _client = None
-    except Exception, e:
-        trace.warning("Unable to connect to Zeitgeist, won't send events. "
-                      "Reason: '%s'" % e)
-        _client = None
-    return _client
-
-
-def post_commit(local, master, old_revno, old_revid, new_revno, new_revid):
-    client = get_client()
-    if client is None:
-        return
-    from zeitgeist.datamodel import Event, Subject, Interpretation, Manifestation
-    revision = master.repository.get_revision(new_revid)
-    if new_revno == 1:
-        interpretation = Interpretation.CREATE_EVENT
-    else:
-        interpretation = Interpretation.MODIFY_EVENT
-    _text  = _("Commited on: ")
-    _text += master.base[7:-1] #don't considere file://
-    _text += _(" Revision no. : ")
-    _text += str(new_revno) + "\n"
-    _text += revision.message.rstrip()
-
-    subject = Subject.new_for_values(
-        uri=unicode(master.base),
-        interpretation=unicode(Interpretation.FOLDER),
-        manifestation=unicode(Manifestation.FILE_DATA_OBJECT),
-        text=unicode(_text),
-        origin=unicode(master.base),
-    )
-    event = Event.new_for_values(
-        timestamp=int(time.time()*1000),
-        interpretation=unicode(interpretation),
-        manifestation=unicode(Manifestation.USER_ACTIVITY),
-        actor="application://bzr.desktop", #something usefull here, always olive-gtk?
-        subjects=[subject,]
-    )
-    client.insert_event(event)
-
-
-def post_pull(pull_result):
-    client = get_client()
-    if client is None:
-        return
-    from zeitgeist.datamodel import Event, Subject, Interpretation, Manifestation
-    master = pull_result.master_branch
-    revision = master.repository.get_revision(pull_result.new_revid)
-    interpretation = Interpretation.RECEIVE_EVENT
-    _text = _("Pulled ")
-    _text += master.base[7:-1] #don't considere file://
-    _text += (" to revision ")
-    _text += str(master.revno())+":\n"
-    _text += revision.get_summary()
-    subject = Subject.new_for_values(
-        uri=unicode(master.base),
-        interpretation=unicode(Interpretation.FOLDER),
-        manifestation=unicode(Manifestation.FILE_DATA_OBJECT),
-        text=unicode(_text),
-        origin=unicode(master.base),
-    )
-    event = Event.new_for_values(
-        timestamp=int(time.time()*1000),
-        interpretation=unicode(interpretation),
-        manifestation=unicode(Manifestation.USER_ACTIVITY),
-        actor="application://bzr.desktop", #something usefull here, always olive-gtk?
-        subjects=[subject,]
-    )
-    client.insert_event(event)
-
-
-branch.Branch.hooks.install_named_hook("post_commit", post_commit,
-                                       "Zeitgeist dataprovider for bzr")
-branch.Branch.hooks.install_named_hook("post_pull", post_pull,
-                                       "Zeitgeist dataprovider for bzr")
+from __future__ import absolute_import
+
+from bzrlib import branch
+
+branch.Branch.hooks.install_named_hook_lazy("post_commit",
+    "bzrlib.plugins.zeitgeist.hooks", "post_commit",
+    "Zeitgeist dataprovider for bzr")
+branch.Branch.hooks.install_named_hook_lazy("post_pull",
+    "bzrlib.plugins.zeitgeist.hooks", "post_pull",
+    "Zeitgeist dataprovider for bzr")
+branch.Branch.hooks.install_named_hook_lazy("post_push",
+    "bzrlib.plugins.zeitgeist.hooks", "post_push",
+    "Zeitgeist dataprovider for bzr")

=== added file 'bzr/hooks.py'
--- bzr/hooks.py	1970-01-01 00:00:00 +0000
+++ bzr/hooks.py	2012-03-24 13:04:20 +0000
@@ -0,0 +1,205 @@
+# -.- coding: utf-8 -.-
+
+# Zeitgeist
+#
+# Copyright © 2009 Markus Korn <[e-mail]>
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import os.path
+
+from gi.repository import Gio
+
+from bzrlib import (
+    branch,
+    errors,
+    trace,
+    urlutils,
+    workingtree,
+    )
+
+_client = None
+_client_checked = None
+
+def get_client():
+    global _client_checked, _client
+    if _client_checked:
+        return _client
+    _client_checked = True
+    try:
+        from zeitgeist.client import ZeitgeistClient
+    except ImportError:
+        _client = None
+        return _client
+    import dbus
+    try:
+        _client = ZeitgeistClient()
+    except dbus.DBusException, e:
+        trace.warning("zeitgeist: %s. No events will be sent." % e.message)
+        _client = None
+    except Exception, e:
+        trace.warning("Unable to connect to Zeitgeist, won't send events. "
+                      "Reason: '%s'" % e)
+        _client = None
+    return _client
+
+
+def subjects_for_branch(branch, new_revno, old_revno, is_push, message):
+    """
+    Generates the event's subject for commit, push and pull events
+    
+    branch - the bzrlib.branch.Branch instance
+    new_revno - the new revision number generated after a commit, 
+        after a push or a pull
+    old_revno - the old revno which is was present before the commit,
+        push or pull event
+    is_push - True then push, is False then pull, is None then commit
+    message - the Commit message
+    """
+
+    from zeitgeist.datamodel import Subject, Interpretation, Manifestation
+    location = urlutils.unescape_for_display(branch.base, "utf-8").decode("utf-8")
+    
+    if is_push is None:
+        # Commit
+        text = u"Revision: %s, Message: %s" %(new_revno, message)
+    elif is_push is True:
+        # Push
+        text = "Pushed branch from %s to %s" %(old_revno, new_revno)
+    elif is_push is False:
+        # Pull
+        text = "Updated branch from %s to %s" %(old_revno, new_revno)
+    
+    folder_subj =  Subject.new_for_values(
+        uri=unicode(branch.base),
+        interpretation=unicode(Interpretation.FOLDER),
+        manifestation=unicode(Manifestation.FILE_DATA_OBJECT),
+        text=text,
+        origin=unicode(branch.base),
+        mimetype="application/x-bzr",
+    )
+    
+    all_subjs = [folder_subj, ]
+    
+    # List down the files only in case of Commit as in case of 
+    # push or pull, remote URI is used using which constructing
+    # file URI is not possible
+    if is_push is None:
+        prev_tree = branch.repository.revision_tree(branch.get_rev_id(old_revno))
+        next_tree = branch.repository.revision_tree(branch.get_rev_id(new_revno))
+    
+        for (file_id, path, content_change, versioned, parent_id, name, kind,
+             executable) in next_tree.iter_changes(prev_tree):
+             
+            name = path[0] if path[0] is not None else path[1]
+            uri = os.path.join(unicode(branch.base), name)
+             
+            # fi.get_content_type returns the file mimetype 
+            f=Gio.File.new_for_uri(uri)
+            fi=f.query_info("*", Gio.FileQueryInfoFlags.NONE, None)
+             
+            subj = Subject.new_for_values(
+                uri=uri,
+                manifestation=unicode(Manifestation.FILE_DATA_OBJECT),
+                text=text,
+                origin=unicode(branch.base),
+                mimetype=fi.get_content_type(),
+            )
+            all_subjs.append(subj)
+    
+    return all_subjs
+    
+def get_actor():
+    # FIXME: Allow overriding this by e.g. qbzr and bzr-gtk
+    return u"application://bzr.desktop"
+
+
+def post_commit(local, master, old_revno, old_revid, new_revno, new_revid):
+    client = get_client()
+    if client is None:
+        return
+    from zeitgeist.datamodel import Event, Interpretation, Manifestation
+    revision = master.repository.get_revision(new_revid)
+    if new_revno == 1:
+        interpretation = Interpretation.CREATE_EVENT
+    else:
+        interpretation = Interpretation.MODIFY_EVENT
+    _text = _("Revision: ")
+    _text += str(new_revno) + "\n"
+    _text += revision.message.rstrip()
+
+    subjects = subjects_for_branch(master, new_revno, old_revno, 
+                None, revision.message.rstrip())
+    #subjects.set_text(_text)
+    event = Event.new_for_values(
+        timestamp=int(revision.timestamp*1000),
+        interpretation=unicode(interpretation),
+        manifestation=unicode(Manifestation.USER_ACTIVITY),
+        actor=get_actor(),
+        subjects=subjects,
+    )
+    client.insert_event(event)
+
+
+def post_pull(pull_result):
+    client = get_client()
+    if client is None:
+        return
+    from zeitgeist.datamodel import Event, Interpretation, Manifestation
+    try:
+        revision = pull_result.master_branch.repository.get_revision(
+            pull_result.new_revid)
+    except errors.UnsupportedOperation:
+        # Some remote repository formats (e.g. git) don't support looking at invidual
+        # revisions
+        revision = pull_result.source_branch.repository.get_revision(
+            pull_result.new_revid)
+    _text = _("Pulled to revision %s:\n %s") % (pull_result.new_revno,
+            revision.get_summary())
+    subjects = subjects_for_branch(pull_result.master_branch, 
+            pull_result.new_revno, pull_result.old_revno, False, None)
+    event = Event.new_for_values(
+        interpretation=unicode(Interpretation.RECEIVE_EVENT),
+        manifestation=unicode(Manifestation.WORLD_ACTIVITY),
+        actor=get_actor(),
+        subjects=subjects
+    )
+    client.insert_event(event)
+
+
+def post_push(push_result):
+    client = get_client()
+    if client is None:
+        return
+    from zeitgeist.datamodel import Event, Interpretation, Manifestation
+    try:
+        revision = push_result.master_branch.repository.get_revision(
+            push_result.new_revid)
+    except errors.UnsupportedOperation:
+        revision = push_result.source_branch.repository.get_revision(
+            push_result.new_revid)
+    _text = _("Pushed to revision %s:\n %s") % (push_result.new_revno,
+        revision.get_summary())
+    subjects = subjects_for_branch(push_result.master_branch, 
+            push_result.new_revno, push_result.old_revno, True, None)
+    event = Event.new_for_values(
+        interpretation=unicode(Interpretation.SEND_EVENT),
+        manifestation=unicode(Manifestation.USER_ACTIVITY),
+        actor=get_actor(),
+        subjects=subjects
+    )
+    client.insert_event(event)
+
+
+