Patchwork D5516: fix: add some arguments to facilitate extensions

login
register
mail settings
Submitter phabricator
Date Jan. 7, 2019, 11:04 p.m.
Message ID <differential-rev-PHID-DREV-lnrmsvfumd7gnwchnyoe-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/37544/
State Superseded
Headers show

Comments

phabricator - Jan. 7, 2019, 11:04 p.m.
hooper created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  For example, these additions allow wrapping `hg fix` to do things like:
  
  - Telling fixer tools what commit a file belongs to
  - Cleaning up temporary files created by Fixer objects
  
  There are no intended functional changes.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/fix.py

CHANGE DETAILS




To: hooper, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 7, 2019, 11:54 p.m.
durin42 added a comment.


  I'm -0 on this: what's special about this functionality that it needs to be an extension of an extension instead of something that can be done in core?

REPOSITORY
  rHG Mercurial

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

To: hooper, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - Jan. 8, 2019, 12:12 a.m.
hooper added a comment.


  In https://phab.mercurial-scm.org/D5516#81679, @durin42 wrote:
  
  > I'm -0 on this: what's special about this functionality that it needs to be an extension of an extension instead of something that can be done in core?
  
  
  Google's internal use of this extension makes some customizations that I doubt would be appealing in core. This patch makes some of it simpler to implement. Making all of it possible through configs/templates might be unduly complex.
  
  A better version of this might put "ctx" into the templater inside Fixer.command, and maybe add a no-op Fixer.cleanup method to make that part more explicit. I think that would be a less trivial super set of this patch.

REPOSITORY
  rHG Mercurial

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

To: hooper, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - Jan. 9, 2019, 7:42 p.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D5516#81683, @hooper wrote:
  
  > In https://phab.mercurial-scm.org/D5516#81679, @durin42 wrote:
  >
  > > I'm -0 on this: what's special about this functionality that it needs to be an extension of an extension instead of something that can be done in core?
  >
  >
  > Google's internal use of this extension makes some customizations that I doubt would be appealing in core. This patch makes some of it simpler to implement. Making all of it possible through configs/templates might be unduly complex.
  
  
  What kinds of customizations?
  
  > A better version of this might put "ctx" into the templater inside Fixer.command, and maybe add a no-op Fixer.cleanup method to make that part more explicit. I think that would be a less trivial super set of this patch.

REPOSITORY
  rHG Mercurial

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

To: hooper, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - Jan. 9, 2019, 8:15 p.m.
hooper added a comment.


  In https://phab.mercurial-scm.org/D5516#81841, @durin42 wrote:
  
  > In https://phab.mercurial-scm.org/D5516#81683, @hooper wrote:
  >
  > > In https://phab.mercurial-scm.org/D5516#81679, @durin42 wrote:
  > >
  > > > I'm -0 on this: what's special about this functionality that it needs to be an extension of an extension instead of something that can be done in core?
  > >
  > >
  > > Google's internal use of this extension makes some customizations that I doubt would be appealing in core. This patch makes some of it simpler to implement. Making all of it possible through configs/templates might be unduly complex.
  >
  >
  > What kinds of customizations?
  
  
  One is to aggregate metadata output from multiple fixer tool executions to display a summary at the end (so wrapping cleanup() is sensible). Not sure who else would use that, or if there's a good way to make a generic interface for it.
  
  Another is basically to add a "--nodeid_for_this_file=deadbeef" to a fixer tool command line. That's where it would be sufficient to have the ctx available. It might be nice to put the ctx into the templater, but that raises some questions about implementation that I wanted to punt for now.
  
  >> A better version of this might put "ctx" into the templater inside Fixer.command, and maybe add a no-op Fixer.cleanup method to make that part more explicit. I think that would be a less trivial super set of this patch.

REPOSITORY
  rHG Mercurial

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

To: hooper, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - March 22, 2019, 2:58 a.m.
hooper abandoned this revision.
hooper added a comment.


  See https://phab.mercurial-scm.org/D6167 for a newer approach.

REPOSITORY
  rHG Mercurial

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

To: hooper, #hg-reviewers
Cc: durin42, mercurial-devel

Patch

diff --git a/hgext/fix.py b/hgext/fix.py
--- a/hgext/fix.py
+++ b/hgext/fix.py
@@ -239,9 +239,11 @@ 
                         replacerev(ui, repo, ctx, filedata[rev], replacements)
                     del filedata[rev]
 
-        cleanup(repo, replacements, wdirwritten)
+        cleanup(repo, replacements, wdirwritten, fixers)
 
-def cleanup(repo, replacements, wdirwritten):
+# Do not inline this function or remove unused arguments. They are meant as a
+# wrapping point for further extensions.
+def cleanup(repo, replacements, wdirwritten, fixers):
     """Calls scmutil.cleanupnodes() with the given replacements.
 
     "replacements" is a dict from nodeid to nodeid, with one key and one value
@@ -251,6 +253,10 @@ 
     "wdirwritten" is a bool which tells whether the working copy was affected by
     fixing, since it has no entry in "replacements".
 
+    "fixers" is the list of fixer tools that was used, in case we want to clean
+    up something they dirtied (though they currently have no side effects within
+    this extension).
+
     Useful as a hook point for extending "hg fix" with output summarizing the
     effects of the command, though we choose not to output anything here.
     """
@@ -497,7 +503,7 @@ 
     for fixername, fixer in fixers.iteritems():
         if fixer.affects(opts, fixctx, path):
             rangesfn = lambda: lineranges(opts, path, basectxs, fixctx, newdata)
-            command = fixer.command(ui, path, rangesfn)
+            command = fixer.command(ui, fixctx, path, rangesfn)
             if command is None:
                 continue
             ui.debug('subprocess: %s\n' % (command,))
@@ -652,6 +658,7 @@ 
             setattr(fixers[name], pycompat.sysstr('_' + key),
                     attrs.get(key, default))
         fixers[name]._priority = int(fixers[name]._priority)
+        fixers[name].name = name
     return collections.OrderedDict(
         sorted(fixers.items(), key=lambda item: item[1]._priority,
                reverse=True))
@@ -671,11 +678,13 @@ 
         """Should this fixer run on the file at the given path and context?"""
         return scmutil.match(fixctx, [self._pattern], opts)(path)
 
-    def command(self, ui, path, rangesfn):
+    def command(self, ui, ctx, path, rangesfn):
         """A shell command to use to invoke this fixer on the given file/lines
 
         May return None if there is no appropriate command to run for the given
         parameters.
+
+        "ctx" is unused, but it is meant for use by further extensions.
         """
         expand = cmdutil.rendercommandtemplate
         parts = [expand(ui, self._command,