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

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect, P1)

Description

[(No longer employed by Mozilla) Aaron Klotz]

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 [[e-mail]] entered the room.
12:53:21 PM - pet-flipper [[e-mail]] 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

Comment 1

[Jason Orendorff [:jorendorff]]

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.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 3

[Jason Orendorff [:jorendorff]]

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

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

Comment 4

[Connor Sheehan [:sheehan]]

(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.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: bug-1542871.bundle

Comment 6

[Jason Orendorff [: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 <[e-mail]>
   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.

Comment 7

[Jason Orendorff [:jorendorff]]

I'm using Mercurial 4.8.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

This is quite surprising.

I see two recent changes in hg format-source:

changeset:   6917:2c7002f9e788
user:        Connor Sheehan <[e-mail]>
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 <[e-mail]>
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)

Comment 9

[Connor Sheehan [:sheehan]]

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.

Comment 10

[Jason Orendorff [:jorendorff]]

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.

Comment 11

[Connor Sheehan [:sheehan]]

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.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

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

Comment 13

[Jason Orendorff [:jorendorff]]

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.

Comment 14

[Brian Birtles (:birtles)]

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.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

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

Comment 16

[Andi [:andi]]

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.

Comment 17

[Andi [:andi]]

Attachment: Bug 1542871 - prevent data loss for clang-format when using incompatible/old clang-format binary. r=sylvestre

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

Comment 18

[Andi [:andi]]

The attached patch should solve the issue.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

[Bob Clary [:bc] (inactive)]

Is this fundamentally the same issue as in bug 1532733 ?

Comment 21

[(No longer employed by Mozilla) Aaron Klotz]

Is there a test suite for any of this stuff?

Comment 22

[Sylvestre Ledru [:Sylvestre]]

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

Comment 23

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/35e35da9ae0e