( )⚙ 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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
68–70

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.

69

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

108–128

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

130

Nit: mutable default argument value

175

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.

177

The comment is overstating reality.

238

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.

312–313

The bad API rears its ugly head again :/

323

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

379

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

382–386

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.

392

pycompat.xrange

419

pycompat.xrange.

431

pycompat.xrange

593–604

I think a lot of these want to be private.

623–624

self.ui.debug().

939–940

Maybe this should be controlled with --verbose?

980

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.

1004

Nit: use a tuple

1021–1023

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.

1026

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?

1031–1036

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.