Page MenuHomePhabricator

rebase: change internal format to support destination map
ClosedPublic

Authored by quark on Aug 11 2017, 5:01 AM.

Details

Summary

A later patch will add multiple destination support. This patch changes
internal state and the rebase state file format to support that. But the
external interface still only supports single destination.

A test was added to make sure rebase still supports legacy state file.

The new state file is incompatible with old clients. We had done similar
state file format change before: 5eac7ab, 92409f8, and 72412af. The state
file is transient, so the impact of incompatibility is limited. Besides,
the old client won't support multiple destinations anyway so it does not
really make sense to make the file format compatible with them.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

quark created this revision.Aug 11 2017, 5:01 AM
quark updated this revision to Diff 836.Aug 13 2017, 12:29 AM
quark updated this revision to Diff 900.Aug 14 2017, 8:12 PM
martinvonz added inline comments.
hgext/rebase.py
191–193

I suppose this was here to allow older versions to write a state file written by a newer client. What will happen after this patch if an older version reads the state file?

For reference: https://patchwork.mercurial-scm.org/patch/6959/

quark added inline comments.Aug 29 2017, 6:05 PM
hgext/rebase.py
191–193

I think you mean "read" instead of "write".

The old client will crash reading the state file. Even if they do not crash, they cannot figure out what to do correctly because they don't have multi-dest support. So I don't think any attempt to make the format compatible with old client is worthwhile.

Looking at 5eac7ab, 92409f8, 72412af, the file format has been changed a few times in recent years. So I disagree with Mads Kiilerich and don't think this patch needs change.

If you think perfect compatibility (sane error message) in both directions is a must have, I can add a repo requirement.

martinvonz added inline comments.Aug 29 2017, 6:13 PM
hgext/rebase.py
191–193

I also don't think it's reasonable to make it possible for old versions to continue the rebase, but I'd like them to at least be able to run "hg rebase --abort". Could you make sure that's possible?

martinvonz added inline comments.Aug 29 2017, 6:18 PM
hgext/rebase.py
191–193

OTOH, at least Durham's 72412afe4c28 seems like it would have broken it similarly, and I don't remember hearing any complaints back then, so maybe not a problem in practice. So do as you feel like, I don't care :-)

quark edited the summary of this revision. (Show Details)Aug 29 2017, 9:40 PM
quark updated this revision to Diff 1417.
This revision was automatically updated to reflect the committed changes.