[PATCH 13 of 13] obsolete: explicitly track folds inside the markers

[Boris FELD]

On 30/09/2018 15:13, Yuya Nishihara wrote:
> On Thu, 27 Sep 2018 19:08:45 +0200, Boris Feld wrote:
>> # HG changeset patch
>> # User Boris Feld <boris.feld at octobus.net>
>> # Date 1537998614 -7200
>> #      Wed Sep 26 23:50:14 2018 +0200
>> # Node ID 1abacb9c2d03520a12af25ae0d9ae5978f6aa3db
>> # Parent  bd762ec24f1858bc577b05de0388688a7756f6ba
>> # EXP-Topic trackfold
>> # Available At https://bitbucket.org/octobus/mercurial-devel/
>> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 1abacb9c2d03
>> obsolete: explicitly track folds inside the markers
> Queued up to the previous patch, dropping the first two which appear to be
> already in. Thanks.
>
> I don't have a strong concern about this patch, but this touches the storage
> so I think this needs more eyes.
>
>> +def makefoldid(relation):
>> +    folddata = ''.join(p.node() for p in relation[0] + relation[1])
>> +    # Since fold only has to compete against fold for the same successors, it
> Does it mean there's no reason to include relation[1] in the hash?
It is probably still useful to have an id as unique as possible. The
goal here is more about producing a unique identifier than a
reproducible one. In fact, we should probably also include local
revisions numbers and users to help distinguish between two similar but
independent fold.
>
>> +    # seems find to use a small ID. Smaller ID save space.
> s/find/fine/
>
>> +    return hashlib.sha1(folddata).hexdigest()[:8]
> Nit: has to be hex(...digest()) to be py3 compat.
>
>> -            for prec in predecessors:
>> +            foldid = None
>> +            foldsize = len(predecessors)
>> +            if 1 < foldsize:
>> +                foldid = makefoldid(rel)
>> +            for foldidx, prec in enumerate(predecessors, 1):
> Nit: any reason to make it 1-origin?
Not a strong reason, but working on ancestors iteration in Rust reminded
us how annoying it was to not have the option to use '0' as a special
value when talking about arbitrary indexes. So we would prefer to be on
the safe side here.
>
>>                  sucs = rel[1]
>>                  localmetadata = metadata.copy()
>>                  if 2 < len(rel):
>>                      localmetadata.update(rel[2])
>> +                if foldid is not None:
>> +                    localmetadata['fold-id'] = foldid
>> +                    localmetadata['fold-idx'] = str(foldidx)
>> +                    localmetadata['fold-size'] = str(foldsize)
> '%d' % for py3 compat.
>
> Nit: I'm not sure if "idx" and "size" are good names, but I'm fine with that.

Ideally, we'll make them official binary field the next iteration of the
obsstore format. Removing the naming concerns.

We will send a V2 for the rest of the Nits.

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel