Page MenuHomePhabricator

bundle: add the possibility to bundle bookmarks (issue5792)
Needs RevisionPublic

Authored by lothiraldan on Mar 16 2018, 9:21 AM.

Details

Reviewers
indygreg
baymax
Group Reviewers
hg-reviewers
Summary

Also take the wlock when unbundling. It shouldn't have a big impact on
performance.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

lothiraldan created this revision.Mar 16 2018, 9:21 AM
pulkit added a subscriber: pulkit.Mar 16 2018, 9:27 AM
pulkit added inline comments.
mercurial/configitems.py
422

Any reason why we have all these config option as experimental? We talked at sprint about moving things out of experimental and we agreed on experimental.bundle-phases. This one sounds similar.

lothiraldan added inline comments.
mercurial/configitems.py
422

I was afraid we might have to delete this option once we have the discussion about bundle spec format with @indygreg. If that would be the case, I would feel more comfortable deleting an experimental option.

If we agreed on moving bundle-phases out of experimental, I guess we can do the same for bundle-bookmarks.

@indygreg Do you think we would still have this option around once we have new-style bundle spec?

martinvonz added inline comments.
tests/test-bundle-bookmarks.t
38–64

Can we have a similar test case where we create divergence? Create a fork in the graph in the debugdrawdag call above. Let's say you have commit F that branches off of B, then do something like this:

$ hg bundle --all bundle
$ hg strip --no-backup C
$ hg bookmarks -f -r F D1
$ hg unbundle -q bundle
$ hg log -G -T '{desc} {bookmarks}\n'
  o  E
  |
  o  D D1 D2
  |
  o  C
  |
  |  o F D1@1
  |/
  o  B
  |
  o  A A1

I don't know if "@1" is the appropriate divergence marker, but I can't think of a better one. I haven't even looked at your code to try to guess what it would be in practice (if it would work at all).

indygreg requested changes to this revision.Apr 12 2018, 12:16 PM

What's the status of this patch? Is it still reviewable? If so, let's get a rebased version submitted, just in case things have changed.

This revision now requires changes to proceed.Apr 12 2018, 12:16 PM

What's the status of this patch? Is it still reviewable? If so, let's get a rebased version submitted, just in case things have changed.

I need to update the patch according to @martinvoz comment.

I was also waiting on your comment on https://phab.mercurial-scm.org/D2880#46263, do you think we will keep the option once the bundlespec system is revamped? If yes, we can make the config option a non-experimental one. But if it will be dropped soon, I think keeping it experimental until it's dropped is preferable. What do you think?

lothiraldan updated this revision to Diff 8263.Apr 14 2018, 5:38 AM
lothiraldan added inline comments.Apr 14 2018, 5:40 AM
tests/test-bundle-bookmarks.t
38–64

I have added such test and indeed this case is not handled right now. I'm not aware of how Mercurial handle this case when exchanging bookmarks, I will need to take a look at the code to find out.

What is the correct behavior here? Change the existing bookmark or the bookmark from the bundle?

martinvonz added inline comments.Apr 14 2018, 12:41 PM
tests/test-bundle-bookmarks.t
38–64

I think it should be the same thing as pulling from another repo. Try putting the same changes in another repo and pulling from it without giving the path a name (nothing in [paths] config). I haven't checked what suffix will be used on the divergent bookmark, but it probably makes sense to use the same here (perhaps it's just going to be "D1@1" as I guessed before).

lothiraldan added inline comments.Apr 17 2018, 3:41 AM
tests/test-bundle-bookmarks.t
38–64

I tried adding such scenarios but failed to produce a divergence scenario, no idea why.

I don't think I will have time to debug the issue before the freeze, so let's skip this changeset for 4.6

Just a reminder that this has not been queued yet.

baymax requested changes to this revision.Jan 24 2020, 12:32 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jan 24 2020, 12:32 PM