[Patch] Isn't properly reverting multiple changesets

[naoliv]

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)

[naoliv]

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.

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

[Don-vip]

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

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

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

[naoliv]

I will try to find a smaller test case.

[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)

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

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

[Don-vip]

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

[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?

[naoliv]

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

Just to summarize the small test case:

• created a node on changeset ​65612093
• deleted this same node on changeset ​65612097

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

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

[GerdP]

correct handling of conflict warnings

[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?

[Don-vip]

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

[GerdP]

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?

[GerdP]

improved version, undo is only used when really wanted

[GerdP]

see [o35400:35401]

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

[Don-vip]

Regression: #19028

[skyper]

Regression: #20395