As stated in the issue5123, unshelve can destroy the second parent of
the context when tried to unshelve with an uncommitted merge. This
patch makes unshelve to abort when called with an uncommitted merge.

• • a/hgext/mq.py +++ b/hgext/mq.py @@ -3518,7 +3518,10 @@ delattr(self.unfiltered(), r'mq')
    
    def abortifwdirpatched(self, errmsg, force=False):
• if self.mq.applied and self.mq.checkapplied and not force: + shelveinprogress = any('shelvedstate' in state + for state in cmdutil.unfinishedstates) + if (self.mq.applied and self.mq.checkapplied and not force and + not shelveinprogress):

@@ -231,8 +235,10 @@

    raise ValueError(
        r'node %s does not exist' % pycompat.sysstr(hex(node)))
self._entries.setdefault(label, []).append(node)

+ self._verifiedbranches.add(label)

def copy(self):
    """return an deep copy of the branchcache object"""

• return type(self)(
• self, self.tipnode, self.tiprev, self.filteredhash, + return branchcache( + self.entries, self.tipnode, self.tiprev, self.filteredhash, self._closednodes)

Oh, right... I saw that and that was part of why I didn't mail earlier :D  I think this is just an "expected" difference in behavior,

+ self.entries = set()
+ chunk = b''
+ for c in iter(functools.partial(fp.read, fncache_chunksize), b''):
+ chunk += c
+ try:
+ p = chunk.rindex(b'\n')
+ self.entries.update(decodedir(chunk[:p + 1]).splitlines())
+ chunk = chunk[p + 1:]
+ except ValueError:
+ # substring '\n' not found, maybe the entry is bigger than the
+ # chunksize, so let's keep iterating
+ pass

+ for c in iter(functools.partial(fp.read, chunksize), b''):
+ chunk += c
+ try:
+ p = chunk.rindex(b'\n')
+ self.entries |= set(decodedir(chunk[:p + 1]).splitlines())

I was under the impression that that was more of a hack, mainly useful for when there are lots of existing `opts.get(b'')` uses in a file, and when there are a few it was better to change to `r''`.

Ah,  we can indeed (thanks for the pointer), with one caveat:  the formatting.
The existing output is nicely formatted for human readability, with each entry on a new line and indented appropriately.  `templatefilters.json()` isn't capable of that.
Do we mind losing that?  It's also tested for in test-phabricator.t. Compare before:

I think "contiguous" is good as it stands for the main use case. "closure"
seems confusing unless we have stronger math background than computer science.
The other candidates would require more knowledge about the theory.

Maybe need a pullreq? We can't review i18n messages.

• params = json.loads(ui.fin.read())
• result = callconduit(repo, name, params)
• s = json.dumps(result, sort_keys=True, indent=2, separators=(b',', b': '))
• ui.write(b'%s\n' % s) + # json.(load|dump)s only returns/accepts unicode strings + params = pycompat.rapply(lambda x: + encoding.unitolocal(x) if isinstance(x, pycompat.unicode) else x, + json.loads(ui.fin.read()) + )

pycompat.byteskwargs() can be used instead.

Queued most of the patches in this series, thanks.

> Perhaps some of `r''` would have to be changed to `b''` since dict keys
>  are now byte strings. See the wiki page for our Py3 hacks.
> 
> https://www.mercurial-scm.org/wiki/Python3

Ah, good point, thanks.  Would it be worth keeping the keys as r'' strings?  rapply can fairly easily be extended with an optional notkeys boolean that would allow doing so.

After this series, the number of test failures in that mode is ~70. Most of the remaining failures are in `stringutil.wrap()`. Essentially `textwrap` from the standard library wants to operate on native `str`. Our version is feeding in `unicode`. We get into trouble when the standard library code does a `''.join()` and some `unicode` is coerced into `str`. TBH I'm not sure how we're not seeing bugs due to this on Python 2. I'm guessing we're lacking test coverage for something passing non-ASCII into `stringutil.wrap()`?

• parsed = json.loads(body) + parsed = pycompat.rapply(lambda x: encoding.unitolocal(x) + if isinstance(x, unicode) else x, json.loads(body))

+ filtername = repo.filtername
+ if filtername is None:
+ filtername = 'None'

repo.ui.log('branchcache', 'updated %s branch cache in %.4f seconds\n',

• repo.filtername, duration) + filtername, duration)

