[Commented On] D12097: branchmap: skip obsolete revisions while computing heads

[marmoute (Pierre-Yves David)]

marmoute added a comment.


  This patches looks good overall, but I would like to seems a couple of comment expanded for clarity.

INLINE COMMENTS

> branchmap.py:352-355
>              return (self.tipnode == repo.changelog.node(self.tiprev)) and (
> -                self.filteredhash == scmutil.filteredhash(repo, self.tiprev)
> +                self.filteredhash
> +                == scmutil.filteredhash(repo, self.tiprev, needobsolete=True)
>              )

Small nit: we should probably split this four-liner into individual statement.

> branchmap.py:526
> +                        # We ignored this obsolete changeset earlier, but now
> +                        # we want to handle it, since it's a parent of newrev.
> +                        obsparents.add(p)

`handle it` is a bit vague. What about:

We ignored this obsolete changeset earlier, but now that it has non-ignored children, we need to make sure its ancestors are not considered heads. So we mark it for the same post-processing we use for parent in other branches.

> scmutil.py:362
> +    This function hashes all the revs filtered from the view (and, optionally,
> +    all obsolete revs) and returns that SHA-1 digest.
>      """

maybe clarify that we are talking of all filtered and obsolete revision that are ≤ to maxrev ?

> scmutil.py:369
> +            return None
> +        key = (maxrev, hash(cl.filteredrevs), hash(frozenset(obsrevs)))
> +    else:

The use of `frozenset` on the obsolete set here is unfortunate. As mean a significant computation just to validate the inner cache key.

Please add a comment about this.

I am not sur why the obsrevs are not a frozenset already so we should look into that (outside of this series).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D12097/new/

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

To: av6, #hg-reviewers
Cc: marmoute, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20220131/2db15772/attachment-0002.html>