(data loss) Suspected bad interaction between hg rebase and format-source

RESOLVED FIXED in Firefox 68

Status

defect
P1
critical
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: aklotz, Assigned: andi)

Tracking

({dataloss})

Trunk
mozilla68
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66 unaffected, firefox67 unaffected, firefox68 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

2 months ago

I have not attempted to reproduce this, but based on what I have seen when reconstituting my patches:

  • When rebasing, any time a file in the patch needs to be merged with upstream (even trivially, this does not require manual resolution of merge conflicts), all the changes made to the affected file are dropped from the resulting patch.
  • In the worst case, entire patches are dropped because the resulting diffs are empty.
12:43:39 PM - aklotz: okay, wtf is going on with hg rebase. It's been vaporizing entire segments of my patches
12:44:45 PM * aklotz is thankful that evolve is still working
12:45:55 PM - sheehan: aklotz: are you using the clang-format/format-source extensions?
12:46:27 PM - aklotz: sheehan: format-source is enabled, yes
12:48:02 PM - sheehan: aklotz: can you reproduce without it? my money is on a weird interaction between rebase and f-s
12:48:24 PM - aklotz: sheehan: Not sure. I'm not too keen on throwing away more data atm
12:50:59 PM - sheehan: aklotz: fair enough. my suggestion would be to disable that extension and see if things get any better. if you can gather enough data for a bug, CC me :)
12:51:29 PM - jmaher|bbiab is now known as jmaher.
12:51:30 PM - aklotz: sheehan: I think the problem occurred any time there was a merge conflict with the file in question. Instead of resolving the merge, the file just got dumped
12:51:48 PM - aklotz: sheehan: Based on my investigation of recovering the lost data and trying to reconstitute my patches
12:53:15 PM - jrmuizel [jrmuizel@corp-nat.fw1.untrust.tor1.mozilla.com] entered the room.
12:53:21 PM - pet-flipper [uid353448@moz-5g3i04.ehhf.l9fb.067c.2001.IP] entered the room.
12:54:55 PM - sheehan: aklotz: yea, that sounds like format-source is the culprit. it's supposed to handle merge conflicts across formatting changes

This has been happening to me. I think I have a reproducible test case, in case anyone wants to look into it.

This is a huge deal. Everyone who uses hg rebase or hg pick should probably turn this extension off right away until it's better understood what happened here.

Reporter

Comment 2

2 months ago

Sylvestre, this is a serious data loss issue. Can you please ask somebody to look into it?

Flags: needinfo?(sledru)

Commenting out [extensions] format-source in my .hgrc fixes it.

It happens regardless of the value of [ui] merge.

