( )⚙ D3991 absorb: import extension from Facebook's hg-experimental

This is an archive of the discontinued Mercurial Phabricator instance.

absorb: import extension from Facebook's hg-experimental
ClosedPublic

Authored by durin42 on Aug 1 2018, 11:17 AM.

Details

Summary

absorb is a wicked-fast command to use blame information to
automatically amend edits to the correct draft revision. Originally
written by Jun Wu, this import is hgext3rd/absorb/__init__.py with:

  • the testedwith value changed
  • the linelog import updated
  • some missing configitems registered
  • some imports reordered per check-code.py
  • some missing future imports added per check-code.py

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durin42 created this revision.Aug 1 2018, 11:17 AM

I only reviewed about half of this. I'm inclined to take it as-is or close to as-is. But I would like to have a handle of the follow-ups before we forget about them.

hgext/absorb.py
67–69

I'd like to see these moved to [commands] and to use hyphens in the names for readability. Could be deferred to a follow-up to minimize import churn. Worth adding an inline TODO/FUTURE to track though.

68

Do we even need this? Can't [alias] do this?

107–127

I'm pretty sure we have a variant of this code somewhere in core that could be reused.

129

Nit: mutable default argument value

174

Nit: the p1 is not necessarily public/immutable from the perspective of the store (because of stack limits). But it should be immutable from the perspective of the absorb operation.

176

The comment is overstating reality.

237

I was initially worried about this because it returns a reference. But it looks like committablectx.__init__ makes a copy of the dict to avoid surprises. So this is fine.

311–312

The bad API rears its ugly head again :/

322

TIL that Python allows 0 + True and 0 + False. Sadness.

378

Using a set comprehension will avoid having to construct an intermediate list.

381–385

We could do without the cast to a list and do a set.pop() to return its single element. I guess it is wonky either way. But seeing the list(set(...)) followed by a [0] access unsettled me. It's all good because of the len(involvedrevs) == 1 check though.

391

pycompat.xrange

418

pycompat.xrange.

430

pycompat.xrange

592–603

I think a lot of these want to be private.

622–623

self.ui.debug().

938–939

Maybe this should be controlled with --verbose?

979

I'd like to think we could delete this code and leave it up to these extensions to do their own monkeypatching.

Actually, this whole advanced wrapping of amend feels very hacky. We should definitely get this logic moved to the actual definition of the amend command since it is part of this repo.

1003

Nit: use a tuple

1020–1022

The status object is a named tuple of sorts. I'd prefer to see us access the named attributes instead of the integer fields. (And I wish we could deprecate accessing by index because it is hard to read.) Suitable as a follow-up.

1025

I don't believe the string ex. appears in the UI today. We should tweak this string.

Also, this logic is action at a distance and is making inferences. Perhaps we should actually test for the underlying condition and display a more accurate error message?

1030–1035

This wants to be templatized. Follow-up territory.

indygreg accepted this revision.Aug 6 2018, 11:53 AM

I'm fine taking this as-is.

This revision is now accepted and ready to land.Aug 6 2018, 11:53 AM
This revision was automatically updated to reflect the committed changes.

absorb.py:67
+
+configitem('absorb', 'addnoise', default=True)
+configitem('absorb', 'amendflag', default=None)

https://www.mercurial-scm.org/wiki/UIGuideline#adding_new_options

Please feel encouraged to send cleanups to absorb - the initial import was kept as pristine as possible to make history clearer.