Page MenuHomePhabricator

angel.ezquerra (Angel Ezquerra)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 6 2019, 1:46 PM (40 w, 1 d)

Recent Activity

Jan 16 2019

angel.ezquerra added a comment to D5496: revset: add "samebranch" keyword argument to the merge revset.
In D5496#82671, @yuja wrote:
> `[, 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).

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."

Jan 16 2019, 1:11 PM
angel.ezquerra added a comment to D5495: revset: add "branch" positional arguments to the merge revset.

Thank you Matt. I just did that.

Jan 16 2019, 3:04 AM
angel.ezquerra abandoned D5497: revset: add tests for the new merge() arguments (withbranch and samebranch).

The tests in this revision where integrated into the other 2 revisions in this set of patches upon request by @yuja .

Jan 16 2019, 2:58 AM

Jan 15 2019

angel.ezquerra added a comment to D5496: revset: add "samebranch" keyword argument to the merge revset.
In D5496#82394, @yuja wrote:

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

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

Jan 15 2019, 7:02 PM

Jan 14 2019

angel.ezquerra added a comment to D5495: revset: add "branch" positional arguments to the merge revset.
In D5495#82397, @yuja wrote:

Generally looks good.
Can you fix a couple of nits? And if possible, fold the tests from D5577
into this and the next patch. We prefer including relevant test in each
commit.

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

merge([withbranch]) as it is an optional parameter.

def merge(repo, subset, x):

  • """Changeset is a merge changeset.

+ """Changeset is a merge changeset
+
+ All merge revisions are returned by default. If a "withbranch"
+ pattern is provided only merges with (i.e. whose second parent
+ belongs to) those branches that match the pattern will be returned.
+ The simplest pattern is the name of a single branch. It is also
+ possible to specify a regular expression by starting the pattern
+ with "re:". This can be used to match more than one branch
+ (e.g. "re:branch1|branch2").

"""
# i18n: "merge" is a keyword
  • getargs(x, 0, 0, _("merge takes no arguments"))

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

Can you merge this with the next if withbranch: block to reduce the number
of conditionally defined variables referenced later?

cl = repo.changelog
  • return subset.filter(lambda r: cl.parentrevs(r)[1] != -1,
  • condrepr='<merge>')

+ # 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)

Nit: these comments seem a bit verbose. It's documented in stringmatcher().

+ if kind == 'literal':
+ matchfn = 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()))

If we don't have anything special about the literal kind, we can always use
the branchmatcher(). if kind == 'literal' isn't needed.

Jan 14 2019, 6:36 PM
angel.ezquerra updated the diff for D5495: revset: add "branch" positional arguments to the merge revset.
Jan 14 2019, 6:27 PM
angel.ezquerra updated the diff for D5496: revset: add "samebranch" keyword argument to the merge revset.
Jan 14 2019, 6:27 PM

Jan 13 2019

angel.ezquerra updated the diff for D5497: revset: add tests for the new merge() arguments (withbranch and samebranch).
Jan 13 2019, 5:45 PM
angel.ezquerra updated the diff for D5496: revset: add "samebranch" keyword argument to the merge revset.
Jan 13 2019, 5:45 PM
angel.ezquerra updated the diff for D5495: revset: add "branch" positional arguments to the merge revset.
Jan 13 2019, 5:45 PM

Jan 11 2019

angel.ezquerra added a comment to D5495: revset: add "branch" positional arguments to the merge revset.
In D5495#82036, @yuja wrote:
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.

Yep, I agree with that.

That being said, I really like your idea since I often find myself being unable to express what I want with a revset (as powerful as those are) because of the lack of a filtering mechanism. Adding a generic filter function would be very useful indeed. I'm not sure if the syntax you propose would work as is though. It seems that it would need a new "&" operator? In any case I believe that it is out of the scope of this particular set of patches. Do you agree?

Yes. Actually I have a PoC-level implementation of generic filtering predicate,
which can be reviewed separately.

If so I can focus on improving this patch by adding the stringmatcher as you suggest (as it seems I'm not the only one who thinks this would be useful). Is that ok?

Sounds good to me. To be clear, I want 'withbranch' instead of
'*withbranch', because the withbranch option doesn't look like a first-class
parameter of the merge() predicate.

Jan 11 2019, 6:10 PM

Jan 9 2019

angel.ezquerra added a comment to D5495: revset: add "branch" positional arguments to the merge revset.
In D5495#81562, @yuja wrote:

+@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.

I understand this will be useful in a certain branch strategy, but the
proposed syntax is hardly extensible. Maybe it can be a non-wildcard argument
of stringmatcher type so we can at least add another option later.
Any thoughts? Do anyone love this feature?
If we had a syntax to filter revisions by sub expression, this and the
samebranch option could be expressed as follows:

merge() & filter($a, p2($a) & branch("..."))
merge() & filter($a, samebranch(parents($a)))
where filter(argname, boolean-expr)

This is much more expressive (or verbose) and can support other types of
branches, but is hard to implement.

Jan 9 2019, 6:21 PM

Jan 6 2019

angel.ezquerra created D5497: revset: add tests for the new merge() arguments (withbranch and samebranch).
Jan 6 2019, 2:03 PM
angel.ezquerra created D5496: revset: add "samebranch" keyword argument to the merge revset.
Jan 6 2019, 2:03 PM
angel.ezquerra created D5495: revset: add "branch" positional arguments to the merge revset.
Jan 6 2019, 2:03 PM