Patchwork D6440: fix: let fixer tools inherit hg's cwd so they can look at the working copy

login
register
mail settings
Submitter phabricator
Date May 23, 2019, midnight
Message ID <differential-rev-PHID-DREV-7livqhd3zqzim3cxhh3b-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/40204/
State New
Headers show

Comments

phabricator - May 23, 2019, midnight
hooper created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This lets fixer tools do things like find configuration files, with the caveat
  that they'll only see the version of that file in the working copy, regardless
  of what revisions are being fixed.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/fix.py
  tests/test-fix.t

CHANGE DETAILS




To: hooper, #hg-reviewers
Cc: mercurial-devel
phabricator - May 23, 2019, 1:32 p.m.
durin42 added inline comments.

INLINE COMMENTS

> fix.py:106
> +
> +Fixer tools are run the in same working directory as the :hg:`fix` command. This
> +allows them to read configuration files from the working copy, or even write to

I wonder if it'd be more generally useful to run the fixer in the repo root than the same `pwd` as the command invocation. WDYT?

REPOSITORY
  rHG Mercurial

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

To: hooper, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - May 24, 2019, 1:02 a.m.
hooper added inline comments.

INLINE COMMENTS

> durin42 wrote in fix.py:106
> I wonder if it'd be more generally useful to run the fixer in the repo root than the same `pwd` as the command invocation. WDYT?

It depends on whether we want to make it easier for tools that want to examine the tree upward from the cwd or tools that want to examine the tree downward from the root. I suspect that "upward" will be more important because of the way some tools use config files that apply to subtrees. For some tools it's weird because reading code to format from stdin is often an afterthought. Either way, for completeness, the root and cwd would both need to be available in either subprocess cwd or the command template. There may also be some room to prefer configs like "cd {root} && mycommand".

REPOSITORY
  rHG Mercurial

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

To: hooper, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - May 24, 2019, 1:15 p.m.
durin42 added inline comments.

INLINE COMMENTS

> hooper wrote in fix.py:106
> It depends on whether we want to make it easier for tools that want to examine the tree upward from the cwd or tools that want to examine the tree downward from the root. I suspect that "upward" will be more important because of the way some tools use config files that apply to subtrees. For some tools it's weird because reading code to format from stdin is often an afterthought. Either way, for completeness, the root and cwd would both need to be available in either subprocess cwd or the command template. There may also be some room to prefer configs like "cd {root} && mycommand".

Hm. Is {root} in the templater for the command? Maybe have a sample in the test demonstrating that?

I'm still dubious that cwd is the right thing, simply because it's unpredictable. I could see $(dirname filepath) or repo root as consistent, but cwd feels off to me somehow. I don't feel strongly though, so as long as {root} exists I can get what I need for hg's fixer definitions...

REPOSITORY
  rHG Mercurial

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

To: hooper, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - May 31, 2019, 5:01 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> durin42 wrote in fix.py:106
> Hm. Is {root} in the templater for the command? Maybe have a sample in the test demonstrating that?
> 
> I'm still dubious that cwd is the right thing, simply because it's unpredictable. I could see $(dirname filepath) or repo root as consistent, but cwd feels off to me somehow. I don't feel strongly though, so as long as {root} exists I can get what I need for hg's fixer definitions...

FWIW, I agree that running using cwd-relative path feels wrong.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/tests/test-fix.t b/tests/test-fix.t
--- a/tests/test-fix.t
+++ b/tests/test-fix.t
@@ -1264,3 +1264,37 @@ 
 
   $ cd ..
 
+We run fixer tools in the cwd so they can look for config files or other
+important things in the working directory. This does NOT mean we are
+reconstructing a working copy of every revision being fixed; we're just giving
+the tool knowledge of the cwd in case it can do something reasonable with that.
+
+  $ hg init subprocesscwd
+  $ cd subprocesscwd
+
+  $ cat >> .hg/hgrc <<EOF
+  > [fix]
+  > printcwd:command = pwd
+  > printcwd:pattern = path:foo/bar
+  > EOF
+
+  $ mkdir foo
+  $ printf "bar\n" > foo/bar
+  $ hg commit -Aqm blah
+
+  $ hg fix -w -r . foo/bar
+  $ hg cat -r tip foo/bar
+  $TESTTMP/subprocesscwd
+  $ cat foo/bar
+  $TESTTMP/subprocesscwd
+
+  $ cd foo
+
+  $ hg fix -w -r . bar
+  $ hg cat -r tip bar
+  $TESTTMP/subprocesscwd/foo
+  $ cat bar
+  $TESTTMP/subprocesscwd/foo
+
+  $ cd ../..
+
diff --git a/hgext/fix.py b/hgext/fix.py
--- a/hgext/fix.py
+++ b/hgext/fix.py
@@ -102,6 +102,13 @@ 
     mapping fixer tool names to lists of metadata values returned from
     executions that modified a file. This aggregates the same metadata
     previously passed to the "postfixfile" hook.
+
+Fixer tools are run the in same working directory as the :hg:`fix` command. This
+allows them to read configuration files from the working copy, or even write to
+the working copy. The working copy is not updated to match the revision being
+fixed. In fact, several revisions may be fixed in parallel. Writes to the
+working copy are not amended into the revision being fixed; fixer tools should
+always write fixed file content back to stdout as documented above.
 """
 
 from __future__ import absolute_import
@@ -233,7 +240,7 @@ 
             for rev, path in items:
                 ctx = repo[rev]
                 olddata = ctx[path].data()
-                metadata, newdata = fixfile(ui, opts, fixers, ctx, path,
+                metadata, newdata = fixfile(ui, repo, opts, fixers, ctx, path,
                                             basectxs[rev])
                 # Don't waste memory/time passing unchanged content back, but
                 # produce one result per item either way.
@@ -530,16 +537,17 @@ 
                 basectxs[rev].add(pctx)
     return basectxs
 
-def fixfile(ui, opts, fixers, fixctx, path, basectxs):
+def fixfile(ui, repo, opts, fixers, fixctx, path, basectxs):
     """Run any configured fixers that should affect the file in this context
 
     Returns the file content that results from applying the fixers in some order
     starting with the file's content in the fixctx. Fixers that support line
     ranges will affect lines that have changed relative to any of the basectxs
     (i.e. they will only avoid lines that are common to all basectxs).
 
     A fixer tool's stdout will become the file's new content if and only if it
-    exits with code zero.
+    exits with code zero. The fixer tool's working directory is inherited from
+    this process.
     """
     metadata = {}
     newdata = fixctx[path].data()
@@ -553,7 +561,7 @@ 
             proc = subprocess.Popen(
                 procutil.tonativestr(command),
                 shell=True,
-                cwd=procutil.tonativestr(b'/'),
+                cwd=None,
                 stdin=subprocess.PIPE,
                 stdout=subprocess.PIPE,
                 stderr=subprocess.PIPE)