Patchwork D8172: notify: optional mail threading based on obsmarker

login
register
mail settings
Submitter phabricator
Date Feb. 26, 2020, 9:36 p.m.
Message ID <differential-rev-PHID-DREV-bmdj3r73y4kgm6k7q2m7-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/45356/
State Superseded
Headers show

Comments

phabricator - Feb. 26, 2020, 9:36 p.m.
joerg.sonnenberger created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  When notify.reply is set and a changeset has a predecessor in the
  repository, include In-Reply-To pointing to the message-id that would
  have been generated for the oldest predecessor. This allows mail
  threading like Phabricator for common cases like rebasing changes, but
  will be optimal for cases like folding.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  hgext/notify.py
  tests/test-notify.t

CHANGE DETAILS




To: joerg.sonnenberger, #hg-reviewers
Cc: mercurial-devel
phabricator - March 9, 2020, 9:13 a.m.
pulkit added a comment.


  The code looks good. Since the whole obsmarker thing is experimental, the config option here should be made experimental too.
  Also, can you add tests when the config is set and `--in-reply-to` flag is also used and add info about that behavior in commit message?

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
Cc: pulkit, mercurial-devel
phabricator - March 11, 2020, 3:40 p.m.
durin42 added a comment.
durin42 added subscribers: marmoute, durin42.


  @marmoute

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
Cc: durin42, marmoute, pulkit, mercurial-devel
phabricator - March 18, 2020, 12:12 p.m.
joerg.sonnenberger added a comment.


  @pulkit You still wanted to explain what you mean with --in-reply-to interaction here.

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
Cc: marmoute, pulkit, mercurial-devel
phabricator - March 19, 2020, 8:02 a.m.
pulkit added a comment.


  In D8172#123965 <https://phab.mercurial-scm.org/D8172#123965>, @joerg.sonnenberger wrote:
  
  > @pulkit You still wanted to explain what you mean with --in-reply-to interaction here.
  
  Thanks for the reminder. I looked into details and it seems that I misunderstood few things and mixed it with `--in-reply-to` flag to `hg email` :/

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
Cc: marmoute, pulkit, mercurial-devel
phabricator - March 19, 2020, 9:07 a.m.
marmoute added a comment.


  I am planning to have a look at this by the end of the week. SOrry for the delay

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
Cc: marmoute, pulkit, mercurial-devel
phabricator - March 20, 2020, midnight
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
phabricator - March 27, 2020, 2:09 p.m.
marmoute added a comment.
marmoute accepted this revision.


  Looks good to me, thanks for the update. Sorry for the delay, it totally fell into the cracks.

INLINE COMMENTS

> marmoute wrote in notify.py:459
> 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.

I was thinking of an inline comment in the code. Having this in the doc is actually good idea thanks. Maybe follow up with an inline command too ?

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

Patch

diff --git a/tests/test-notify.t b/tests/test-notify.t
--- a/tests/test-notify.t
+++ b/tests/test-notify.t
@@ -6,8 +6,15 @@ 
   > EOF
 
   $ cat <<EOF >> $HGRCPATH
+  > [experimental]
+  > evolution = true
+  > 
   > [extensions]
   > notify=
+  > strip=
+  > 
+  > [phases]
+  > publish=False
   > 
   > [hooks]
   > incoming.notify = python:hgext.notify.hook
@@ -15,6 +22,8 @@ 
   > [notify]
   > sources = pull
   > diffstat = False
+  > reply = True
+  > messageidseed = notifyseed
   > 
   > [usersubs]
   > foo@bar = *
@@ -205,7 +214,7 @@ 
   adding manifests
   adding file changes
   added 1 changesets with 2 changes to 2 files
-  new changesets 00a13f371396
+  new changesets 00a13f371396 (1 drafts)
   MIME-Version: 1.0
   Content-Type: text/plain; charset="us-ascii"
   Content-Transfer-Encoding: 7bit
@@ -266,7 +275,7 @@ 
   adding manifests
   adding file changes
   added 1 changesets with 2 changes to 2 files
-  new changesets 00a13f371396
+  new changesets 00a13f371396 (1 drafts)
   MIME-Version: 1.0
   Content-Type: text/plain; charset="us-ascii"
   Content-Transfer-Encoding: 7bit
@@ -316,7 +325,7 @@ 
   adding manifests
   adding file changes
   added 1 changesets with 2 changes to 2 files
