This is an archive of the discontinued Mercurial Phabricator instance.

effectflag: document effect flag
ClosedPublic

Authored by lothiraldan on Aug 28 2017, 7:03 AM.

Details

Summary

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

lothiraldan created this revision.Aug 28 2017, 7:03 AM
quark added a subscriber: quark.EditedAug 28 2017, 12:41 PM

As I commented earlier in an email thread. I'm against the series because it introduces redundant data and is incorrect in some cases (see the "if A and C has a same patch content" example below).

I think it's cleaner and more correct to just record a hash of patch to detect content change, so we will be able to detect undos:

A -- amend --> B -- unamend --> C

When asking for the difference between A and C, we can quickly answer "no change". With effectflag, it cannot get that answer.

I am also not sure if obsstore is the best place to store it. It could be some caching data associated to changelog nodes.

It seems to me that other chnages like meta, parent, user, data could just be calculated directly from changelog because changelog is fast. You may argue that obsstore may have nodes unknown to changelog but I don't think the end-user care about those nodes (since they cannot see the actual content of them).

@quark Thank you for reposting your comment here, it's helpful to not lose track
on all these discussions.

We've had this experiment in Evolve for a little while now and the users using it
are giving positive feedback. The next step is to upstream it in core but turned off by
default like the others experiments and gather data and feedback.

The effect-flag design poses indeed the limitation you're bringing up. I've updated the
first commit message to make it more obvious:
https://phab.mercurial-scm.org/D533

However, I feel that it's not in this experiment scope, even though it
overlaps with effect-flag on content change tracking.

I hope that with all our current experiments (operation, effect-flag) and
potential future ones (content hash), we will be able to find the best
way to store and display the obsolescence history, both for centralized
and decentralized workflows, because we do have the same goal: making the
obsolescence history understandable to users.

I propose that we start a separate experiment, so users could use it
separately from effect-flag. I would be happy to collaborate on a first
version of this experiment before the sprint and use the sprint to further
discuss the design, what do you think ?

quark requested changes to this revision.Sep 15 2017, 11:55 PM

@quark Thank you for reposting your comment here, it's helpful to not lose track
on all these discussions.
We've had this experiment in Evolve for a little while now and the users using it
are giving positive feedback. The next step is to upstream it in core but turned off by
default like the others experiments and gather data and feedback.

I think the upstream has a higher barrier about implementation details. A lot of fb extensions need to be partially or completely rewritten before going upstream. I'm not sure if evolve is somehow an exception. But it does not feel quite right especially because your approach has known hard-to-solve defect and there are known better alternatives.

[...]
I propose that we start a separate experiment, so users could use it
separately from effect-flag. I would be happy to collaborate on a first
version of this experiment before the sprint and use the sprint to further
discuss the design, what do you think ?

I'm not going to the sprint this time unfortunately. But if you have concrete examples where effect-flag is better in theory, I'd be happy to discuss them here.


Since you have admitted the defect without giving a solution, and did not response to the alternative I proposed, I'm still -1 on this series. I have marked it explicitly hopefully to make other reviewers' life easier.

I think what I proposed is not hard to implement (I can complete it in 3 work days), so if you can help implement it instead, that'll be great and I'll be happy to provide technical help for any difficulty you encounter.

This revision now requires changes to proceed.Sep 15 2017, 11:55 PM
In D542#12108, @quark wrote:

@quark Thank you for reposting your comment here, it's helpful to not lose track
on all these discussions.
We've had this experiment in Evolve for a little while now and the users using it
are giving positive feedback. The next step is to upstream it in core but turned off by
default like the others experiments and gather data and feedback.

I think the upstream has a higher barrier about implementation details. A lot of fb extensions need to be partially or completely rewritten before going upstream. I'm not sure if evolve is somehow an exception. But it does not feel quite right especially because your approach has known hard-to-solve defect and there are known better alternatives.

[...]
I propose that we start a separate experiment, so users could use it
separately from effect-flag. I would be happy to collaborate on a first
version of this experiment before the sprint and use the sprint to further
discuss the design, what do you think ?

I'm not going to the sprint this time unfortunately. But if you have concrete examples where effect-flag is better in theory, I'd be happy to discuss them here.

Since you have admitted the defect without giving a solution, and did not response to the alternative I proposed, I'm still -1 on this series. I have marked it explicitly hopefully to make other reviewers' life easier.
I think what I proposed is not hard to implement (I can complete it in 3 work days), so if you can help implement it instead, that'll be great and I'll be happy to provide technical help for any difficulty you encounter.

I've found the information from the effectflag that's used in "hg obslog" useful. As you've probably seen, I've also sent a patch for recording the operation by default (implemented by Durham once upon a time, I think). I don't mind seeing turning both of these features on by default and we can let time tell which of them users find most useful and keep only that, or we can decide to keep both. I was about to queue this series, but it uses the old [evolution] config section, so at least that will have to change before I can accept it.

I've found the information from the effectflag that's used in "hg obslog" useful. As you've probably seen, I've also sent a patch for recording the operation by default (implemented by Durham once upon a time, I think). I don't mind seeing turning both of these features on by default and we can let time tell which of them users find most useful and keep only that, or we can decide to keep both. I was about to queue this series, but it uses the old [evolution] config section, so at least that will have to change before I can accept it.

I also think that both information are useful in both distributed and centralized workflows. I will update the config and send new versions.

quark added a comment.Sep 26 2017, 2:14 PM

I've found the information from the effectflag that's used in "hg obslog" useful. As you've probably seen, I've also sent a patch for recording the operation by default (implemented by Durham once upon a time, I think). I don't mind seeing turning both of these features on by default and we can let time tell which of them users find most useful and keep only that, or we can decide to keep both. I was about to queue this series, but it uses the old [evolution] config section, so at least that will have to change before I can accept it.

According to https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-May/098173.html, I think you prefer not storing the information in obsstore. Have you changed mind or do you think it could be improved in the future?

In D542#13779, @quark wrote:

I've found the information from the effectflag that's used in "hg obslog" useful. As you've probably seen, I've also sent a patch for recording the operation by default (implemented by Durham once upon a time, I think). I don't mind seeing turning both of these features on by default and we can let time tell which of them users find most useful and keep only that, or we can decide to keep both. I was about to queue this series, but it uses the old [evolution] config section, so at least that will have to change before I can accept it.

According to https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-May/098173.html, I think you prefer not storing the information in obsstore. Have you changed mind or do you think it could be improved in the future?

I have not changed my mind. I think the *feature* is useful, so I'm happy to get this in. I don't buy the argument about needing to know the effect in changesets that are not known locally, so I think I'd be fine with changing this code in the future to calculate the effectflags on the fly. The effectflags are currently only used in "hg obslog", which is used much less frequently than "hg log", so I think we'd even be fine with not caching the values (so they're always calculated on the fly).

lothiraldan updated this revision to Diff 2113.Sep 27 2017, 4:08 AM

I've updated the series @martinvonz

martinvonz added inline comments.Sep 30 2017, 12:56 PM
mercurial/obsutil.py
310

The extra leading "#' seems unusual. I suspect you didn't add it on purpose

lothiraldan updated this revision to Diff 2259.Oct 1 2017, 7:52 AM
lothiraldan marked an inline comment as done.Oct 1 2017, 7:59 AM
lothiraldan updated this revision to Diff 2288.Oct 1 2017, 9:44 AM
This revision was automatically updated to reflect the committed changes.