[PATCH accept-scripts] land: fix bug we see when we merge, rather than rebase

[Kevin Bullock]

> On Apr 26, 2018, at 10:59, Augie Fackler <raf at durin42.com> wrote:
> 
> # HG changeset patch
> # User Augie Fackler <raf at durin42.com>
> # Date 1524713796 14400
> #      Wed Apr 25 23:36:36 2018 -0400
> # Node ID c8dd55d63f9e56d62db3ff1e1b0a8fef228fd19f
> # Parent  66e6638d2a6e78f3af813a274eadbd90679b888e
> land: fix bug we see when we merge, rather than rebase
> 
> I triggered this when I tagged 4.6rc1 concurrent with 8c8b6d13949a
> being landed and accepted. That change was only a doc change, so it
> wasn't worth redoing the RC, but then the tagging revision got stuck
> because it wasn't a child of a head in the repo.
> 
> Unfortunately, I couldn't figure out how to fix this bug without what
> amounts to a ground-up rewrite of the land script. Sigh.

Thanks for poking at this. Code LGTM but I'm getting weird test failures:

---------- test output
--- /Users/kbullock/Source/Projects/hg/accept/tests/test-land.t
+++ /Users/kbullock/Source/Projects/hg/accept/tests/test-land.t.err
@@ -23,15 +23,8 @@
 More complicated example built using debugbuilddag. This emulates a bug
 encountered during the hg 4.6 freeze.
   >>> import os
-  >>> with open(os.environ['HG_ACCEPT_CONFIG']) as f:
-  ...   cfg = f.read()
-  >>> cfglines = cfg.splitlines()
-  >>> cfglines[-2] = cfglines[-2][:-len('source')] + 'complexsrc'
-  >>> cfglines[-1] = cfglines[-1][:-len('target')] + 'complextgt'
-  >>> with open(os.environ['HG_ACCEPT_CONFIG'], 'w') as f:
-  ...   f.write('\n'.join(cfglines))
-
-  $ hg init complexsrc
+  /usr/bin/python: No module named heredoctest
+  [1]
   $ hg init complextgt
   $ cd complexsrc
   $ hg debugbuilddag '. @stable +2 :pubhead <2 +2 :daggyfix /3 +1 :accepted +3'
@@ -81,20 +74,26 @@

 We expect to get the hash of 7 as blocker
   $ blocker
-  38e0ccfa60f109f813eba32713eada1543664dbf alice
+  ff05b7beb4519606efcf1cf8ccf7964280e566e9 alice
 We expect to land 3::6.
   $ land
+  hg: parse error: empty query
+  Traceback (most recent call last):
+    File "/Users/kbullock/Source/Projects/hg/accept/tests/../land", line 50, in <module>
+      conf.source, ' + '.join(all_accepted))))
+    File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/__init__.py", line 290, in load
+      **kw)
+    File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/__init__.py", line 338, in loads
+      return _default_decoder.decode(s)
+    File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/decoder.py", line 366, in decode
+      obj, end = self.raw_decode(s, idx=_w(s, 0).end())
+    File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/decoder.py", line 384, in raw_decode
+      raise ValueError("No JSON object could be decoded")
+  ValueError: No JSON object could be decoded
+  [1]
   $ hg log -G -T'{rev}:{node|shortest} {branch} {tags}\n'
-  o  6:ee2d stable tip
+  o  2:f3cf stable tip
   |
-  o    5:4721 stable
-  |\
-  | o  4:6cfb stable
-  | |
-  | o  3:0ccc stable
-  | |
-  o |  2:f3cf stable
-  |/
   o  1:a079 stable
   |
   o  0:1ea7 default

ERROR: test-land.t output changed
!
Failed test-land.t: output changed
# Ran 1 tests, 0 skipped, 1 failed.
python hash seed: 3774207118
---------- /test output

Also some nitpicks/suggestions, none blocking.

> @@ -32,29 +35,64 @@ for l in os.popen("hg log -R %s -r 'defa
> 
> take = []
> takelog = ""
> -while True:
> -    hit = False
> -    for n in cq.draft():
> -        a = cq.accepted(n)
> -        if not a:
> -            logging.debug("%s not accepted, skipping", n)
> -            continue
> +
> +def _destcontains(node):
> +    try:
> +        return subprocess.check_output(
> +            ['hg', 'log', '-R', dest, '-r', node, '-T.'],
> +            stderr=subprocess.STDOUT) == '.'

Would be nice if this took a list of nodes.

> +    except subprocess.CalledProcessError:
> +        return False
> +
> +# Find all accepted revisions
> +all_accepted = [n for n in cq.draft() if cq.accepted(n)]
> +info = json.load(os.popen("hg log -R %s -r '%s' -Tjson" % (
> +    conf.source, ' + '.join(all_accepted))))

You're using ' + '.join(all_accepted) enough times that it probably merits a name. Maybe also a helper function for doing 'hg log -R {conf.source} -r {revset} -T {template}'.

> +# dict of node: its parents.
> +parents = {i['node']: i['parents'] for i in info}
> 
> -        if cq.parent(n) not in heads:
> -            logging.debug("last public ancestor of %s not in %s, skipping",
> -                          n, dest)
> -            continue
> +# Identify heads and roots of accepted revision ranges
> +accepted_roots = {r.strip() for r in os.popen(
> +    "hg log -R %s -r 'roots(%s)' -T'{node}\\n'" % (
> +        conf.source, ' + '.join(all_accepted)))}
> +accepted_heads = [h.strip() for h in os.popen(
> +    "hg log -R %s -r 'heads(%s)' -T'{node}\\n'" % (
> +        conf.source, ' + '.join(all_accepted)))]
> +# Condense accepted revisions down into accepted ranges
> +range_ = collections.namedtuple('range', 'roots head')

A more specific name would be nice here.

pacem in terris / мир / शान्ति / ‎‫سَلاَم‬ / 平和
Kevin R. Bullock