-  new changesets 00a13f371396
+  new changesets 00a13f371396 (1 drafts)
   MIME-Version: 1.0
   Content-Type: text/plain; charset="us-ascii"
   Content-Transfer-Encoding: 7bit
@@ -369,7 +378,7 @@ 
   adding manifests
   adding file changes
   added 2 changesets with 0 changes to 0 files
-  new changesets 3332653e1f3c:fccf66cd0c35
+  new changesets 3332653e1f3c:fccf66cd0c35 (2 drafts)
   MIME-Version: 1.0
   Content-Type: text/plain; charset="us-ascii"
   Content-Transfer-Encoding: 7bit
@@ -436,7 +445,7 @@ 
   adding manifests
   adding file changes
   added 1 changesets with 1 changes to 1 files
-  new changesets 0f25f9c22b4c
+  new changesets 0f25f9c22b4c (1 drafts)
   MIME-Version: 1.0
   Content-Type: text/plain; charset="us-ascii"
   Content-Transfer-Encoding: 8bit
@@ -480,7 +489,7 @@ 
   adding manifests
   adding file changes
   added 1 changesets with 1 changes to 1 files
-  new changesets a846b5f6ebb7
+  new changesets a846b5f6ebb7 (1 drafts)
   notify: sending 2 subscribers 1 changes
   (run 'hg update' to get a working copy)
   $ cat b/mbox | "$PYTHON" $TESTDIR/unwrap-message-id.py | "$PYTHON" $TESTTMP/filter.py
@@ -493,7 +502,7 @@ 
   Subject: long line
   From: test@test.com
   X-Hg-Notification: changeset a846b5f6ebb7
-  Message-Id: <hg.a846b5f6ebb7.*.*@*> (glob)
+  Message-Id: <hg.e7dc7658565793ff33c797e72b7d1f3799347b042af3c40df6d17c8d5c3e560a@test.com>
   To: baz@test.com, foo@bar
   
   changeset a846b5f6ebb7 in b
@@ -543,6 +552,8 @@ 
   (branches are permanent and global, did you want a bookmark?)
   $ echo a >> a/a
   $ hg --cwd a ci -m test -d '1 0'
+  $ echo a >> a/a
+  $ hg --cwd a ci -m test -d '1 0'
   $ hg --traceback --cwd b pull ../a | \
   >  "$PYTHON" $TESTDIR/unwrap-message-id.py | \
   >  "$PYTHON" $TESTTMP/filter.py
@@ -551,8 +562,8 @@ 
   adding changesets
   adding manifests
   adding file changes
-  added 1 changesets with 1 changes to 1 files
-  new changesets f7e5aaed4080
+  added 2 changesets with 2 changes to 1 files
+  new changesets f7e5aaed4080:485bf79b9464 (2 drafts)
   MIME-Version: 1.0
   Content-Type: text/plain; charset="us-ascii"
   Content-Transfer-Encoding: 7bit
@@ -561,11 +572,24 @@ 
   Subject: test
   From: test@test.com
   X-Hg-Notification: changeset f7e5aaed4080
-  Message-Id: <hg.f7e5aaed4080.*.*@*> (glob)
+  Message-Id: <hg.12e9ae631e2529e9cfbe7a93be0dd8a401280700640f802a60f20d7be659251d@test.com>
   To: baz@test.com, foo@bar, notify@example.com
   
   changeset f7e5aaed4080 in b
   description: test
+  MIME-Version: 1.0
+  Content-Type: text/plain; charset="us-ascii"
+  Content-Transfer-Encoding: 7bit
+  X-Test: foo
+  Date: * (glob)
+  Subject: test
+  From: test@test.com
+  X-Hg-Notification: changeset 485bf79b9464
+  Message-Id: <hg.15281d60c27d9d5fb70435d33ebc24cb5aa580f2535988dcb9923c26e8bc5c47@test.com>
+  To: baz@test.com, foo@bar, notify@example.com
+  
+  changeset 485bf79b9464 in b
+  description: test
   (run 'hg update' to get a working copy)
 
 revset selection: don't send to address that waits for mails
@@ -584,7 +608,7 @@ 
   adding manifests
   adding file changes
   added 1 changesets with 0 changes to 0 files (+1 heads)
-  new changesets 645eb6690ecf
+  new changesets 645eb6690ecf (1 drafts)
   MIME-Version: 1.0
   Content-Type: text/plain; charset="us-ascii"
   Content-Transfer-Encoding: 7bit
