Edgewall Software
Modify

Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#7251 closed enhancement (wontfix)

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

Reported by: jkugler@… Owned by: Christian Boos
Priority: low Milestone:
Component: version control Version: 0.12dev
Severity: normal Keywords: commit hook
Cc: Branch:
Release Notes:
API Changes:

Description

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)

Attachments (1)

post-commit-changeset-date.patch (556 bytes ) - added by daniel.stockman@… 11 years ago.
patch updated to latest version, trailing newline added to patch to avoid error

Download all attachments as: .zip

Change History (8)

comment:1 by jkugler@…, 11 years ago

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

comment:2 by Noah Kantrowitz, 11 years ago

Component: timelineversion control
Keywords: chgset date removed
Milestone: 0.11.1
Priority: normallow
Version: 0.12dev

by daniel.stockman@…, 11 years ago

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

comment:3 by Christian Boos, 10 years ago

Milestone: 0.11-retriage
Resolution: wontfix
Status: newclosed

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.

comment:4 by Remy Blank, 10 years ago

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

comment:5 by joshua@…, 10 years ago

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.

comment:6 by Christian Boos, 10 years ago

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.

comment:7 by joshua@…, 10 years ago

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. :)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.