Page MenuHomePhabricator

shelve: merge in obsshelve changes implemented at facebook
AbandonedPublic

Authored by durin42 on Jun 4 2018, 6:16 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

test-obsshelve.t could be merged with test-shelve.t at some point, but
for now I want the comprehensive coverage. Note that obsshelve in
hg-experimental is a fork of shelve, so we're just merging things back
together. The path to graduation from experimental for obsshelve is to
have a better solution around hiding revisions without creating a ton
of markers that'll get exchanged. We've spent a fair amount of time
talking about what that should look like, but in the interim the
experimental-branded obsshelve is a good thing we should have in core.

As a follow-up I'd like to make obsshelve a case of test-shelve.t, but
I thought this would be easier for the initial import.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durin42 created this revision.Jun 4 2018, 6:16 PM

We just sent https://phab.mercurial-scm.org/D3685 that update shelve to not use rebase anymore.

After reading your obsshelve series, it seems that we have some overlap.

We have an implementation of shelve without strip nor obsmarkers.

The first series https://phab.mercurial-scm.org/D3685 is the first half, cleaning up the shelve extensions in order to simplify the code flow and gain access to more data.

The second half implements the strip less version. We wanted to wait until the first half has been merged but we can send the whole series if that can help the discussion. The proposed implementation is based on the internal phase plan.

The cleaning refactoring series seems worthy on its own but will results in conflicts. Do you think it would be possible to rebase your obsshelve serie on top of our refactoring? This way we could have a clean obsshelve code and you should be able to remove some of the hacks in your obsshelve serie.

We have an implementation of shelve without strip nor obsmarkers.

Can we see that now? Otherwise I'm going to lobby to land this in the name of having something done.

We have an implementation of shelve without strip nor obsmarkers.

Can we see that now? Otherwise I'm going to lobby to land this in the name of having something done.

I pushed the full series as RFC on the mailing list (https://www.mercurial-scm.org/pipermail/mercurial-devel/2018-June/117682.html) as I was not sure how phabricator would handle the fact that there is two independent parts + one part that needs the two other. I sent it as linear to more easily see the big picture but the first part of internal phase can be merged independently.

I plan to send the next part of the series after the first part has landed for flow control.

smf added a subscriber: smf.Jun 14 2018, 7:03 PM

durin42 (Augie Fackler) <phabricator@mercurial-scm.org> writes:

durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.
REVISION SUMMARY

test-obsshelve.t could be merged with test-shelve.t at some point, but
for now I want the comprehensive coverage. Note that obsshelve in
hg-experimental is a fork of shelve, so we're just merging things back
together. The path to graduation from experimental for obsshelve is to
have a better solution around hiding revisions without creating a ton
of markers that'll get exchanged. We've spent a fair amount of time
talking about what that should look like, but in the interim the
experimental-branded obsshelve is a good thing we should have in core.
As a follow-up I'd like to make obsshelve a case of test-shelve.t, but
I thought this would be easier for the initial import.

I'm trying to catch up on this discussion, so apologies if this is the
wrong thread to reply to.

I've been mulling on this problem for quite some time now. To clarify,
I'm referring to the problem of hiding / stripping changesets that are
known not to be shared. I keep coming back to the idea of a new phase
because it is so elegant. Looking at the other patch series, I am sad we
didn't implement the phases using the < operator and instead testing
directly for the name of the phase.

Having a "internal-only" phase makes a lot of sense for temporary
commits that only hg is making. In fact, I'd like to suggest taking it
even further: not making obs markers for anything above the draft phase
(i.e. secret, internal, etc.). This would push development of wip
branches to perhaps be secret instead of the default draft.

Glancing at the other discussions in IRC and on the list, it seems we're
mostly agreed on the new phase?

P.S. I think I adjusted my email filters to put phabricator emails I'm
tagged in directly into my inbox; so replying to this message would help
test that (even if just to say, "we should this discussion to another
thread")

durin42 abandoned this revision.Nov 11 2018, 2:26 PM