@@ -593,7 +617,7 @@ 
   Subject: test
   From: test@test.com
   X-Hg-Notification: changeset 645eb6690ecf
-  Message-Id: <hg.645eb6690ecf.*.*@*> (glob)
+  Message-Id: <hg.ba26b2c63e7deb44e86c934aeea147edde12a11b6ac94bda103dcab5028dc928@test.com>
   To: baz@test.com, foo@bar
   
   changeset 645eb6690ecf in b
@@ -616,7 +640,7 @@ 
   Subject: changeset in b: default template
   From: test@test.com
   X-Hg-Notification: changeset 5cd4346eed47
-  Message-Id: <hg.5cd4346eed47.*.*@*> (glob)
+  Message-Id: <hg.8caa7941b24fc673d10910cb072e2d167362a3c5111cafefa47190d9b831f0a3@test.com>
   To: baz@test.com, foo@bar
   
   changeset 5cd4346eed47 in $TESTTMP/b
@@ -647,7 +671,7 @@ 
   Subject: with style
   From: test@test.com
   X-Hg-Notification: changeset ec8d9d852f56
-  Message-Id: <hg.ec8d9d852f56.*.*@*> (glob)
+  Message-Id: <hg.ccd5049818a6a277251189ce1d6d0cca10723d58214199e7178894adb99ed918@test.com>
   To: baz@test.com, foo@bar
   
   changeset ec8d9d852f56
@@ -672,7 +696,7 @@ 
   Subject: 14721b538ae3: with template
   From: test@test.com
   X-Hg-Notification: changeset 14721b538ae3
-  Message-Id: <hg.14721b538ae3.*.*@*> (glob)
+  Message-Id: <hg.7edb9765307a5a24528f3964672e794e2d21f2479e96c099bf52e02abd17b3a2@test.com>
   To: baz@test.com, foo@bar
   
   with template
@@ -695,6 +719,8 @@ 
   > EOF
   $ hg commit -Am addfunction
   adding f1
+  $ hg debugobsolete eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee b86bc16ff894f057d023b306936f290954857187
+  1 new obsolescence markers
   $ hg --cwd ../b pull ../a | \
   >  "$PYTHON" $TESTDIR/unwrap-message-id.py
   pulling from ../a
@@ -703,7 +729,8 @@ 
   adding manifests
   adding file changes
   added 1 changesets with 1 changes to 1 files
-  new changesets b86bc16ff894
+  1 new obsolescence markers
+  new changesets b86bc16ff894 (1 drafts)
   MIME-Version: 1.0
   Content-Type: text/plain; charset="us-ascii"
   Content-Transfer-Encoding: 7bit
@@ -711,7 +738,7 @@ 
   Subject: addfunction
   From: test@test.com
   X-Hg-Notification: changeset b86bc16ff894
-  Message-Id: <hg.b86bc16ff894.*.*@*> (glob)
+  Message-Id: <hg.4c7cacfbbd6ba170656be0c8fc0d7599bd925c0d545b836816be9983e6d08448@test.com>
   To: baz@test.com, foo@bar
   
   changeset b86bc16ff894
@@ -739,6 +766,9 @@ 
   > }
   > EOF
   $ hg commit -m changefunction
+  $ hg debugobsolete 485bf79b9464197b2ed2debd0b16252ad64ed458 e81040e9838c704d8bf17658cb11758f24e40b6b
+  1 new obsolescence markers
+  obsoleted 1 changesets
   $ hg --cwd ../b --config notify.showfunc=True pull ../a | \
   >  "$PYTHON" $TESTDIR/unwrap-message-id.py
   pulling from ../a
@@ -747,7 +777,9 @@ 
   adding manifests
   adding file changes
   added 1 changesets with 1 changes to 1 files
-  new changesets e81040e9838c
+  1 new obsolescence markers
+  obsoleted 1 changesets
+  new changesets e81040e9838c (1 drafts)
   MIME-Version: 1.0
   Content-Type: text/plain; charset="us-ascii"
   Content-Transfer-Encoding: 7bit
@@ -755,7 +787,8 @@ 
   Subject: changefunction
   From: test@test.com
   X-Hg-Notification: changeset e81040e9838c