I don't think totalsize has to be stat()ed. We can just loop over until
fp.read() reaches to EOF. It's unreliable to assume that the stat('fncache')
see the identical file to the fp open()ed before.

(resend without the "On ... wrote:" line)

if not bcache.validfor(repo):
    # invalidate the cache
    raise ValueError(r'tip differs')

• cl = repo.changelog
• for line in lineiter:
• line = line.rstrip('\n')
• if not line:
• continue
• node, state, label = line.split(" ", 2)
• if state not in 'oc':
• raise ValueError(r'invalid branch state')
• label = encoding.tolocal(label.strip())
• node = bin(node)
• if not cl.hasnode(node):
• raise ValueError(
• r'node %s does not exist' % pycompat.sysstr(hex(node)))
• bcache.setdefault(label, []).append(node)
• if state == 'c':
• bcache._closednodes.add(node) - + bcache.load(repo, f) except (IOError, OSError): return None
  
  @@ -214,6 +198,26 @@
  
  return bcache
  
  + def load(self, repo, f): + """ fully loads the branchcache by reading from the file f """ + cl = repo.changelog + lineiter = iter(f)

with ui.configoverride(overrides, 'record'):

+ if repo.vfs.exists('histedit-state'):
+ raise error.Abort(_("histedit in progress, can't qrecord"))

cmdutil.dorecord(ui, repo, committomq, cmdsuggest, False,

I looked into why we don't return diff headers in quiet mode and found the
behavior is from https://phab.mercurial-scm.org/rHG8f8bb77d560e70bcc95577e4dfa877df18d876ab which does not have
any mention about why it is done. We also show the diff headers in git, so I
think it's fine showing diff header in normal diff in quiet mode.

+@predicate('expectsize(set[, size])', safe=True, takeorder=True)
+def expectrevsetsize(repo, subset, x, order):
+ """Abort if the revset doesn't expect given size"""
+ args = getargsdict(x, 'expect', 'set size')

`getuipathfn()` uses `repo.pathto()` when a relative path was requested (including by setting `legacyrelativevalue=True` or `forcerelativevalue=True`), so I think there shouldn't be much change in behavior with that commit.

nit-pick is resolved. `getintrange()` will throw a `ParseError` on setting size to one of `min:`, `:max` or `:`. In ideal case, on calling from `expectsize()` it shouldn't fail.

Getting close.

> It might
>  break some Windows tests, but it should already be broken since https://phab.mercurial-scm.org/rHGa997163e7fae2fe933f8d0c6d1013205befd1ee4.
>  Some (or all?) `uipathfn()`s have to respect `ui.slash` config on Windows.

Did that commit change any behavior on Windows?

• a/mercurial/scmutil.py +++ b/mercurial/scmutil.py @@ -51,8 +51,10 @@ )
  
  if pycompat.iswindows: + from . import windows as platform from . import scmwindows as scmplatform else: + from . import posix as platform from . import scmposix as scmplatform

It would have been easy to make all diffs respect ui.relative-paths,
but we don't want that since it makes the diff invalid. Perhaps it
makes sense to do that with --noprefix since the point of that is to
make paths that are easy to copy&paste, and the diff is already
invalid anyway. But this patch just makes the --stat version respect
the config option. The --stat view is not even close to a valid diff,
so I think it makes sense to show the paths in more human-friendly
form.

+@predicate('expectsize(set[, size])', safe=True, takeorder=True)
+def expectrevsetsize(repo, subset, x, order):
+ """Abort if the revset doesn't expect given size"""
+ args = getargsdict(x, 'expect', 'set size')
+ size = args.get('size')
+ if size is not None:
+ minsize, maxsize = getintrange(size,
+ _('expectsize requires a size range'
+ ' or a positive integer'),
+ _('size range bounds must be integers'))

