D6140: revset: add new contiguous(x) function for "x::x"

mharbison72 (Matt Harbison) phabricator at mercurial-scm.org
Thu Mar 21 00:16:38 EDT 2019


mharbison72 added a comment.


  In https://phab.mercurial-scm.org/D6140#89771, @martinvonz wrote:
  
  > In https://phab.mercurial-scm.org/D6140#89770, @martinvonz wrote:
  >
  > > Josef 'Jeff' Sipek <jeffpc at josefsipek.net> sent this to mercurial-devel. I'm adding it here for reference.
  > >
  > > I read this doc string and the patch intro several times, and every time I
  > >  concluded that this function was useless.  Only after reading some of the
  > >  other replies, did I realize that "x" here can be a set.
  >
  >
  > The docstring does say "in the set" :) But I agree that it's not very clear. I copied the pattern from other functions. I would probably have said "in the input set" otherwise. Do you think that would have been clearer? We could make that change to all the existing cases of plain "set" referring to the input.
  
  
  That seems good to me.  Maybe there should be a general blurb somewhere that a single element is treated like a set for input purposes, and doesn't need any special syntax to make it a set.  That was something I got confused by when I first started using revsets.
  
  In https://phab.mercurial-scm.org/D6140#89769, @martinvonz wrote:
  
  > In https://phab.mercurial-scm.org/D6140#89749, @mharbison72 wrote:
  >
  > > I'm sure I've used the word "contiguous" when describing the `::` operator to people, but the case @marmoute referenced and even the `contiguous(9+3+4)` result don't match my expectations of the English word.
  >
  >
  > I think we're still looking for English words where the behavior of this function would match expectations.
  >
  > > (For the latter, the 3 doesn't seem contiguous with the rest of the set.)
  >
  > I'm not sure which 3 you mean. 4 and 9 were made contiguous by the addition of 8, but 3 was a on different branch, so it wasn't. What the revset actually does is to include all commit that have both ancestors and descendants in the input set.
  
  
  Sorry, I meant to specify the last test:
  
    $ hg log -G -T '{rev}\n' --config experimental.graphshorten=True
    @  9
    o  8
    | o  7
    | o  6
    |/|
    | o  5
    o |  4
    | o  3
    o |  2
    |/
    o  1
    o  0
    
    $ log 'contiguous(9+3+4)'
    3
    4
    8
    9
  
  I agree that the output is behaving as documented, and this is existing behavior.  I was just noting the mental disconnect I have with the name in this situation, where 3 is "contiguous".  I'm not sure what you mean by "3 ... wasn't", because it's in the output.
  
  >>   FWIW, the first 3 words in the help for `::` is "A DAG range".
  > 
  > Maybe it's just me, but I think it's a bit unfortunate if we describe `::` as "a DAG range" and then have a `dagrange()` function that doesn't support all the same cases (specifically, it won't support `::x` and `x::`).
  
  Yeah, I can see what you mean now.  I'm not sure what the `:x` kind of range is called in python, but it kind of reminds me of an open range, whereas `x::y` is closed or bounded.  So maybe `dagclosedrange(x)` handles this, and `dagopenrange(...)` handles `::x` and `x::`?  As I was typing this out, I ended up with the same idea you had suggested above with `dagrange(roots=X, heads=X)`.  I'm not sure why you think that's weird- it seems like the same rules for heads and roots with `::`.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers
Cc: mharbison72, yuja, av6, spectral, gracinet, marmoute, mercurial-devel


More information about the Mercurial-devel mailing list