Modify

Opened 3 years ago

Closed 13 months ago

Last modified 3 months ago

#15906 closed defect (fixed)

[Patch] Isn't properly reverting multiple changesets

Reported by: naoliv Owned by: team
Priority: major Milestone:
Component: Plugin reverter Version:
Keywords: Cc: Don-vip, Upliner

Description

Have this object as an example https://www.openstreetmap.org/way/560417360/history
It was inserted in changeset 56253775 and deleted in 56254562.

If I ask to revert both changesets 56253775 56254562 I would expect this object to remain deleted.
ie, by first reverting 56254562, JOSM would restore it; then by reverting 56253775, JOSM should delete it.
In the end, for this specific object, nothing should be changed (since restoring + deleting = its current deleted status)

But the result is very different from this: JOSM just restores the object (and if we upload, we will reinsert it).

The test is very simple here: ask to revert both 56253775 56254562 and see the how object with id:560417360 stays restored.

JOSM:

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2018-02-10 01:40:02 +0100 (Sat, 10 Feb 2018)
Revision:13400
Build-Date:2018-02-10 02:33:49
URL:http://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (13400 pt_BR) Linux Debian GNU/Linux unstable (sid)
Memory Usage: 501 MB / 5120 MB (279 MB allocated, but free)
Java version: 9.0.1+11-Debian-1, Oracle Corporation, OpenJDK 64-Bit Server VM
Screen: :0.0 1920x1080
Maximum Screen Size: 1920x1080
Java package: openjdk-9-jre:amd64-9.0.1+11-1
Java ATK Wrapper package: libatk-wrapper-java:all-0.33.3-15
VM arguments: [--illegal-access=warn, --add-modules=java.se.ee, -Dawt.useSystemAAFontSettings=on]

Plugins:
+ Create_grid_of_ways (33856)
+ ImportImagePlugin (33563)
+ OpeningHoursEditor (33876)
+ PicLayer (34021)
+ SimplifyArea (33918)
+ apache-commons (33668)
+ areaselector (336)
+ austriaaddresshelper (1511306130)
+ buildings_tools (34040)
+ download_along (33710)
+ editgpx (33782)
+ ejml (32680)
+ geojson (73)
+ geotools (33958)
+ jogl (1.1.0)
+ jts (32699)
+ log4j (32699)
+ measurement (33760)
+ merge-overlap (34056)
+ opendata (34019)
+ photo_geotagging (33967)
+ poly (33570)
+ reverter (34036)
+ tageditor (33806)
+ todo (30303)
+ turnlanes-tagging (260)
+ turnrestrictions (33780)
+ undelete (33980)
+ utilsplugin2 (33991)

Attachments (3)

15906.patch (2.0 KB) - added by GerdP 20 months ago.
15906-v2.patch (2.1 KB) - added by GerdP 20 months ago.
correct handling of conflict warnings
15906-v3.patch (3.2 KB) - added by GerdP 13 months ago.
improved version, undo is only used when really wanted

Download all attachments as: .zip

Change History (22)

comment:1 Changed 2 years ago by naoliv

Cc: Don-vip added
Owner: changed from Upliner to team

Vincent, it seems that ​[o34028] reverted the changes from ​[o31596] and reintroduced #11699

And it seems that there is a discrepancy between the svn repo and the git mirror.

At the svn repo we have:

    @Override
    public void actionPerformed(ActionEvent e) {
        final ChangesetIdQuery dlg = new ChangesetIdQuery();
        if (dlg.showDialog().getValue() != 1) return;
        final Collection<Integer> changesetIds = dlg.getIdsInReverseOrder();
        final RevertType revertType = dlg.getRevertType();
        if (revertType == null) return;

        boolean newLayer = dlg.isNewLayerRequired();
        final boolean autoConfirmDownload = newLayer || changesetIds.size() > 1;
        MainApplication.worker.submit(new RevertChangesetTask(changesetIds, revertType, autoConfirmDownload, newLayer));
    }