+ $ hg log -r 'expectsize(0:2, 3)'
+ changeset: 0:2785f51eece5
+ branch: a
+ user: test
+ date: Thu Jan 01 00:00:00 1970 +0000
+ summary: 0

@spectral @yuja Can I close this?

• absorig = scmutil.origpath(self.ui, repo, absf) + absorig = scmutil.backuppath(self.ui, repo, f) self.ui.note(_('saving current version of %s as %s\n') %
• (f, os.path.relpath(absorig))) + (f, os.path.relpath(absorig, start=repo.root)))

> You can try out some combinations of `expect(5:0) & 1:10` and
>  `10:1 & expect(0:5)`.

I got into many errors while using this. I might be not understanding this correctly. Could you please elaborate?

@yuja we don't need a seed to reproduce: failed examples contain all the information (that's a difference with the Python version)

This test is useful for me, because it's the only one that really tests the corectness of MissingAncestor, and it's not a bench either.
I agree it feels out of place in  with the unit tests, so my proposal is to make an integration test out of it.

So do we prefer `legacyrelativevalue` then? Or `legacywasrelative`? Or `legacyrelative`? (I think the last one is least clear.)

Updated the tests added at d1d3094b54f9 and queued, thanks.

> I agree with that we would never set the `commands.commit.interactive.{...}`
>  in hgrc, but the feature itself is useful if you have to work on unclean
>  codebase unlike in Google. For example, I sometimes need to commit changes
>  ignoring unrelated whitespace cleanups made by editor or code formatter,
>  because I can't control the development workflow.
>
> That's why I thought there would be users relying on the current behavior.

I think I understand what you're saying.  I was under the impression that what we cared about was that `hg record -b` should continue working. Since there are no diffopts available on `hg commit -i`, you're thinking that this could be written as `hg commit -i --config commands.commit.interactive.ignorews=1`.

While I'm sympathetic to that argument, it is so long and unwieldy that I think I'd recommend that users just do `hg --config extensions.record= record -b` (or, more likely, set `[extensions] record=` in their hgrc).

First, I think the word expect is too general. Perhaps, this should be called
expectsize() or expectlen().

> What I thought confusing is `scmutil.getuipathfn(ctx.repo(), legacyvalue=True)`
>  in https://phab.mercurial-scm.org/D5801. "What does the `True` mean? relative, absolute, or a complete
>  different stuff?"

Same reason it's confusing, I believe: it's unclear if "legacyvalue=True" means "use the legacy value" (incorrect) or "for the legacy value, use the value True" (correct). I was hoping the "value" part would clarify that, but I agree that it's still not clear.  I think you're also saying that the fact that the function deals with producing a cwd-relative or absolute (well, repo-relative) is also not clear and I agree with that too.

> Looks good, but I find it isn't easy to parse the meaning of
>  `getuipathfn(repo, forcevalue=True)`. Perhaps it can be spelled as
>  `forcerelative=True`.

The problem is that this an override value for a boolean value, so it's easy to mistake `forcerelative=False` to mean "don't force it to be relative". That's why I went for `forcevalue` (meaning "force to this value"). I agree that it can still easily be read as "force a value". Perhaps `forceto` works better, although it's a bit unconventional? I'll change it to that and you can tell me if it seems worse.

I am not whether this is correct. This should have fix failure of `test-lfs-serve.t#lfsremote-on` failure on py3 but it does not.

+ if interactive:
+ choice = repo.ui.promptchoice(
+ _("Undelete file %s (Yn)?$$ &Yes $$ &No") % f)

