D6876: phabricator: support automatically obsoleting old revisions of pulled commits

mharbison72 (Matt Harbison) phabricator at mercurial-scm.org
Tue Oct 8 00:20:37 EDT 2019


mharbison72 added a comment.


  In D6876#102925 <https://phab.mercurial-scm.org/D6876#102925>, @marmoute wrote:
  
  > The feature seens pretty usful, but is also a potential foot-gun/data-loss engine. I think it is useful to take the feature, but maybe with proper documentaiton warning and turned of by efaut. I made a couple of comment about the implementation.
  
  I basically agree with the inline comments.  This was an import with minimal changes as a baseline, and I didn't want to spend time making a bunch of follow ups until I gauged interest in this.
  
  Alternately, if there's concern over obsolete marker growth, should this use the archived phase instead?  Since the use case is mostly around stuff a developer imports locally, I don't see much value in those markers being exchangeable.  The only potential hole I see is if someone has pushed this to a non publishing repo before pulling.  Then everyone else could pull the (not obsoleted) commit.  But then //they// would archive it on pull too, instead of N developers potentially creating N markers for the same commit.

INLINE COMMENTS

> marmoute wrote in phabricator.py:187
> So, this only checks the diff number, right ? So in theory, we could obsolete a later version of this, silently dropping change (instead of detecting potential phase-divergence)

It looks like, based on the loop where it's used, it will obsolete //all// (non public) commits that match.  The only way I can think of where that happens is if I `phabimport` someone else's patches, they make updates, and then I `phabimport` again.  That's why I was wondering aloud about doing this on import, but if the eventual pull cleans it all up, that may be sufficient for the use case I had in mind.  I don't see how a developer would do that on their own commits with amend and friends.

Do you have any ideas to detect phase divergence?  And can that even happen if we're ignoring public commits?

> marmoute wrote in phabricator.py:192
> It would probably make sense to wrap the pull exchange logic instead. It would catch more instance and could reuse the same transaction.

I wonder if it would be better in some txn hook.  And then it could (I think) share the existing transaction and lock.

> marmoute wrote in phabricator.py:216
> This should be `non public()` instead of `draft()` these could be draft.

Agreed.

I saw mention of the archived phase the other day; how would that (and other special phases) play here?

> marmoute wrote in phabricator.py:225
> We should do the computation before locking to avoid other process updating the repo under our feet.

You mean //after// locking, correct?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6876/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6876

To: mharbison72, #hg-reviewers
Cc: pulkit, sheehan, Kwan, mercurial-devel, marmoute


More information about the Mercurial-devel mailing list