This is an archive of the discontinued Mercurial Phabricator instance.

notify: optional mail threading based on obsmarker
ClosedPublic

Authored by joerg.sonnenberger on Feb 26 2020, 4:35 PM.

Details

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.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

pulkit added a subscriber: pulkit.EditedMar 9 2020, 5:13 AM

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 used; and add info about that behavior in commit message?

durin42 removed a subscriber: durin42.Mar 11 2020, 11:40 AM

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

@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 :/

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

marmoute requested changes to this revision.Mar 19 2020, 8:00 PM

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

hgext/notify.py
136

maybe reply-to-predecessors would be clearer ?

137

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

139

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.

453–457

nits: could be [] instead of list()

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.

This revision now requires changes to proceed.Mar 19 2020, 8:00 PM
marmoute accepted this revision.Mar 27 2020, 10:08 AM

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

hgext/notify.py
459

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 ?

marmoute accepted this revision.Mar 29 2020, 1:33 PM
pulkit accepted this revision.Mar 30 2020, 6:50 AM
This revision is now accepted and ready to land.Mar 30 2020, 6:50 AM