This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8725

rebase: remove now unnecessary logic to allow empty commit when branch changes
ClosedPublic

Authored by mjacob on Jul 10 2020, 6:49 AM.

Download Raw Diff

Details

Reviewers

martinvonz

Group Reviewers

hg-reviewers

Commits

rHGa0192a03216d: rebase: remove now unnecessary logic to allow empty commit when branch changes

Summary

This was a workaround for a bug in the empty commit check in repo.commit(),
where the parent branch name was incorrectly compared with the wdir branch name
instead of the branch name passed via extra. The bug was fixed in D8724.

The workaround was introduced in b2415e94b2f5.

Diff Detail

Repository

rHG Mercurial

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mjacob created this revision.Jul 10 2020, 6:49 AM

Herald added a reviewer: martinvonz. · View Herald TranscriptJul 10 2020, 6:49 AM

Herald added a reviewer: hg-reviewers. · View Herald Transcript

Herald added a subscriber: mercurial-patches. · View Herald Transcript

Could you include in the commit message which commit added the code and which commit made it unnecessary? "the previous patch/commit" is an okay way of saying that , IMO, even though it requires the reviewer to queue the two patches right after each other for the description to make sense.

In D8725#130160, @martinvonz wrote:

Could you include in the commit message which commit added the code and which commit made it unnecessary? "the previous patch/commit" is an okay way of saying that , IMO, even though it requires the reviewer to queue the two patches right after each other for the description to make sense.

In this case, I could use the Differential revision (D8724) to refer to the other patch. Would that also make sense? (I wonder why you didn’t mention this possibility — is it because the repository should be self-contained?)

In D8725#130162, @mjacob wrote:

In D8725#130160, @martinvonz wrote:

Could you include in the commit message which commit added the code and which commit made it unnecessary? "the previous patch/commit" is an okay way of saying that , IMO, even though it requires the reviewer to queue the two patches right after each other for the description to make sense.

In this case, I could use the Differential revision (D8724) to refer to the other patch. Would that also make sense? (I wonder why you didn’t mention this possibility — is it because the repository should be self-contained?)

Yes, but the D8724 will get recorded, so I think that's actually a good way to refer to it. I think we have done that many times before.

mjacob mentioned this in D8724: commit: check branch change on to-be-committed changeset.Jul 10 2020, 5:55 PM

mjacob edited the summary of this revision. (Show Details)

mjacob added a child revision: D8727: rebase: add test case that shows that inmemory rebase does not preserve merges.Jul 13 2020, 5:05 PM

mjacob mentioned this in D8727: rebase: add test case that shows that inmemory rebase does not preserve merges.Jul 13 2020, 5:12 PM

Manuel Jacob <me@manueljacob.de> mentioned this in rHG07b3166e94ca: commit: check branch change on to-be-committed changeset.Jul 14 2020, 12:52 AM

Manuel Jacob <me@manueljacob.de> added a commit: rHGa0192a03216d: rebase: remove now unnecessary logic to allow empty commit when branch changes.

This revision was not accepted when it landed; it landed in state Needs Review.

Closed by commit rHGa0192a03216d: rebase: remove now unnecessary logic to allow empty commit when branch changes (authored by Manuel Jacob <me@manueljacob.de>). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			hgext/rebase.py (3 lines)

Status	Author	Revision
Closed	mjacob	D8733 overlayworkingctx: remove unused `nofilechanges()` and `_compact()` methods
Closed	mjacob	D8732 rebase: correctly check for empty commit in in-memory mode
Closed	mjacob	D8731 memctx: make `parents()` return list of one element if it’s not a merge
Closed	mjacob	D8729 commit: factor out empty commit check to `basectx.isempty()`
Closed	mjacob	D8728 overlayworkingctx: rename misleadingly named `isempty()` method
Closed	mjacob	D8727 rebase: add test case that shows that inmemory rebase does not preserve merges
Closed	mjacob	D8725 rebase: remove now unnecessary logic to allow empty commit when branch changes
Closed	mjacob	D8724 commit: check branch change on to-be-committed changeset

Diff 21878

hgext/rebase.py

	if commitmsg is None:			if commitmsg is None:
	commitmsg = ctx.description()			commitmsg = ctx.description()
	date = self.date			date = self.date
	if date is None:			if date is None:
	date = ctx.date()			date = ctx.date()
	extra = {b'rebase_source': ctx.hex()}			extra = {b'rebase_source': ctx.hex()}
	for c in self.extrafns:			for c in self.extrafns:
	c(ctx, extra)			c(ctx, extra)
	keepbranch = self.keepbranchesf and repo[p1].branch() != ctx.branch()
	destphase = max(ctx.phase(), phases.draft)			destphase = max(ctx.phase(), phases.draft)
	overrides = {(b'phases', b'new-commit'): destphase}			overrides = {(b'phases', b'new-commit'): destphase}
	if keepbranch:
	overrides[(b'ui', b'allowemptycommit')] = True
	with repo.ui.configoverride(overrides, b'rebase'):			with repo.ui.configoverride(overrides, b'rebase'):
	if self.inmemory:			if self.inmemory:
	newnode = commitmemorynode(			newnode = commitmemorynode(
	repo,			repo,
	wctx=self.wctx,			wctx=self.wctx,
	extra=extra,			extra=extra,
	commitmsg=commitmsg,			commitmsg=commitmsg,
	editor=editor,			editor=editor,

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	21834		Jul 10 2020, 6:49 AM	★	★
Diff 2	21878	rHGa0192a03216d6f0d279aa17312e946a06776875d	Jul 9 2020, 1:00 AM	★	★