• 1. advertise to browsers svc = Zeroconf.ServiceInfo('_http._tcp.local.',
• name + '._http._tcp.local.', + pycompat.bytestr(name + r'._http._tcp.local.'), server = host, port = port,
• properties = {'description': desc,
• 'path': "/" + path}, + properties = { + 'description': pycompat.bytestr(desc), + 'path': pycompat.bytestr(r"/" + path)}, address = localip, weight = 0, priority = 0) server.registerService(svc)
  1. advertise to Mercurial clients svc = Zeroconf.ServiceInfo('_hg._tcp.local.',
• name + '._hg._tcp.local.', + pycompat.bytestr(name + r'._hg._tcp.local.'), server = host, port = port,
• properties = {'description': desc,
• 'path': "/" + path}, + properties = { + 'description': pycompat.bytestr(desc), + 'path': pycompat.bytestr(r"/" + path)}, address = localip, weight = 0, priority = 0)

Looks good, but I find it isn't easy to parse the meaning of
getuipathfn(repo, forcevalue=True). Perhaps it can be spelled as
forcerelative=True.

> Currently, some os.path functions when opening repositories may
>  raise an uncaught TypeError or ValueError if the path is invalid.

What kind of "invalid"? If the path doesn't exist? Or only if the path variable is of the wrong type (not bytes? not unicode? neither?)?

The ultimate reason I abandoned that and went with the "only respect commandline options, not config, and don't do this for revert --interactive" patch series was because I had to include a note that said something like "if a user *does* set commands.commit.interactive.{ignorews,ignoreblanklines,...}, this will still trigger issue5839; it is unlikely that any user will want to set these options, though."

It's up to you, though. I worry I'm too ingrained in the internal-software development culture where we control what versions people have and can provide quick assistance when users have questions or encounter problems. I might not be properly considering the issues people might run into outside of an enterprise situation.

> + relative = pats or ui.configbool('commands', 'status.relative'):
> + uipathfn = scmutil.getuipathfn(repo, relative)

Not sure if it's worth fixing the syntax error here before it goes public. I only noticed because I was bisecting a test failure. The next patch drops the trailing :.

Fixed various check-code errors by black -l 80 -S tests/test-demandimport.py.

Removed useless '%s' % and queued, thanks.

+ name = pycompat.sysstr(name)

return list(grp.getgrnam(name).gr_mem)

+++ b/mercurial/help.py
@@ -394,7 +394,17 @@

def addtopichook(topic, rewriter):
    helphooks.setdefault(topic, []).append(rewriter)

-def makeitemsdoc(ui, topic, doc, marker, items, dedent=False):
+def _templateextsyms(ui, topic, doc):
+ for name, ext in extensions.extensions(ui):
+ kw = getattr(ext, 'templatekeyword', None)
+ if kw is not None:
+ doc = addtopicsymbols(
+ 'templates', 'Filters\n=======', kw, keepmarker=True)
+ return doc
+
+addtopichook('templating', _templateextsyms)
+
+def makeitemsdoc(ui, topic, doc, marker, items, dedent=False, keepmarker=False):

"""Extract docstring from the items key to function mapping, build a
single documentation block and use it to overwrite the marker in doc.
"""

@@ -420,11 +430,14 @@

            doclines.append('  ' + l.strip())
    entries.append('\n'.join(doclines))
entries = '\n\n'.join(entries)

+ if keepmarker:
+ entries = entries + '\n\n' + marker

return doc.replace(marker, entries)

-def addtopicsymbols(topic, marker, symbols, dedent=False):
+def addtopicsymbols(topic, marker, symbols, dedent=False, keepmarker=False):

def add(ui, topic, doc):

• return makeitemsdoc(ui, topic, doc, marker, symbols, dedent=dedent) + return makeitemsdoc(ui, topic, doc, marker, symbols, dedent=dedent, + keepmarker=keepmarker)

I did not add this to revert --interactive, since that does not currently have any way of getting args specified on the commandline that affect the whitespace settings (so I'm keeping revert --interactive *ignoring* the user's diff settings).

+ track = ui.configlist('blackbox', 'track')
+ if not track:
+ self._active = False
+ elif b'*' in track:
+ self._trackedevents = re.compile(b'.*')
+ else:
+ try:
+ self._trackedevents = re.compile(b'|'.join(track))
+ except re.error as e:
+ ui.warn('invalid blackbox.track setting: %s\n' % e)
+ self._active = False