(In reply to Jason Orendorff [:jorendorff] from comment #1)

This has been happening to me. I think I have a reproducible test case, in case anyone wants to look into it.

Please share, that will be extremely useful for fixing this bug.

Flags: needinfo?(jorendorff)

Steps to reproduce:

  1. In a clone of mozilla-central, with format-source installed, run these commands:

    wget https://bugzilla.mozilla.org/attachment.cgi?id=9057057 -O bug-1542871.bundle
    hg unbundle bug-1542871.bundle
    hg log -vpr 6ac6782e95e3
    

    You should get this changeset:

    changeset:   539033:6ac6782e95e3
    tag:         tip
    parent:      538768:93075ec49df3
    user:        Jason Orendorff <jorendorff@mozilla.com>
    date:        Tue Apr 09 13:46:04 2019 -0500
    files:       js/src/frontend/Parser.cpp js/src/frontend/TokenStream.h
    description:
    Bug 1539821 - Part 1: Delete ModifierException::NoneIsOperand. r?jwalden
    

    Note that the changeset contains about 6 hunks of changes.

  2. Now run these commands:

    hg rebase -r 6ac6782e95e3 -d central
    hg log -vpr tip
    

Expected: Either the rebased changeset contains all the hunks; or hg rebase fails with merge conflicts.

Actual: Rebase succeeds, losing all the hunks but one.

Flags: needinfo?(jorendorff)

I'm using Mercurial 4.8.

This is quite surprising.

I see two recent changes in hg format-source:

changeset:   6917:2c7002f9e788
user:        Connor Sheehan <sheehan@mozilla.com>
date:        Tue Apr 02 20:29:37 2019 +0000
summary:     py3: remove Python 3 incompatible calls to `execfile` (Bug 1541166) r=lars

changeset:   6904:848dc37f2562
user:        Connor Sheehan <sheehan@mozilla.com>
date:        Wed Mar 27 12:50:05 2019 -0400
summary:     hgext: add `buglink` field to source formatting related extensions

Connor, do you think one of these two changes could have created that?

Anyway, I think it is time to disable hg format-source. It was enabled to address the big C++ reformat.
Patches which needed to be rebase are probably done now.
Andi, could you please take care of that tomorrow? (please note that we might need it when we will reformat js and/or python and/or rust)

Flags: needinfo?(sledru)
Flags: needinfo?(sheehan)
Flags: needinfo?(bpostelnicu)
Priority: -- → P1

I don't think those changes would have caused any issues. 2c7002f9e788 only changed the way we make libraries available for Mercurial extensions in v-c-t (via path mangling) to be Python 3 compatible and shouldn't have changed any behaviour. 848dc37f2562 only added a buglink field so any internal Mercurial failures are accompanied by that field to point users at the right Bugzilla component. The tests are all passing, even after these changes.

I'm having some trouble getting the test case to reproduce correctly, as the rebase simply fails with "abort: clang-format: mach exited with status 1". Running hg diff at this point shows nothing, but then hg status shows:

# The repository is in an unfinished *rebase* state.

# Unresolved merge conflicts:
# 
#     js/src/frontend/Parser.cpp
#     js/src/frontend/TokenStream.h

Even if I manually hg resolve -m js/src/frontend/Parser.cpp && hg resolve -m js/src/frontend/TokenStream.h and then hg rebase --continue, the rebase continues to fail.

This is my format-source config, which probably needs updating:

[format-source]
clang-format = /home/connor/mozilla-unified/mach clang-format --assume-filename $HG_FILENAME -p
clang-format:configpaths = .clang-format, .clang-format-ignore
clang-format:fileext = .cpp, .c, .h

If the plan is to turn off format-source in mach vcs-setup, I can assist with that work.

Flags: needinfo?(sheehan)

Could this be due to the change in bug 1542146? I did mach bootstrap, as suggested in comment 5 of that bug, and then tried again. The rebase succeeded with all hunks present.

That's what I wanted, but it seems like there must be some error-handling code in the extension that isn't right. (?)

Also, this is still different behavior from what Connor saw.

Okay, I managed to get the rebase to succeed as well. I ran mach bootstrap, then the rebase still failed, so I tried mach clang-format just as a smoke test, which told me I needed to run a clobber (it's been a while since I built Firefox, as I'm working in developer services and don't need to do so very often). I ran mach clobber and then mach clang-format. Once that completed, the rebase went through without issue.

My initial thought is that my underlying call to clang-format was hitting the clobber error and giving exit code 1, which caused the rebase to fail instead of actually hit the data loss bug. If that's the case, I'd say Jason is correct that the error handling code in format-source needs some work.

Why the merges were dropping hunks, I'm still not sure.

we could just comment the StatementMacros line as a quick fix.

Useful things to know when recovering:

  • hg obslog REV shows the history of rebased/amended/etc. changesets that ended up being turned into REV.

  • hg touch --hidden --duplicate -r REV..REV can be used to duplicate those old rebased changesets, effectively bringing them back to life.

Possibly unrelated but I had the following trouble with hg qref today after popping all patches and then re-applying a couple.

Prior to this I disabled format-source in .hgrc as instructed in dev-platform and confirmed that it does not show up in hg config.

However, I still got the very odd interaction which I've never seen before:

> hg qpush
> hg qref -e

Editor pops up twice (i.e. write message, close editor, then it opens again with the original commit message). Odd.

> hg qpush
> hg qref -e

Again, editor pops up twice. Really odd. Show it to a colleague.

> hg qref -e

Again, same problem.

> hg glog

I see:

* Second patch
* Second patch
* Second patch
* First patch
* First patch (qparent)

i.e. the first patch has been committed(!) and both have 1~2 duplicate changesets. Inspecting the contents of the changesets using hg log -p -r xxx I get:

* Second patch -> empty
* Second patch -> empty
* Second patch -> empty
* First patch -> empty
* First patch -> contents of first patch

i.e. I've lost the contents of the second patch entirely.

> hg qapplied

* Second patch
* First patch

Inspecting the contents of the files, indeed the second patch is empty.

Incidentally, since a few days ago I have been getting Processing 2 file(s)... when I push patches.

e.g.

> hg push <some patch>

applying addRemoveIDL
Processing 3 file(s)...
applying convertTickToSync
Processing 1 file(s)...
applying addIsRemovable
Processing 2 file(s)...
applying trackRemovableElements
Processing 3 file(s)...
...

The number of files seems to match the number that would match the pattern in https://hg.mozilla.org/hgcustom/version-control-tools/rev/0c95e8341526#l1.23 which makes me wonder if bug 1542632 is related.

Brian, sorry but these tools aren't supported or tested with mq as it has been deprecated for a long time

Assignee

Comment 16

2 months ago

Indeed the issue is related with bug 1542146. This happens only to users who rebase on heads that have that option in .clang-format and are not using the latest version of clang-tidy package. I can confirm the issue is not in the extension itself but in the way how we wrap clang-format for the extension.
Will have a patch for this shortly.

Flags: needinfo?(bpostelnicu)
Assignee

Updated

2 months ago
Assignee: nobody → bpostelnicu
Assignee

Comment 17

2 months ago

This issue has been detected and reproduced by :klotz and :jorendorff.

Assignee

Comment 18

2 months ago

The attached patch should solve the issue.

Comment 19

2 months ago
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/35e35da9ae0e
prevent data loss for clang-format when using incompatible/old clang-format binary. r=sylvestre

Comment 20

2 months ago

Is this fundamentally the same issue as in bug 1532733 ?

Reporter

Comment 21

2 months ago

Is there a test suite for any of this stuff?

Yes and no, we have clang-format & format-sources extensions tests but not together

Comment 23

2 months ago
bugherder
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Updated

2 months ago
Depends on: 1544590
See Also: → 1544968
You need to log in before you can comment on or make changes to this bug.