But with the git mirror (git clone https://github.com/openstreetmap/josm-plugins.git) we can see a different file:

    @Override
    public void actionPerformed(ActionEvent arg0) {
        final ChangesetIdQuery dlg = new ChangesetIdQuery();
        if (dlg.showDialog().getValue() != 1) return;
        final Collection<Integer> changesetIds = dlg.getIdsInReverseOrder();
        final RevertType revertType = dlg.getRevertType();
        if (revertType == null) return;

        boolean newLayer = dlg.isNewLayerRequired();
        final boolean autoConfirmDownload = newLayer || changesetIds.size() > 1;
        for (Integer changesetId : changesetIds) {
            MainApplication.worker.submit(new RevertChangesetTask(changesetId, revertType, autoConfirmDownload, newLayer));
            newLayer = false; // reuse layer for subsequent reverts
        }
    }

Unsure if this is a different problem (and if it's also affecting other plugins) and if it should be reported as a new ticket.

comment:2 Changed 2 years ago by Don-vip

I think the git mirror is no longer updated for plugins. But with our own migration to git we won't have this problem anymore.

Concerning reverter plugin, it's highly possible I broke something. This plugin code is very complex, I'll try to take a look...

comment:3 Changed 2 years ago by Don-vip

Please tell me which changesets you are currently trying to revert.

comment:4 Changed 2 years ago by naoliv

65592801 65489656 65489618 65489605 65489531 65489380 65488621 65488605 65011889 65011304 65009770 65008915 64991164 64991027 64990962 64990882 64990695

If I try them one by one, things go smoothly; asking to revert all of them then it gets a mess.

comment:5 Changed 2 years ago by Don-vip

That's a huge list, can you check if only two or three are enough to provoke the issue? Complexity of the bug analysis raises with the number of changesets.

comment:6 Changed 2 years ago by naoliv

I will try to find a smaller test case.

comment:7 Changed 2 years ago by naoliv

Is it fine if I create a test case at master.apis.dev.openstreetmap.org? (since objects at the main API are much more dynamic, volatile and subject to be changed)

comment:8 Changed 2 years ago by naoliv

I have created a very simple case where it's possible to see this problem, using the API from master.apis.dev.openstreetmap.org

I have created a note at https://master.apis.dev.openstreetmap.org/changeset/141654 and then deleted it at https://master.apis.dev.openstreetmap.org/changeset/141655

So if we ask to revert both 141655 141654 at the same time the result should be the sum of "undeleting the node" + "removing the node" (ie, we should have no nodes reinserted in OSM)

But the reverter plugin reinserts it when asked to revert both 141655 141654 (as if it was reverting only 141655)

If we ask to first revert only 141655 and then at the same layer revert only 141654, we have the desired result: JOSM won't reinsert the node and will say that there is no changes to be sent.

comment:9 Changed 2 years ago by naoliv

Of course we can have a test at the main API...
Do the same test using 65612097 65612093 at the main API. There is no need to play with the dev one.

comment:10 Changed 2 years ago by Don-vip

Thanks! It will be easier on the dev API, it will give us more time :)

comment:11 Changed 2 years ago by GerdP

In the current code the changeset ids are stored in a TreeMap and no matter in what order you specify them they are always processed starting with the highest (newest) number going to the lowest (oldest). I think that makes sense.
Maybe we should add a hint in the popup to tell the user that the ids are sorted and that it doesn't matter in which order they are given?

comment:12 Changed 20 months ago by naoliv

Cc: Upliner added

Will Upliner get mad if I include him on cc? :-)

Just to summarize the small test case:

This is at the main API server (so there is no need to change anything on JOSM).

If I revert only 65612097 the node gets properly restored.
Then if I revert 65612093, using the previous layer, the node is then deleted.

Revertig separately the two changesets we have no modifications to send to the server (and this is the correct result here).

Now, if I revert both changesets together (no matter the order (65612093 65612097 or 65612097 65612093)), the final result is only a wrongly restored node (as if it was restoring only 65612097).

comment:13 Changed 20 months ago by GerdP

Ah, sorry, seems I didn't understand that the final result is wrong. I thought this ticket is only about the order of the changeset ids. I've traced the code and I think the problem is that we don't exectute the commands created for the first revert before calculating the commands for the next.
I'm now working on a fix. Problem is that we allow to cancel the revert, so we have to make sure that we roll back to the right place...

Changed 20 months ago by GerdP

Attachment: 15906.patch added

Changed 20 months ago by GerdP

Attachment: 15906-v2.patch added

correct handling of conflict warnings

comment:14 Changed 20 months ago by GerdP

Please review v2. The attached patch seems to solve this problem. Maybe there is a better place to handle the undo stack to reduce code duplication?

comment:15 Changed 19 months ago by Don-vip

@Upliner can you please take a look? The patch looks fine but maybe you got a better idea?

comment:16 Changed 18 months ago by GerdP

Summary: Isn't properly reverting multiple changesets[Patch] Isn't properly reverting multiple changesets

I am not sure how well we can trust the undoing of actions, so I'd prefer to have a solution not using it.
My current understanding is that ChangesetReverter should be changed to accept a list of changeset ids. Maybe it could simply treat the content of multiple changesets as a concatenation?

Last edited 18 months ago by GerdP (previous) (diff)

Changed 13 months ago by GerdP

Attachment: 15906-v3.patch added

improved version, undo is only used when really wanted

comment:17 Changed 13 months ago by GerdP

Resolution: fixed
Status: newclosed

see [o35400:35401]

  • execute commands for each reverted changeset
  • change RevertChangesetCommand so that it just collects already executed commands (see #17401)

comment:18 Changed 12 months ago by Don-vip

Regression: #19028

comment:19 Changed 3 months ago by skyper

Regression: #20395

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.