support "tip" and "default" in pushlog?fromchange=...&tochange=tip

RESOLVED FIXED

Status

defect
RESOLVED FIXED
11 years ago
5 years ago

People

(Reporter: Pike, Assigned: djc)

Tracking

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

11 years ago
It'd be nice to be able to get all changes after a particular changeset.

One way to do that would be to specify "tip" as tochange, which is probably a good thing to do anyway. I guess "default" should behave the same way.

The other way would be to make the tochange= argument optional.

I guess doing both would be cool, even. Maybe make "tip" the default value for tochange, that should be good enough.
Reporter

Comment 1

11 years ago
djc, could you confirm that we just need to apply the following to both tochange and fromchange in pushlog-feed.py around lines 134f?

from mercurial.node import hex

def lookup(changespec):
  return hex(web.repo.lookup(changespec))

(I didn't find a nicer way to get back to hex than to import it from node.)
Reporter

Comment 2

11 years ago
PS: AFAICT, that should even include tags then, so we could do things like "get all changes since FIREFOX_3_1_0_RELEASE" or somesuch.
Assignee

Comment 3

11 years ago
Pike: I'll be hacking on pushlog some more today, I'll make sure to fix this bug.
Assignee

Updated

11 years ago
Assignee: nobody → dirkjan
Assignee

Updated

11 years ago
Status: NEW → ASSIGNED
Reporter

Comment 4

11 years ago
This is the adhoc patch, untested even.

I didn't add any error reporting, as there isn't any error reporting for any other error so far, if you pass in junk, it'll raise an mercurial.repo.RepoError: unknown revision 'junk'.

This seems to be the least cost route to cause less load on hg from our pollers, see bug 452102.
Attachment #335541 - Flags: review?(ted.mielczarek)
Attachment #335541 - Flags: review?(dirkjan)
Assignee

Comment 5

11 years ago
Hmm, yeah, sorry for taking so long on this. I think your patch is mostly fine, but need to check if pushlog keeps full id's (hex) or shortened versions (short).
Reporter

Comment 6

11 years ago
I did a .dump on one of Ted's test databases, and see lines like

INSERT INTO "changesets" VALUES(5, 11, '6a42c9b08be22189856ec006fcb3a68ac814ee61');
Reporter

Comment 7

11 years ago
.... thinking about this, I guess we can simplify getpushlogentriesbychangeset after doing lookup:

    fromchange += "%"
    e = conn.execute("SELECT pushid FROM changesets WHERE node LIKE ?", (fromchange,)).fetchone()

should become

    e = conn.execute("SELECT pushid FROM changesets WHERE node == ?", (fromchange,)).fetchone()


Is there a way to test this without a website running?
Assignee

Comment 8

11 years ago
I'll check it out. Should work just fine.
Assignee

Comment 9

11 years ago
This is my attempt at this. It gives the current time for empty feeds (as there are no csets to get the time from), and picks the null revision as the default fromchange value. That's not necessarily effective, though, as that one is probably not in the pushlog db. So I'm not sure what a good default for the fromchange would be. I've used "default" as the tochange default, I think that makes more sense than tip in the face of named branches.
Attachment #335541 - Attachment is obsolete: true
Attachment #335541 - Flags: review?(ted.mielczarek)
Attachment #335541 - Flags: review?(dirkjan)
Reporter

Comment 10

11 years ago
Comment on attachment 335689 [details] [diff] [review]
Patch to add sane defaults and run params through repo lookup.

r=me. Ted?
Attachment #335689 - Flags: review+
Reporter

Updated

11 years ago
Attachment #335689 - Flags: review?(ted.mielczarek)
Comment on attachment 335689 [details] [diff] [review]
Patch to add sane defaults and run params through repo lookup.

+    elif 'fromchange' in req.form or 'tochange' in req.form:
+        fromchange = hex(repo.lookup(req.form.get('fromchange', ['null'])[0]))

I think we should make 'fromchange' required. I don't see a good use case for letting people query everything from the first changeset on.

Is repo.lookup expensive, or will this  stay quick?

Looks good to me otherwise.
Attachment #335689 - Flags: review?(ted.mielczarek) → review+
Assignee

Comment 12

11 years ago
I agree about 'fromchange'. Yes, repo.lookup() is very quick. I'll update the patch.
Assignee

Comment 13

11 years ago
Attachment #335689 - Attachment is obsolete: true
Attachment #336317 - Flags: review?(ted.mielczarek)
Reporter

Comment 14

11 years ago
Can we get this reviewed and deployed? I'm really hopeful that we can see significantly less load from our pollers if we can search by range.
Attachment #336317 - Flags: review?(ted.mielczarek) → review+
Assignee

Updated

11 years ago
Depends on: 453606
Assignee

Comment 15

11 years ago
r+ted, deployment bug in 453606.
No longer depends on: 453606
Pushed: http://hg.mozilla.org/users/bsmedberg_mozilla.com/hgpoller/rev/1f5164ca184a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
Product: Release Engineering → Developer Services
You need to log in before you can comment on or make changes to this bug.