There are definitely repositories in the wild where p1 is nullrev (and p2 is not). It's unusual but expressable so, of course, it happened.

For that matters, there is also case with nullrev != p1 && p1 == p2.

socket.error is a subclass of IOError, which we catch higher up. It

I've seen many bugs in the git codebase that were caused by it not
having a null revision and being forced to treat root commits
differently. Mercurial has a null revision and I think it's generally
a bug to treat it differently from other commits in graph algorithms.

> Okay, I didn't notice that. And it's tricky to map `samebranch=False` to
>  "different branch" constraint. I would read it as "I don't care whether
>  the branches are the same or not."
>
> We can instead express it as `merge() - merge(samebranch=True)`.

Do you mean that the flag should only indicate whether you want to hide the same branch merges?

@yuja the doc you're linking is about integration tests, so it wouldn't apply to these tests which are really unitary in my mind. Usually the main difference would be the access to the private constructs that the integration tests can't perform, but it's true that most of these implementations are public anyway.

Queued this, many thanks.

Replaced all [[ -f ]] with [ -f ]. `[[]]` requires Bash.

I haven't handled your other remark,

> `[, samebranch]` or [, samebranch=False]`.

I guess that means:

@predicate('merge([withbranch [, samebranch=None]])', safe=True)

Right? (I realized that it is incorrect to say that samebranch's default value is False).

Queued the series, thanks.

Fixed pyflakes warnings in flight.

What about making the argument a revset instead of a branch name. You can get the same result `merge(branch("foo")` but have a more expressive result `merge(only(4.8, 4.7))` ?

> `_performrevert()` would be in the same boat, but it was explicitly flagged on
>  at https://phab.mercurial-scm.org/rHGf37a69ec3f4717fdb4f00699ca06c225f106696c. This implies that the `diff.ignorews` option would be used
>  in practice to exclude whitespace changes while interactive commit/revert.
>  So disabling any whitespace options would break someone's workflow.

I'm confused after this comment. How do you want me to move forward?

Generally looks good.

withbranch = ''
if 'withbranch' in args:
    withbranch = getstring(args['withbranch'],
                           _('withbranch argument must be a string'))
    kind, branchname, branchmatcher = stringutil.stringmatcher(withbranch)

+ samebranch = None
+ if 'samebranch' in args:
+ # i18n: "samebranch" is a keyword
+ samebranch = getboolean(args['samebranch'],
+ _('samebranch argument must be a True or False'))

cl = repo.changelog
# create the function that will be used to filter the subset
if withbranch:
    # matchfn is a function that returns true when a revision
    # is a merge and the second parent belongs to a branch that
    # matches the withbranch pattern (which can be a literal or a regex)
    if kind == 'literal':

• matchfn = lambda r: (cl.parentrevs(r)[1] != -1
• and repo[r].p2().branch() == withbranch) + basematchfn = lambda r: (cl.parentrevs(r)[1] != -1 + and repo[r].p2().branch() == withbranch) else:
• matchfn = lambda r: (cl.parentrevs(r)[1] != -1
• and branchmatcher(repo[r].p2().branch()))
• else:
• # matchfn is a function that returns true when a revision is a merge
• matchfn = lambda r: cl.parentrevs(r)[1] != -1 + basematchfn = lambda r: (cl.parentrevs(r)[1] != -1 + and branchmatcher(repo[r].p2().branch())) + else: + basematchfn = lambda r: cl.parentrevs(r)[1] != -1 + if samebranch is None: + matchfn = basematchfn + else: + # if samebranch was specified, build a new match function + # that on top of basematch checks if the parents belong (or not) + # to the same branch (depending on the value of samebranch) + def matchfn(r): + c = repo[r] + if not basematchfn(r): + return False + issamebranchmerge = c.p1().branch() == c.p2().branch() + return issamebranchmerge if samebranch else not issamebranchmerge

Queued with minor modifications, thanks.

I'm not sure when ``opener.options`` would ever be explicitly
set to None. It is definitely not possible to construct a repo
this way because ``localrepo.resolvestorevfsoptions()`` always
returns a dict and ``localrepo.makelocalrepository()`` always
sets ``opener.options`` to this value.

Because we always execute this code now, if options are empty
we defaulted to creating version 0 revlogs. So we had to change
the code slightly to fall back to the default revlog version
and flags.

As astute reader will note that it is not possible to create
version 0 revlogs now. However, I don't think it was possible
before, as this required ``opener.options`` being unset, which
I don't think was possible. I suspect this means our test
coverage for version 0 revlog repositories is possibly
non-existent! Since I don't see a config option to disable
revlog v1, I'm not even sure if we had a way to create new
repos with version 0 revlogs! Who knows.

Queued up to this, thanks.

• if self._initempty:
• # changelogs don't benefit from generaldelta + if self._initempty and (self.version & 0xFFFF == revlog.REVLOGV1): + # changelogs don't benefit from generaldelta.

> It was intentionally added by https://phab.mercurial-scm.org/rHG3f1dccea9510c122cf9ab0e7a5a19ceed3600a7f (with no tests.)

Do you want me to add tests for that?  If yes, please mention the things I need to take care of on
 writing those tests.

+ if p.returncode:
+ raise Exception('failed to build docker image: %s %s' % (p.stdout, p.stderr))

• a/mercurial/cmdutil.py +++ b/mercurial/cmdutil.py @@ -237,6 +237,7 @@ def dorecord(ui, repo, commitfunc, cmdsuggest, backupall, filterfn, *pats, **opts): opts = pycompat.byteskwargs(opts) + ignorews = opts.get('ignorews', False)

+ $ cat >> testmocks.py << EOF
+ > # mock out util.makedate() to supply testable values
+ > import os
+ > from mercurial import pycompat, util
+ > from mercurial.utils import dateutil
+ >
+ > def mockmakedate():
+ > filename = os.path.join(os.environ['TESTTMP'], 'testtime')
+ > try:
+ > with open(filename, 'rb') as timef:
+ > time = float(timef.read()) + 1
+ > except IOError:
+ > time = 0.0
+ > with open(filename, 'wb') as timef:
+ > timef.write(pycompat.bytestr(time))
+ > return (time, 0)
+ >
+ > dateutil.makedate = mockmakedate
+ > EOF

Removed these lines. The extension is enabled at the beginning.

+ $ hg debugrebuilddirstate
+ unknown exception encountered, please report by visiting
+ https://mercurial-scm.org/wiki/BugTracker
+ Python 2.7.12 (default, Nov 12 2018, 14:36:49) [GCC 5.4.0 20160609]
+ Mercurial Distributed SCM (version 4.8.1+588-479a5ea51ccc+20181224)
+ ** Extensions loaded: narrow, sparse
+ Traceback (most recent call last):
+ File "/place/vartmp/hgtests.zMelCK/install/bin/hg", line 43, in <module>
+ dispatch.run()

Perhaps scmutil.matchfiles() can be used instead of the low-level constructor.

This would not make it possible to select multiple "merged with" branches by doing: hg log -r "merge(feature1, feature2)"
Instead I guess you are proposing that for that use case we force the user to do: hg log -r "merge('re:(feature1|feature2)')

Did I understand you correctly?

+ bases = self._common.bases
+ if callable(bases):
+ # This happens for Rust exported object, and we don't know
+ # at the moment how to make an equivalent of property for them
+ bases = bases()
+

• 1. The presence of nullrev will confuse heads(). So filter it out. return set(self._repo.revs('heads(%ld)',
• self._common.bases - {nullrev})) + (r for r in bases if r != nullrev)))

Queued up to this patch, thanks.

Sorry for late, but doesn't it break hg record --ignore-all-space, etc.?
It was intentionally added by 3f1dccea9510 (with no tests.)

I think it would be a good idea to make the "branch" arguments more flexible. One option could be to use a stringmatcher to add support for regular expressions as you suggest. I can look into that. However there may be some other options worth exploring. The one you suggest is very interesting although I find the syntax a bit complicated for the common use cases that I want to enable which are:

1. Ignore merges from the same branch, which in a named-branch based branching strategy are usually irrelevant
2. Look into merges with a specific branch (e.g. which branches have been merged with the default branch)?

In my experience those two are the ones that are the most common and I think we should try to make the easy to use. That is, I think that even if mercurial had a filter function like the one you propose I would still want to be able to express those 2 common merge properties in a simple way.

Queued for stable, thanks.

> This is logically incorrect. The problem is that we're doing variable
>  expansion at too lower layer. `vfs(expand(user_specified_path))` makes
>  some sense, but `vfs(expand(getcwd()))` is clearly wrong. And the vfs class
>  can't know where the `base` comes from.

If I add a condition for expanding env var if present in `hgrc`, can this work as a fix?

The code already supports this but there is no syntax to make use of
it, so it seems reasonable to create such syntax. I create a new
hgignore syntax "rootedglob". There might be a better name, but
"rooted" is the terminology in use in user-facing documentation.

+@predicate('merge(*withbranch)', safe=True)

def merge(repo, subset, x):

• """Changeset is a merge changeset. + """Changeset is a merge changeset + + All merge revisions are returned by default. If one or more "withbranch" + names are provided only merges with those branches (i.e. whose + second parent belongs to one of those branches) will be returned.

def __init__(self, base, audit=True, cacheaudited=False, expandpath=False,
             realpath=False):

+ if '$' in base and os.path.isdir(base):
+ # when there exists a repo '$foo' and an env var foo=bar, stop
+ # expanding path. refer issue5739.
+ expandpath = False

> mharbison72 wrote in histedit.py:1707
> Out of curiosity, why the sleep?

Otherwise, the user won't be able to see the warning as the editor opens for histedit.

def diffordiffstat(ui, repo, diffopts, node1, node2, match,

• changes=None, stat=False, fp=None, prefix='',
• root='', listsubrepos=False, hunksfilterfn=None): + changes=None, stat=False, graphwidth=0, fp=None,

Looks generally good.

• return max(i * graphwidth maxtotal, int(bool(i))) + return max(i * graphwidth max(diffstatsum(stats)), int(bool(i)))

> Does the class name actually matter? Personally I don't care if
>  lazyancestors() function returns a LazyAncestors object. We'll anyway
>  need a wrapper function to make pure ancestors and rustext ancestors
>  compatible.

Yes, that's in line with your other comments, whereas I was pursuing the
goal of putting the whole `ancestor` module under responsibility of
`policy.importmod` : in that case, it would have been necessary to have
the same name.

We like the UX of `hg branch --rev R XXX` for changing the branch of a commit, it's the same UX than bookmark and topic. It's also easier than `hg branch --force XXX; hg amend`.

@@ -4438,7 +4438,7 @@

remotebookmarks = fremotebookmarks.result()
remotebookmarks = bookmarks.unhexlifybookmarks(remotebookmarks)
pullopargs['remotebookmarks'] = remotebookmarks

• for b in opts['bookmark']: + for b in opts.get('bookmark'):

> commands.py:4441
> pullopargs['remotebookmarks'] = remotebookmarks
> for b in opts['bookmark']:
> b = repo._bookmarks.expandname(b)

@yuja this one?

I tried to queue this, but the patch doesn't include any metadata (e.g. author
and date.) Please check your configuration.

+$ hg amend --config rewrite.updatetimestamp=True
+$ hg log --limit 2
+changeset: 40473:e64479cbaf37

nodes = None

• if opts['bookmark'] or revs: + if opts.get('bookmark') or revs:

+#if updatetime option is turned on
+ $ hg amend --date '1997-1-1 0:1'

Looks mostly good.

> ctx points to the tipmost branch head. so `rev not in revs` doesn't mean any
>  of the `revs` do not belong to the branch.

Is there any possibility of workaround within this iteration itself @yuja? I can do outside the current loop by creating a branches list and map it by iterating over scmutil.revrange() as suggested by you. But, that won't fetch all details. Just changeset and corresponding branch. We need to keep the same output format, right?

Nice, thanks!

0. I move metaedit command to core
1. I move branch changing functionality to metaedit

fm.startitem()

+ rev = ctx.rev()
+ if opts.get('rev') and rev not in revs:
+ continue

+/ # TODO find a solution to make it GIL safe again.
+/
+/ This is non trivial, and can wait until we have a clearer picture with
+/ more Rust Mercurial constructs.
+/
+/ One possibility would be to a GILProtectedIndex wrapper enclosing
+/ a Python<'p> marker and have it be the one implementing the
+/ Graph trait, but this would mean the Graph implementor would become
+/ likely to change between subsequent method invocations of the hg-core
+/ objects (a serious change of the hg-core API):
+/ either exposing ways to mutate the Graph, or making it a non persistent
+/ parameter in the relevant methods that need one.

> IIRC, listkeys is a newer command than lookup. If the peer doesn't support listkeys, I suspect this batch query would fail. In that case, maybe listkeys has to be skipped if the peer doesn't support it and if --bookmark is not specified.

listkeys shouldn't need compatibility check in the caller, because it's defined like this in wireprotov1peer.py (https://www.mercurial-scm.org/repo/hg-committed/file/tip/mercurial/wireprotov1peer.py#l381):

   @batchable
   def listkeys(self, namespace):
       if not self.capable('pushkey'):
           yield {}, None

+/ The purpose of this module is to hide identifiers from other Rust users
+/
+/ Some of the identifiers we are defining are meant for consumption
+/ from Python with other naming conventions. For instance, py_class!
+/ does not let us make a distinction between the Python and the Rust name,
+/ and that name ends up in particular in the __class__ attribute, so that
+/ renaming it inside its Python module is not enough.
+/
+/ For Rust consumers, we will reexport these definitions, following the
+/ appropriate naming convention.
+mod concealed_detail {
+ use super::*;
+
+ py_class!(pub class lazyancestors |py| {

+try:
+ from . import rustext

Thanks for the info @yuja. I'm thinking to move --rev/-r to branches command and replacing --show/-s that I've made to --rev/-r in the branch command itself.

I have a concern about compatibility with ancient Mercurial. Can you check it
and send a follow-up as needed?

No stackaliases found in core. Maybe it's an extension-specific config?

_('set branch name even if it shadows an existing branch')),
('C', 'clean', None, _('reset branch name to parent branch name')),
('r', 'rev', [], _('change branches of the given revs (EXPERIMENTAL)')),

+ ('s', 'show', None, _('show branch name of the given rev'))

],
_('[-fC] [NAME]'),
helpcategory=command.CATEGORY_CHANGE_ORGANIZATION)

@@ -1097,6 +1098,11 @@

ui.write("%s\n" % repo.dirstate.branch())
return

+ elif opts.get('show') and label:
+ ctx = scmutil.revsingle(repo, label)
+ ui.write("%s\n" % ctx.branch())
+ return

@yuja I'm not sure by what you consider exactly to be internals here. If that's the `[5]` and [6]`, maybe a `parents(revision)` method on the index would be better ? The obvious drawback would be to write more C code.

Given that my ultimate goal here is to delegate the whole `ancestors` module to `policy.importmod`, where would we have the factory function you're thinking of ?

+ $ find main/d* -type f
+ main/d1/f
+ main/d3/g
+ main/d3/f
+ main/d5/f
+ main/d7/f

+def copytoworkingcopy(repo, tr):
+ if tr:
+ def write(file):
+ spec = repo.svfs.read(FILENAME)
+ file.write(spec)
+ file.close()
+ tr.addfilegenerator('narrowspec', (DIRSTATE_FILENAME,), write,
+ location='plain')
+ else:
+ spec = repo.svfs.read(FILENAME)
+ repo.vfs.write(DIRSTATE_FILENAME, spec)

This is ready for review again, thanks

AFAIK, the protocol is never stateful (not sure how that'd work with http), so you can't rely on a notion of session-level state on the server to guarantee consistency.

Queued this, thanks.