I wasn't able to come up with a reason to support these but only when committing interactively (as I said in the commit description), but I guess there's justification in https://www.mercurial-scm.org/pipermail/mercurial-devel/2011-June/032316.html.

So I think I can understand why this is desired for this command. Maybe a better option is to change the difffeatureopts() call to replace the section that it looks in? i.e. change:

  diffopts = patch.difffeatureopts(repo.ui, whitespace=True)

to:

  diffopts = patch.difffeatureopts(repo.ui, section='commands-interactive', whitespace=True)

This way we aren't looking at the `[diff]` section, so the options specified on the commandline end up working, and if someone really does want this as a default, they can get that behavior.

I'm not at all attached to that section name, i.e. I could see this being something like the following, to make it look at commands.commit.interactive.ignorews (and others):

  diffopts = patch.difffeatureopts(repo.ui, section='commands', configprefix='commit.interactive.', whitespace=True)

I think I like that option, I'll send a patch for it.

+from future import absolute_import, print_function

+ try:
+ isfile = os.path.isfile(path)
+ # Python 2 raises TypeError, Python 3 ValueError.
+ except (TypeError, ValueError) as e:
+ raise error.Abort(_('invalid path %s: %s') % (
+ path, pycompat.bytestr(e)))

Curious why subprocess output can be a unicode. Is it a sort of text-mode
stream?

command.append('l')
for arg in args:

• command += "%d:%s" % (len(arg), arg) + command += pycompat.bytestr("%d:%s" % (len(arg), arg))

for info in tar:

+ infoname = pycompat.bytestr(info.name)

Perhaps, "summary-template" is more consistent with the other config keys.

> Why is that? @yuja, can you elaborate?
>
> This is version-controlled code that changes in lock-step with the branchmap module, adding backwards compatibility tests here would add a costly maintenance burden. If you have an older revision of the branchmap module, you also have an older revision of the perf code.

I'm pretty sure the idea is that you should be able to enable the latest perf extension and run different versions of hg (perhaps versions that you had already built) against it to compare performance.

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -2955,7 +2955,8 @@ def grep(ui, repo, pattern, *pats, **opt
                     copies.setdefault(rev, {})[fn] = copy
                     if fn in skip:
                         skip[copy] = True
-                        continue
+            if fn in skip:
+                continue
             files.append(fn)

+  mercurial/wireprotov2peer.py:307:
+   >                 self._ui.debug(_('received %r\n') % frame)
+   don't mark debug messages for translation
+  mercurial/wireprotov2peer.py:513:
+   >     return [True if raw[i:i+1] == b'1' else False for i in range(len(raw))]
+   missing whitespace in expression

@@ -2808,6 +2808,10 @@

elif (line.startswith('GIT binary patch') or
      line.startswith('Binary file')):
    isbinary = True

+ elif line.startswith('rename from'):
+ filename = line.split()[-1]
+ elif line.startswith('rename to'):
+ filename += ' => %s' % line.split()[-1]

• if fn in skip: + copy = None + if follow: + try: + copied = flog.renamed(fnode) + except error.WdirUnsupported: + copied = ctx[fn].renamed() + copy = copied and copied[0] if copy:
• skip[copy] = True
• continue + copies.setdefault(rev, {})[fn] = copy + if fn in skip: + skip[copy] = True + continue files.append(fn)

+ mercurial/wireprotov2peer.py:307:
+ > self._ui.debug(_('received %r\n') % frame)
+ don't mark debug messages for translation
+ mercurial/wireprotov2peer.py:513:
+ > return [True if raw[i:i+1] == b'1' else False for i in range(len(raw))]
+ missing whitespace in expression

$ hg log -b --cwd=inexistent default

• abort: $ENOENT$: 'inexistent' + abort: \$ENOENT\$: ('inexistent'|inexistent) (re)

Maybe `anonymous`, defaulting to True?