-  Message-Id: <hg.e81040e9838c.*.*@*> (glob)
+  Message-Id: <hg.99b80bf1c5d0bf8f8a7e60107c1aa1da367a5943b2a70a8b36517d701557edff@test.com>
+  In-Reply-To: <hg.15281d60c27d9d5fb70435d33ebc24cb5aa580f2535988dcb9923c26e8bc5c47@test.com>
   To: baz@test.com, foo@bar
   
   changeset e81040e9838c
@@ -774,3 +807,50 @@ 
   +    return a + b + c + e;
    }
   (run 'hg update' to get a working copy)
+
+Retry the In-Reply-To, but make sure the oldest known change is older.
+This can happen when folding commits that have been rebased by another user.
+
+  $ hg --cwd ../b strip tip
+  saved backup bundle to $TESTTMP/b/.hg/strip-backup/e81040e9838c-10aad4de-backup.hg
+  $ hg debugobsolete f7e5aaed408029cfe9890318245e87ef44739fdd e81040e9838c704d8bf17658cb11758f24e40b6b
+  1 new obsolescence markers
+  obsoleted 1 changesets
+  $ hg --cwd ../b --config notify.showfunc=True pull ../a | \
+  >  "$PYTHON" $TESTDIR/unwrap-message-id.py
+  pulling from ../a
+  searching for changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 1 changes to 1 files
+  2 new obsolescence markers
+  obsoleted 2 changesets
+  new changesets e81040e9838c (1 drafts)
+  MIME-Version: 1.0
+  Content-Type: text/plain; charset="us-ascii"
+  Content-Transfer-Encoding: 7bit
+  Date: * (glob)
+  Subject: changefunction
+  From: test@test.com
+  X-Hg-Notification: changeset e81040e9838c
+  Message-Id: <hg.99b80bf1c5d0bf8f8a7e60107c1aa1da367a5943b2a70a8b36517d701557edff@test.com>
+  In-Reply-To: <hg.12e9ae631e2529e9cfbe7a93be0dd8a401280700640f802a60f20d7be659251d@test.com>
+  To: baz@test.com, foo@bar
+  
+  changeset e81040e9838c
+  diffs (12 lines):
+  
+  diff -r b86bc16ff894 -r e81040e9838c f1
+  --- a/f1	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/f1	Thu Jan 01 00:00:00 1970 +0000
+  @@ -2,6 +2,6 @@ int main() {
+       int a = 0;
+       int b = 1;
+       int c = 2;
+  -    int d = 3;
+  -    return a + b + c + d;
+  +    int e = 3;
+  +    return a + b + c + e;
+   }
+  (run 'hg update' to get a working copy)
diff --git a/hgext/notify.py b/hgext/notify.py
--- a/hgext/notify.py
+++ b/hgext/notify.py
@@ -160,6 +160,7 @@ 
     error,
     logcmdutil,
     mail,
+    obsutil,
     patch,
     pycompat,
     registrar,
@@ -219,6 +220,9 @@ 
     b'notify', b'outgoing', default=None,
 )
 configitem(
+    b'notify', b'reply', default=False,
+)
+configitem(
     b'notify', b'sources', default=b'serve',
 )
 configitem(
@@ -275,6 +279,7 @@ 
         self.root = self.strip(self.repo.root)
         self.domain = self.ui.config(b'notify', b'domain')
         self.mbox = self.ui.config(b'notify', b'mbox')
+        self.reply = self.ui.configbool(b'notify', b'reply')
         self.test = self.ui.configbool(b'notify', b'test')
         self.charsets = mail._charsets(self.ui)
         self.subs = self.subscribers()
@@ -437,6 +442,19 @@ 
         msg['X-Hg-Notification'] = 'changeset %s' % ctx
         if not msg['Message-Id']:
             msg['Message-Id'] = messageid(ctx, self.domain, self.messageidseed)
+        if self.reply:
+            unfi = self.repo.unfiltered()
+            has_node = unfi.changelog.index.has_node
+            predecessors = list(
+                unfi[ctx2]
+                for ctx2 in obsutil.allpredecessors(unfi.obsstore, [ctx.node()])
+                if ctx2 != ctx.node() and has_node(ctx2)
+            )
+            if predecessors:
+                pred = min(predecessors, key=lambda ctx: ctx.rev())
+                msg['In-Reply-To'] = messageid(
+                    pred, self.domain, self.messageidseed
+                )
         msg['To'] = ', '.join(sorted(subs))
 
         msgtext = msg.as_bytes() if pycompat.ispy3 else msg.as_string()