D8172: notify: optional mail threading based on obsmarker

[marmoute (Pierre-Yves David)]

This revision now requires changes to proceed.
marmoute added a comment.
marmoute requested changes to this revision.


  The feature looks interresting, but I had a couple of feedback.

INLINE COMMENTS

> notify.py:136
>  
> +notify.reply (EXPERIMENTAL)
> +  If True and the changeset has predecessor in the repository, include

maybe `reply-to-predecessors` would be clearer ?

> notify.py:137
> +notify.reply (EXPERIMENTAL)
> +  If True and the changeset has predecessor in the repository, include
> +  the "In-Reply-To" header with the notification mail. This option should

The phrasing used in the rest of the doc is `If set`

> notify.py:139
> +  the "In-Reply-To" header with the notification mail. This option should
> +  be used in combination with ``notify.messageidseed``.
> +

From waht I understand, this feature will misbehave if notify.messageidseed is not set. I am I right, I believe we should replace "should" by "must" and abort if we detect the new option being used without the other one.

> notify.py:453-457
> +            predecessors = list(
> +                unfi[ctx2]
> +                for ctx2 in obsutil.allpredecessors(unfi.obsstore, [ctx.node()])
> +                if ctx2 != ctx.node() and has_node(ctx2)
> +            )

nits: could be `[]` instead of `list()`

> notify.py:459
> +            if predecessors:
> +                pred = min(predecessors, key=lambda ctx: ctx.rev())
> +                msg['In-Reply-To'] = messageid(

The "min" does not garantee we are using the root predecessors here. It is just an attemps to get the first that resulted in an email, right? If so, you should document it so that the next soul looking at this code do not get mislead.

You should also document that in case of fold, only one will be replied to.

REPOSITORY
  rHG Mercurial

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

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

To: joerg.sonnenberger, #hg-reviewers, marmoute
Cc: marmoute, pulkit, mercurial-devel