Page MenuHomePhabricator

phabricator: support automatically obsoleting old revisions of pulled commits
Needs ReviewPublic

Authored by mharbison72 on Sep 25 2019, 1:17 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

This is basically an import of the pullcreatemarkers extension[1] from the FB
repo, with minor adjustments to getmatchingdiff() to work with modern hg.
Since this is very phabricator specific, it makes more sense to me to bundle it
into the existing extension. It wasn't very obvious from the old name what
functionality was provided, and it may make sense to do this in other scenarios
besides hg pull.

There are two use cases that I can see- first, ensuring that old revisions are
cleaned up for a contributor (I seem to recall something I submitted recently
needed to be explicitly pruned, though most submissions do clean up
automatically). Second, any hg phabread | hg import - would otherwise need to
be manually cleaned up. The latter is annoying enough that I tend not to grab
the code and try it when reviewing.

It is currently guarded by a config option (off by default), because @marmoute
expressed concerns about duplicate marker creation if the pushing reviewer also
creates a marker. I don't think that's possible here, since the obsolete
revisions are explicitly excluded. But maybe there are other reasons someone
wouldn't want older revisions obsoleted. The config name reflects the fact that
I'm not sure if other things like import should get this too.

I suspect that we could wrap a function deeper in the pull sequence to improve
both the code and the UX. For example, when pulling an obsolete marker, it can
print out a warning that the working directory parent is obsolete, but that
doesn't happen here. (It won't happen with this test. It *should* without the
--bypass option, but doesn't.) It should also be possible to not have to
query the range of new revisions, and maybe it can be added to the existing
transaction.

[1] https://bitbucket.org/facebook/hg-experimental/src/default/hgext3rd/pullcreatemarkers.py

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mharbison72 created this revision.Sep 25 2019, 1:17 AM

@sheehan this might be of interest to you.

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.

hgext/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)

192

It would probably make sense to wrap the pull exchange logic instead. It would catch more instance and could reuse the same transaction.

216

This should be non public() instead of draft() these could be draft.

225

We should do the computation before locking to avoid other process updating the repo under our feet.

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.

hgext/phabricator.py
187

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?

192

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

216

Agreed.

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

225

You mean after locking, correct?

marmoute added inline comments.Oct 8 2019, 6:17 AM
hgext/phabricator.py
187

User A phasend a patch

  • User A phasend a changeset A as D1337
  • User B phabimport D1337, rebase (as A') and pushes
  • User A fix critical bug in D1337 and amend it as A''
  • User A pull the old version of D1337 without the fix.

If we obsolete the new version of D1337 (A''), we are silently dropping the fix to the critical bug.

If markers were properly exchange, the evolution machinery would detect the divergence, warn the user and be able to automatically solve it. Preserving the fix to the criticial bug.

216

Thing could be in secret phase too.

225

I do mean after locking (sorry)

mharbison72 added inline comments.Oct 8 2019, 8:12 AM
hgext/phabricator.py
187

I see. So maybe this should only obsolete things that have no precursors, and warn (and ignore) things that do. That means it doesn’t clean up if the reviewer accidentally forgets to import with —obsolete, but that’s easy enough to fix with a copy/paste command if A and A' are identified in the output.

216

Right, and this is probably the typical case. (The phabimport alias I have imports as secret to prevent accidental push.) But secret is visible to the user, and benefits from the cleanup. Archived isn’t visible (IIUC), so it probably doesn’t need to be obsoleted too- especially if we’re worried about marker growth.

(I’m also not sure how well thg handles archived, and that probably needs to be squared away first if we opt to archive instead of obsolete here.)