Page MenuHomePhabricator

changegroupv4: add sidedata helpers
ClosedPublic

Authored by Alphare on Feb 19 2021, 6:16 AM.

Details

Summary

These helpers carry the information and computers needed to rewrite sidedata
when generating/applying patches. We will be making use of them soon.

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

Alphare created this revision.Feb 19 2021, 6:16 AM
baymax updated this revision to Diff 25996.Mar 1 2021, 11:52 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

marmoute requested changes to this revision.Mar 2 2021, 2:01 PM
marmoute added a subscriber: marmoute.

You should document the new argument in the existing docstring of the various function you updated. This would help to understand what this is about.

This revision now requires changes to proceed.Mar 2 2021, 2:01 PM
Alphare updated this revision to Diff 26092.Mar 4 2021, 10:18 AM
marmoute requested changes to this revision.Mar 9 2021, 11:45 AM
marmoute added inline comments.
mercurial/changegroup.py
1000–1001

A better thing would be for the strip case to properly setup the bundle call to make sure the sidedata are removed.

The current code seems okay for now, but adding a clear comment about how we should do that ideally would be great.

mercurial/utils/storageutil.py
515

This will need some doc about what is the semantic of this function.

This revision now requires changes to proceed.Mar 9 2021, 11:45 AM
Alphare marked an inline comment as done.Mar 10 2021, 1:43 PM
Alphare added inline comments.
mercurial/changegroup.py
1000–1001

But we don't want to remove the sidedata, we want to leave them there. When restoring a strip bundle, we want the sidedata to be the same as before, so we might as well not drop them only to re-generate them.

Alphare updated this revision to Diff 26223.Mar 10 2021, 2:12 PM
marmoute added inline comments.Mar 11 2021, 1:16 PM
mercurial/changegroup.py
1000–1001

My point is that we should (eventually) have the proper way to declare the wanted sidedata when we generate a bundle, and the higher level strip code will set this properly before genering the backup bundle.

Alphare marked an inline comment as done.Mar 12 2021, 5:35 AM
Alphare updated this revision to Diff 26257.Mar 12 2021, 6:34 AM
marmoute accepted this revision.Mar 12 2021, 1:10 PM

Out of curiosity, why not put this into changegroupv3 (since it's still experimental)? Should it no longer be marked experimental? It's one of several things keeping LFS experimental.

Out of curiosity, why not put this into changegroupv3 (since it's still experimental)? Should it no longer be marked experimental? It's one of several things keeping LFS experimental.

My feeling is that they are enough usage of cg3 in the wild (lfs, narrow) that making a new cg3 iteration incompatible with the previous one would break some people. However you probably have a better image of those people than I do.

Out of curiosity, why not put this into changegroupv3 (since it's still experimental)? Should it no longer be marked experimental? It's one of several things keeping LFS experimental.

My feeling is that they are enough usage of cg3 in the wild (lfs, narrow) that making a new cg3 iteration incompatible with the previous one would break some people. However you probably have a better image of those people than I do.

Honestly I don't. I'm totally fine with starting v4, I just don't want v3 to be forever experimental if everyone starts focusing on v4.

Alphare updated this revision to Diff 26334.Mar 15 2021, 6:24 AM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.