This is an archive of the discontinued Mercurial Phabricator instance.

mercurial/rewriteutil.py
65–72	I think we usually treat bad config as no config, i.e. use the default ("skip" in this case). You may want to check if I seem to be right about that.

mjacob added inline comments.Jul 14 2020, 12:26 PM

mercurial/rewriteutil.py
65–72	I found cases where an error was raised if an invalid value was used, e.g. https://phab.mercurial-scm.org/diffusion/HG/browse/default/mercurial/merge.py$605 I think that errors should never pass silently. I might sent patches (after the next release) to make it possible to enumerate all legal values in configitems and raise an error if another value was specified.

Alphare accepted this revision.Jul 15 2020, 11:26 AM

Alphare added a subscriber: Alphare.

Alphare added inline comments.

mercurial/rewriteutil.py
65–72	I also feel like this "bad config is no config" has only bitten me and never been a feature, I vote for the `ConfigError`.

martinvonz added a subscriber: durin42.Jul 15 2020, 12:27 PM

martinvonz added inline comments.

mercurial/rewriteutil.py
65–72	I also feel like this "bad config is no config" has only bitten me and never been a feature I agree. I was trying to think what the reason for ignoring bad config might be. I tried to think if backward- or forward-compatibility would explain it, but I can't think of a scenario where it makes sense to allow bad config. @durin42, do you know why we've mostly allowed bad config?

mjacob added a commit: rHGa391d0710f22: rewriteutil: add utility to check whether empty successors should be skipped.Jul 17 2020, 1:41 PM

pulkit accepted this revision.Jul 17 2020, 2:35 PM

This revision is now accepted and ready to land.Jul 17 2020, 2:35 PM

mjacob closed this revision.Jul 17 2020, 2:36 PM

Revision Contents
Changeset List

			Path	Packages
M			mercurial/rewriteutil.py (17 lines)

Commit	Parents	Author	Summary	Date
67082fc1c2bf	d87723e64c26	Manuel Jacob		Jul 12 2020, 12:06 AM

Status	Author	Revision
Closed	mjacob	D8749 absorb: make it explicit if empty changeset was created
Closed	mjacob	D8748 absorb: consider rewrite.empty-successor configuration
Closed	mjacob	D8747 rebase: consider rewrite.empty-successor configuration
Closed	mjacob	D8746 rewriteutil: add utility to check whether empty successors should be skipped
Closed	mjacob	D8745 config: add option to control creation of empty successors during rewrite

Diff 21898

mercurial/rewriteutil.py


	To allow new unstable changesets, set the config:			To allow new unstable changesets, set the config:
	`experimental.evolution.allowunstable=True`			`experimental.evolution.allowunstable=True`
	"""			"""
	allowunstable = obsolete.isenabled(repo, obsolete.allowunstableopt)			allowunstable = obsolete.isenabled(repo, obsolete.allowunstableopt)
	if allowunstable:			if allowunstable:
	return revset.baseset()			return revset.baseset()
	return repo.revs(b"(%ld::) - %ld", revs, revs)			return repo.revs(b"(%ld::) - %ld", revs, revs)


				def skip_empty_successor(ui, command):
				empty_successor = ui.config(b'rewrite', b'empty-successor')
				if empty_successor == b'skip':
				return True
				elif empty_successor == b'keep':
				return False
				else:
				raise error.ConfigError(
				_(
				b"%s doesn't know how to handle config "
				b"rewrite.empty-successor=%s (only 'skip' and 'keep' are "
				b"supported)"
				)
				% (command, empty_successor)
				)
				martinvonzUnsubmitted Not Done I think we usually treat bad config as no config, i.e. use the default ("skip" in this case). You may want to check if I seem to be right about that. martinvonz: I think we usually treat bad config as no config, i.e. use the default ("skip" in this case).
				mjacobAuthorUnsubmitted Done I found cases where an error was raised if an invalid value was used, e.g. https://phab.mercurial-scm.org/diffusion/HG/browse/default/mercurial/merge.py$605 I think that errors should never pass silently. I might sent patches (after the next release) to make it possible to enumerate all legal values in configitems and raise an error if another value was specified. mjacob: I found cases where an error was raised if an invalid value was used, e.g. https://phab.
				AlphareUnsubmitted Not Done I also feel like this "bad config is no config" has only bitten me and never been a feature, I vote for the `ConfigError`. Alphare: I also feel like this "bad config is no config" has only bitten me and never been a feature, I…
				martinvonzUnsubmitted Not Done I also feel like this "bad config is no config" has only bitten me and never been a feature I agree. I was trying to think what the reason for ignoring bad config might be. I tried to think if backward- or forward-compatibility would explain it, but I can't think of a scenario where it makes sense to allow bad config. @durin42, do you know why we've mostly allowed bad config? martinvonz: > I also feel like this "bad config is no config" has only bitten me and never been a feature…

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	21898			Jul 14 2020, 11:32 AM	★	★

This is an archive of the discontinued Mercurial Phabricator instance.

rewriteutil: add utility to check whether empty successors should be skippedClosedPublic

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 21898

mercurial/rewriteutil.py

rewriteutil: add utility to check whether empty successors should be skipped
ClosedPublic

Revision Contents
Changeset List