Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add a hook to detect changesets with wrong file metadata

ASSIGNED
Assigned to

Status

Developer Services
Mercurial: hg.mozilla.org
ASSIGNED
4 years ago
7 months ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Details

(Whiteboard: [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/1055] )

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
See e.g. changeset http://hg.mozilla.org/mozilla-central/rev/ad1334d4a447ba5f9e96a2d6a2cf2f26009e4e3a which is the last occurrence. There is a difference between the list of files and the actual changes. I wrote a hook detecting and rejecting those.
(Assignee)

Comment 1

4 years ago
Created attachment 8363271 [details] [diff] [review]
Reject changesets with wrong file metadata

Not who's the best person to review this.
Attachment #8363271 - Flags: review?(gps)
(Assignee)

Comment 2

4 years ago
(In reply to Mike Hommey [:glandium] from comment #1)
> Created attachment 8363271 [details] [diff] [review]
> Reject changesets with wrong file metadata
> 
> Not who's the best person to review this.

s/Not/Don't know/

I'll add a few notes:
- I did run the code on recent copy of mozilla-central *without* the rebase_source check, and it found 488 broken rebased changesets.
- Of those 488, I checked a few random ones as well as the most recent ones, and they were all broken.
- Every single one of those 488 had the rebase_source information.
- I did check that the optimized manifest diff code returns the same thing as the two lines of mercurial API code for all (parent, changeset) where changeset has a rebase_source info in mozilla-central.
Several questions on how we'd deploy and use this:

What's the run time on this? Not worried about single landing but also Sheriff merges from inbound to m-c and also relman uplifts when the trains run.

I.e. do we need a recommendation to only install on certain repos? (or easier maybe to list exlcusions)

Also, what is the plan for dealing with historical issues you've detected?

Other than giving incorrect information in the web ui of hg.m.o, are there any other issues caused by these commits?

Comment 4

4 years ago
Comment on attachment 8363271 [details] [diff] [review]
Reject changesets with wrong file metadata

Review of attachment 8363271 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozhghooks/changelog_correctness.py
@@ +12,5 @@
> +    #   modified, added, removed = repo.status(cs2.node(), cs1.node())[:3]
> +    #   changed_files = set(modified) | set(added) | set(removed)
> +    # Presumably, using repo.manifest.revdiff would be much faster, but
> +    # it raises exceptions when used on changesets part of the push.
> +    # So a manual diff of the manifests, as below, is faster.

What exception does this raise? I'm looking at the Mercurial source, and pretxnchangegroup is called after the incoming bundle is fully applied to revlogs. You /should/ be able to access everything from that hook.

I'd strongly prefer our hook didn't reimplement Mercurial internals like this. I'm tempted to grant r- because of that.

@@ +62,5 @@
> +    rev = repo.changectx(node).rev()
> +    tip = repo.changectx('tip').rev()
> +    merges = 0
> +    for i in xrange(rev, tip + 1):
> +        cs = repo.changectx(i)

Nit: ctx is used throughout Mercurial to identify changeset contexts.

@@ +70,5 @@
> +            continue
> +
> +        # The inconsistency we're looking for only happens on rebases.
> +        if 'rebase_source' not in cs.extra():
> +            continue

Please comment your findings that confirmed this, just for posterity.

@@ +84,5 @@
> +        print "Following is the list of changesets from your push with"
> +        print "inconsistent metadata:"
> +        for c in broken:
> +            print '   ', c
> +        # Print a suggestion as to how to fix it.

This message needs some work.

I would say "hg rebase" everywhere you used "rebase." I'd tell people to upgrade to Mercurial 2.6 or newer. I'd explicitly state this is a bug with hg rebase in versions before 2.6.

I'd also print the command they can use to fix things. Although, hg rebase -s rev -d rev is smart enough to no-op if the rebase would be a no-op. That makes things a little difficult...

I'm also pretty sure you it's recommended to use ui.write() instead of print.
Attachment #8363271 - Flags: review?(gps) → feedback+
(Assignee)

Comment 5

4 years ago
(In reply to Hal Wine [:hwine] (use needinfo) from comment #3)
> Several questions on how we'd deploy and use this:
> 
> What's the run time on this? Not worried about single landing but also
> Sheriff merges from inbound to m-c and also relman uplifts when the trains
> run.

On my machine it takes 4s for a push with ~700 changesets.
 
> I.e. do we need a recommendation to only install on certain repos? (or
> easier maybe to list exlcusions)

Considering this is caused by rebase, I'm afraid this can affect train branches (aurora, beta, etc.), although actual data may say differently. I guess people would more likely transplant than rebase on those.

> Also, what is the plan for dealing with historical issues you've detected?

At this point, there's nothing we can really do about them.

> Other than giving incorrect information in the web ui of hg.m.o, are there
> any other issues caused by these commits?

ISTR there were issues with hg blame and such, but gps might have better insight.
(Assignee)

Comment 6

4 years ago
(In reply to Gregory Szorc [:gps] from comment #4)
> Comment on attachment 8363271 [details] [diff] [review]
> Reject changesets with wrong file metadata
> 
> Review of attachment 8363271 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozhghooks/changelog_correctness.py
> @@ +12,5 @@
> > +    #   modified, added, removed = repo.status(cs2.node(), cs1.node())[:3]
> > +    #   changed_files = set(modified) | set(added) | set(removed)
> > +    # Presumably, using repo.manifest.revdiff would be much faster, but
> > +    # it raises exceptions when used on changesets part of the push.
> > +    # So a manual diff of the manifests, as below, is faster.
> 
> What exception does this raise? I'm looking at the Mercurial source, and
> pretxnchangegroup is called after the incoming bundle is fully applied to
> revlogs. You /should/ be able to access everything from that hook.

It raises an exception in revlog.deltaparent, on self.index[rev][3], because self.index[rev] returns the rev number, not a list or whatever this is for changesets that are there.

(...) 
> I'm also pretty sure you it's recommended to use ui.write() instead of print.

We're using print in other hooks. I just mimicked.

That said, a really fast hook would be to just forbid rebase changesets altogether. This information is actually mostly useless noise: it tells you want changeset was rebased, but those are local to developers and are nowhere to be found. The only indication it can give is that the changeset *was* rebased, but there are so many that are rebased and not marked (anything where mq was used, for example) that it's not very beneficial.

Comment 7

4 years ago
Mercurial changeset objects store a list of changed paths. This allows Mercurial to resolve which changesets changed which paths without having to perform a secondary (and expensive) lookup by computing the delta between two manifests. This is a really big deal for the Firefox repo because manifests are large and accessing their data takes a long time in comparison to changeset scanning.

The underlying bug is that the list of files in the changeset (the cached copy if you will) is wrong due to a bug. I'm not sure what else this will impact in Mercurial. I'm guessing things like {filechanges} in templates are probably impacted.

We can't fix the issue in existing repos without changing SHA-1's. If we ever rewrote history, we would want to fix this issue as part of the conversion.

Comment 8

4 years ago
(In reply to Mike Hommey [:glandium] from comment #6)
> (In reply to Gregory Szorc [:gps] from comment #4)
> > What exception does this raise? I'm looking at the Mercurial source, and
> > pretxnchangegroup is called after the incoming bundle is fully applied to
> > revlogs. You /should/ be able to access everything from that hook.
> 
> It raises an exception in revlog.deltaparent, on self.index[rev][3], because
> self.index[rev] returns the rev number, not a list or whatever this is for
> changesets that are there.

Weird. Mercurial "monkeypatches" the index during appends and redirects accesses to incoming changesets before the transaction is committed. This *should* work. Perhaps its another Mercurial bug?

The use of deltaparent there is worrying. I think I was hitting that while testing some implementations. Try the following instead?

        pnode = parents[0].manifestnode()
        revdiff = m.revdiff(m.rev(pnode), m.rev(ctx.manifestnode()))
        patch = mdiff.patchtext(revdiff)
        diff = m.parse(patch)
        expected = set(diff.keys())

> That said, a really fast hook would be to just forbid rebase changesets
> altogether. This information is actually mostly useless noise: it tells you
> want changeset was rebased, but those are local to developers and are
> nowhere to be found. The only indication it can give is that the changeset
> *was* rebased, but there are so many that are rebased and not marked
> (anything where mq was used, for example) that it's not very beneficial.

For m-c, yes, this info makes little sense. But for aurora, beta, esr, etc, it helps a lot to identify the actual source of changesets. I started a thread a while ago about possibly even requiring the uplifts be annotated with this metadata if an uplifted changeset originated with an m-c commit. Hard problem.
(Assignee)

Comment 9

4 years ago
(In reply to Gregory Szorc [:gps] from comment #8)
> (In reply to Mike Hommey [:glandium] from comment #6)
> > (In reply to Gregory Szorc [:gps] from comment #4)
> > > What exception does this raise? I'm looking at the Mercurial source, and
> > > pretxnchangegroup is called after the incoming bundle is fully applied to
> > > revlogs. You /should/ be able to access everything from that hook.
> > 
> > It raises an exception in revlog.deltaparent, on self.index[rev][3], because
> > self.index[rev] returns the rev number, not a list or whatever this is for
> > changesets that are there.
> 
> Weird. Mercurial "monkeypatches" the index during appends and redirects
> accesses to incoming changesets before the transaction is committed. This
> *should* work. Perhaps its another Mercurial bug?

Seems plausible.

> The use of deltaparent there is worrying.

It's not. It's an optimization:

  if rev1 != nullrev and self.deltaparent(rev2) == rev1:
    return str(self._chunk(rev2))

that is: if the deltaparent of rev2 is rev1, just return the delta chunk directly, instead of computing a diff.

> For m-c, yes, this info makes little sense. But for aurora, beta, esr, etc,
> it helps a lot to identify the actual source of changesets. I started a
> thread a while ago about possibly even requiring the uplifts be annotated
> with this metadata if an uplifted changeset originated with an m-c commit.
> Hard problem.

Transplant stores "transplant_source", not "rebase_source".
I thought you could trick rebase into "copying" changesets. I'm wrong: it complains when you attempt to rebase immutable/public changesets. Graft or transplant is what I'm thinking of.
(Assignee)

Comment 11

4 years ago
So, how do we go ahead here? Do we simply forbid any changeset with rebase markers? Or go ahead with my optimized hook?
Do we have a decent recourse for stripping rebase markers? We may have to write a simple extension to make that somewhat foolproof. The one-liner is a bunch of export + strip + import. Seems dangerous.
(Assignee)

Comment 13

4 years ago
qimport/qpop/qpush?
(In reply to Mike Hommey [:glandium] from comment #13)
> qimport/qpop/qpush?

Only works with mq. There are a growing number of us who have seen the light and are ditching mq for branches or bookmarks.
(Assignee)

Comment 15

4 years ago
You can still use mq with branches and bookmarks for such a one-off. And that could be part of the one-liner: hg --config extensions.mq= qimport...
It works for a single patch. What about modifying tip~4? You'll introduce a new anonymous head if you replace that changeset. I think you need an extension or short script to cover all the possibilities.
(Assignee)

Comment 17

4 years ago
qimport works for more than a single patch. But I just tried with a bookmark, and it apparently doesn't handle doing qpop gracefully.
How about an extension that would make the standard rebase not emit rebase_source? We'd tell people "upgrade mercurial and re-rebase" if they were using a broken version, and the second rebase would not have broken file metadata and would not have rebase_source.
Globally stripping rebase_source feels like too big of a hammer. Providing an extension that strips rebase_source from non-immutable changesets that are a parent of a specific rev or the working copy [and rewrites history] feels like the right way to go.

FWIW, we had a discussion a few months back about creating a unified repository for all these misc extensions. I filed bug 965330 to (finally) have that repo created. This extension could potentially be the first customer!
(Assignee)

Comment 19

4 years ago
Can we at least go forward with this hook until there is such an extension?
(Assignee)

Comment 20

3 years ago
For the record, since comment 2, 38 more slipped in, and the most recent is http://hg.mozilla.org/mozilla-central/rev/822a89cfe094
(Assignee)

Comment 21

3 years ago
Created attachment 8479525 [details] [diff] [review]
Reject changesets with wrong file metadata

This is essentially the same patch, except it applies to the new repository, adds some comments, and has a unit test.

Note I added an arbitrary wiki url where we could put various instructions on how to unfuck mercurial, with instructions with this particular issue.
Attachment #8479525 - Flags: review?(gps)
(Assignee)

Updated

3 years ago
Attachment #8363271 - Attachment is obsolete: true
Comment on attachment 8479525 [details] [diff] [review]
Reject changesets with wrong file metadata

This also needs an 'IGNORE BROKEN CHANGESETS' like https://hg.mozilla.org/hgcustom/version-control-tools/file/6d6cc743a5af/hghooks/mozhghooks/prevent_broken_csets.py#l34 for merges from trunk to aurora etc, as well as merging broken csets from say fx-team to mozilla-central in the day after this lands.
Comment on attachment 8479525 [details] [diff] [review]
Reject changesets with wrong file metadata

Nice work!

Make sure though to create http://wiki.mozilla.org/Troubleshooting_Mercurial#Fix_rebase before this lands - looks like it doesn't exist yet.
Comment on attachment 8479525 [details] [diff] [review]
Reject changesets with wrong file metadata

Review of attachment 8479525 [details] [diff] [review]:
-----------------------------------------------------------------

::: hghooks/mozhghooks/changelog_correctness.py
@@ +88,5 @@
> +            # Merge changesets don't store the same kind of file list.
> +            continue
> +
> +        # Running this check thoroughly on all changesets reveals that all the
> +        # detected onces have a 'rebase_source' marker. As the test is rather

s/onces/ones/
Attachment #8479525 - Flags: review?(gps) → review+
(Assignee)

Comment 25

3 years ago
http://hg.mozilla.org/hgcustom/version-control-tools/rev/19e4d0593f00

Now, this would need to be deployed and http://wiki.mozilla.org/Troubleshooting_Mercurial#Fix_rebase to be created.

(In reply to Ed Morley [:edmorley] from comment #22)
> Comment on attachment 8479525 [details] [diff] [review]
> Reject changesets with wrong file metadata
> 
> This also needs an 'IGNORE BROKEN CHANGESETS' like
> https://hg.mozilla.org/hgcustom/version-control-tools/file/6d6cc743a5af/
> hghooks/mozhghooks/prevent_broken_csets.py#l34 for merges from trunk to
> aurora etc, as well as merging broken csets from say fx-team to
> mozilla-central in the day after this lands.

Or, we could enable it on all branches but aurora, beta and release, and make it "ride" the train. Or we could enable it everywhere, and disable it temporarily when required for a merge.
I guess i should have set the flag explicitly, but this is r- from me until 'IGNORE BROKEN CHANGESETS' or similar is addressed. 

Disabling the hook doesn't help us for merging integration repos around on trunk in the first few days of this landing. This would result in a tree closure whilst we requested that IT disable the hook. (if sheriffs had access to the hgrcs i might say it would be ok,  but we don't). 

It can always be removed later once everything merges around on trunk and changes have propagated through to release.
Status: NEW → ASSIGNED
(Assignee)

Comment 27

3 years ago
If the hook is enabled with no *new* broken changesets on any integration branches, then there is no problem.
(Assignee)

Comment 28

3 years ago
The last broken one was two weeks ago.
Product: Release Engineering → Developer Services

Updated

3 years ago
Whiteboard: [kanban:engops:https://kanbanize.com/ctrl_board/6/184]

Updated

3 years ago
Depends on: 1075275

Updated

3 years ago
Whiteboard: [kanban:engops:https://kanbanize.com/ctrl_board/6/184] → [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/1045] [kanban:engops:https://kanbanize.com/ctrl_board/6/184]

Updated

3 years ago
Whiteboard: [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/1045] [kanban:engops:https://kanbanize.com/ctrl_board/6/184] → [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/1054] [kanban:engops:https://kanbanize.com/ctrl_board/6/184]

Updated

3 years ago
Whiteboard: [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/1054] [kanban:engops:https://kanbanize.com/ctrl_board/6/184] → [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/1055] [kanban:engops:https://kanbanize.com/ctrl_board/6/184]
Whiteboard: [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/1055] [kanban:engops:https://kanbanize.com/ctrl_board/6/184] → [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/1055]
QA Contact: hwine → klibby
You need to log in before you can comment on or make changes to this bug.