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 ?

In D542#11973, @lothiraldan 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.

This revision now requires changes to proceed.Sep 15 2017, 11:55 PM

In D542#12108, @quark wrote:

In D542#11973, @lothiraldan 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.

In D542#13621, @martinvonz 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?

In D542#13779, @quark wrote:

In D542#13621, @martinvonz 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–324	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

Closed by commit rHG2fd06499dc8e: effectflag: document effect flag (authored by lothiraldan). · Explain WhyOct 1 2017, 5:20 PM

This revision was automatically updated to reflect the committed changes.

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	1346		Aug 28 2017, 7:03 AM	★	★
Diff 2	2113		Sep 27 2017, 4:08 AM	★	★
Diff 3	2259		Oct 1 2017, 7:52 AM	★	★
Diff 4	2288		Oct 1 2017, 9:44 AM	★	★
Diff 5	2312	rHG2fd06499dc8e6a5a784b1334b925c289d7b54e4e	Aug 24 2017, 12:40 PM	★	★

	plen = len(foreground)			plen = len(foreground)
	succs = set(c.node() for c in foreground)			succs = set(c.node() for c in foreground)
	mutable = [c.node() for c in foreground if c.mutable()]			mutable = [c.node() for c in foreground if c.mutable()]
	succs.update(allsuccessors(repo.obsstore, mutable))			succs.update(allsuccessors(repo.obsstore, mutable))
	known = (n for n in succs if n in nm)			known = (n for n in succs if n in nm)
	foreground = set(repo.set('%ln::', known))			foreground = set(repo.set('%ln::', known))
	return set(c.node() for c in foreground)			return set(c.node() for c in foreground)

	# logic around storing and using effect flags			# effectflag field
				#
				# Effect-flag is a 1-byte bit field used to store what changed between a
				# changeset and its successor(s).
				#
				# The effect flag is stored in obs-markers metadata while we iterate on the
				# information design. That's why we have the EFFECTFLAGFIELD. If we come up
				# with an incompatible design for effect flag, we can store a new design under
				# another field name so we don't break readers. We plan to extend the existing
				# obsmarkers bit-field when the effect flag design will be stabilized.
				#
				# The effect-flag is placed behind an experimental flag
				# `effect-flags` set to off by default.
				#

				martinvonzUnsubmitted Done The extra leading "#' seems unusual. I suspect you didn't add it on purpose martinvonz: The extra leading "#' seems unusual. I suspect you didn't add it on purpose
	EFFECTFLAGFIELD = "ef1"			EFFECTFLAGFIELD = "ef1"

	DESCCHANGED = 1 << 0 # action changed the description			DESCCHANGED = 1 << 0 # action changed the description
	METACHANGED = 1 << 1 # action change the meta			METACHANGED = 1 << 1 # action change the meta
	DIFFCHANGED = 1 << 3 # action change diff introduced by the changeset			DIFFCHANGED = 1 << 3 # action change diff introduced by the changeset
	PARENTCHANGED = 1 << 2 # action change the parent			PARENTCHANGED = 1 << 2 # action change the parent
	USERCHANGED = 1 << 4 # the user changed			USERCHANGED = 1 << 4 # the user changed
	DATECHANGED = 1 << 5 # the date changed			DATECHANGED = 1 << 5 # the date changed

This is an archive of the discontinued Mercurial Phabricator instance.

effectflag: document effect flag
ClosedPublic

Details

Diff Detail

Event Timeline

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.

Revision Contents
Changeset List

Diff 2312

mercurial/obsutil.py