466049 - can we somehow avoid having changesets show up in the changelog that are about to be rolled back?

Status: RESOLVED WORKSFORME

(Developer Services :: Mercurial: hg.mozilla.org, defect)

Description

[Johnathan Nightingale [:johnath]]

Today I tested the closed tree hook on mozilla-central and everything worked out fine there.  But the pushlog got confused, it reported the checkin even though it didn't happen.

We suspect that this is caused by the treeclosure hook coming later in the list than the pushlog hook.  Given that we don't want pushlog reporting checkins that we end up punting, the easiest fix is probably to just reorder the lines in the hooks section so that the pushlog hook is last.

Comment 1

[Aravind Gottipati [:aravind]]

So here are the hooks we currently have for mozilla-central.

There is the default hook for all repos (in /etc/mercurial/hgrc)

[hooks]
pretxnchangegroup.z_linearhistory = python:mozhghooks.pushlog.log

And then there are the repo specific hooks (in mozilla-central/.hg/hgrc)
[hooks]
pretxnchangegroup.a_treeclosure = python:mozhghooks.treeclosure.hook
pretxnchangegroup.b_singlehead = python:mozhghooks.single_head_per_branch.hook

It looks like the a and b are setup correctly and should come before the z.  Something else is probably off here.

Comment 2

[(not currently active) Ted Mielczarek]

So, this isn't a pushlog bug or a config bug. This is inherent in the design of mercurial and the way pretxn hooks work. There's a whole section on this in the hg book:
http://hgbook.red-bean.com/hgbookch10.html#x14-20400010.3

firebot pulls the changelog RSS feed, so it saw the checkin while the hook was still running. Once the pretxnchangegroup hooks are running, the changesets are *already in the repository*, which means that anyone can pull them (or see them in the changelog feed). This sounds bad for our purposes, especially if we're going to have a hook that does a HTTP request.

Comment 3

[(not currently active) Ted Mielczarek]

CCing djc in case he has any ideas.

Comment 4

[Benjamin Smedberg]

The standard solution to this is to have separate "incoming" and "outgoing" trees: the checkin doesn't get pushed to the outgoing tree until the last hook.

Comment 5

[Dirkjan Ochtman (:djc)]

I've asked some Mercurial guys. I'm currently on vacation, so spending less time on software stuff, but will look some more next week if I think of it.

Comment 6

[Dirkjan Ochtman (:djc)]

The Mercurial lead agrees with bsmedberg on the gateway repo solution.

Comment 7

[Aravind Gottipati [:aravind]]

What about adding this hook somewhere else, like as a prechangegroup hook instead of pretxnchangegroup.

Comment 8

[Johnathan Nightingale [:johnath]]

(In reply to comment #7)
> What about adding this hook somewhere else, like as a prechangegroup hook
> instead of pretxnchangegroup.

So, prechangegroup would be earlier, so the pushlog would be even more certain to report pushes which didn't end up being permitted.  And we can't move the treeclosed check there, because at prechangegroup time we don't have the incoming changesets yet, so we can't check whether they have said the magic words to check in despite a closed tree.

I had floated the idea yesterday on IRC of moving the pushlog back instead, making it a changegroup hook instead of being pre-anything, since I view it principally as a reporting function based on what has actually landed, not as a thing which might want a vote on whether things get to land or not.

Ted contends that pushlog SHOULD have a vote though, because if a push breaks pushlog for some reason, we shouldn't allow it.  I'm still thinking through whether I agree with him or not.  :)  Obviously no one wants to break pushlog, but if a changeset breaking pushlog is not a likely occurrence, or if we will still get after the fact reporting of that information, then it could be an easy change.  Still, I bow pretty deeply to the other people on this bug when it comes to Hg knowledge...

Comment 9

[Johnathan Nightingale [:johnath]]

Ted has gently reminded me that the pushlog hook isn't running in this case,
and indeed is not a part of Firebot's confusion. Firebot subscribes to the
hg-built-in changelog feed which is not managed by pushlog, or any other hook,
it just reads the repo directly.

He also reminds me that if I had continued reading that chapter of the Hg book, I'd see that the silliness of pretxn hooks like treeclosed is that *while they are running*, while the tinderbox is being asked about tree state, the changeset looks like it has been committed, so things which query the changelog will see it. THAT is what is confusing Firebot here.

Comment 10

[Benjamin Smedberg]

There are two problems here: the fact that the changeset shows up in the feed (which is annoying but transient), and that anyone who pulls http://hg.mozilla.org/mozilla-central in the middle of running the hook will get a non-official changeset. I think the latter is the bigger problem, really: the feed is just a symptom.

Comment 11

[(not currently active) Ted Mielczarek]

Mossop has suggested that we refactor the hook into:
1) A cron job that runs frequently and checks the open/closed status from Tinderbox, and creates or deletes a /repo/.hg/CLOSED file
2) A hook that simply tests for existence of this file to determine "tree closed".

Comment 12

[Dirkjan Ochtman (:djc)]

That shrinks the window, but doesn't eliminate it, right?

Comment 13

[Aravind Gottipati [:aravind]]

For now this belongs in customizations, if we decide that we want to split the server side into two levels, toss it back to server-ops.

Comment 14

[(not currently active) Ted Mielczarek]

hg 1.2 reportedly fixes this by changing the way hooks work.

Comment 15

[(not currently active) Ted Mielczarek]

I think this is fixed.