trac-post-commit-hook should use changeset time, not current time

[jkugler@…]

Currently, when the trac-post-commit-hook runs, it uses now() for its time. This is usually OK, but when the commit hook is run later, such as in a batch situation, or catching up after a malfunction, now() will not be accurate. The patch below fixed this. chgset.date *can* be None if the _get_prop failed to get a date, so a fallback to now() is provided.

Index: contrib/trac-post-commit-hook
===================================================================
--- contrib/trac-post-commit-hook       (revision 7068)
+++ contrib/trac-post-commit-hook       (working copy)
@@ -151,7 +151,10 @@
         self.author = chgset.author
         self.rev = rev
         self.msg = "(In [%s]) %s" % (rev, chgset.message)
-        self.now = datetime.now(utc)
+        if chgset.date:
+            self.now = chgset.date
+        else:
+            self.now = datetime.now(utc)

         cmd_groups = command_re.findall(self.msg)

[daniel.stockman@…]

patch updated to latest version, trailing newline added to patch to avoid error

[Christian Boos]

No, the modification time of the ticket has to be when the ticket is actually modified. The changeset date might be arbitrary (even in Subversion), so using that could lead to things like a ticket modification date being anterior to the ticket creation date, etc.

[Remy Blank]

Ah, ok, I'll change the new ticket update plugin in multirepos, then. I was using the changeset date.

[joshua@…]

Why not simply have code like:

if modify_time < ticket_open_time:
    modify_time = ticket_open_time

That would solve the problem highlighted by cboos.

[Christian Boos]

That would handle the first issue mentioned as an example, not the "etc." part ;-)

In the current way change groups are associated to tickets, the time of the change is a critical information and adding a posteriori a new change in the middle of others will lead to all sorts of problems, so better avoid doing it in the first place.

Besides those technical problems, I think that adding the ticket change at the time the notification is done is the right thing to do: the changeset might have been "on hold" in some other clone (private or staging), and only when landing in a tracked repository the issue will be closed, for example.

Note also that for the majority of Subversion repositories, we will usually have the same "present" time for the changeset and the ticket modification.

Or do you think about another use case where using the changeset time would be more appropriate?

The original reporter talked about a "batch situation", I don't think this can / should happen. The notification is triggered by hooks so they will be called when the changeset is created or modified. Doing a "resync" for example won't trigger the notifications.

[joshua@…]

Heh…I am the original reporter, just have a new e-mail address.

As to the "batch" situation that originally spawned this report: we had a situation arise where due to a glitch (the details of which I forget now), the post-commit-hook wasn't running. So, when it got fixed, and it pulled in all the changes that it didn't currently have, all the change times got recorded as the time we ran the script, not the time at which the changes were made. And no, I don't remember exactly what we used to sync up the database after we fixed the error in the post-commit-hook. It has been almost a year, after all. :)