This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6882

histedit: sniff-test for untracked file conflicts before prompting for rules
ClosedPublic

Authored by durin42 on Sep 25 2019, 1:54 PM.

Download Raw Diff

Details

Reviewers

None

Group Reviewers

hg-reviewers

Commits

rHG699102b10530: histedit: sniff-test for untracked file conflicts before prompting for rules

Summary

This bug is as old as histedit, which is more than 10 years! I'm a
little sad about the extra calculations here that we're just going to
throw out, but I don't see any better way to look for untracked file
conflicts and I want the bug fixed.

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

durin42 created this revision.Sep 25 2019, 1:54 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptSep 25 2019, 1:54 PM

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

martinvonz added a subscriber: martinvonz.Sep 25 2019, 2:03 PM

martinvonz added inline comments.

hgext/histedit.py
1974–1993	Would it be enough to check if any untracked file is in any of those commits? That should be a lot simpler and cheaper.

durin42 added inline comments.Sep 25 2019, 2:10 PM

hgext/histedit.py
1974–1993	Maybe? The tricky bit would be if the user has a gazillion untracked or ignored files, since both of those can come into play.

martinvonz added inline comments.Sep 25 2019, 2:24 PM

hgext/histedit.py
1974–1993	Won't they (correctly) cause a conflict with your approach too?

durin42 added inline comments.Sep 25 2019, 2:34 PM

hgext/histedit.py
1974–1993	They'll correctly cause a conflict, but I trust the merge code to efficiently figure out what paths to look up, rather than (eg) enumerating a large build/ hierarchy that's wholly ignored.

martinvonz added inline comments.Sep 25 2019, 2:39 PM

hgext/histedit.py
1974–1993	Ah, now I see what you mean. There are also config options for `merge.checkunknown`, `merge.checkignored`, and `experimental.merge.checkpathconflicts` (and maybe others) that my proposal would miss. So I agree with doing it the way you have.
1989	could you pass these as keyword arguments?

durin42 marked 4 inline comments as done.Sep 25 2019, 2:42 PM

durin42 updated this revision to Diff 16618.

durin42 added inline comments.Sep 25 2019, 2:43 PM

hgext/histedit.py
1989	Maybe, but I'll do what happens at the other callsite. I also cleaned up the TODO, as I figured it out with print-debugging.

martinvonz added inline comments.Sep 25 2019, 5:04 PM

hgext/histedit.py
1975–1978	Does `None` (the default value for that argument) work? It seems like it should. If it does, then that seems preferable.
1987–1988	They all seem right to me. I don't think `followcopies` should make a difference for untracked/ignored files.

durin42 marked 2 inline comments as done.Sep 25 2019, 5:07 PM

durin42 added inline comments.

hgext/histedit.py
1975–1978	No, because that gives me the workingctx, and I need p1 of the workingctx. Again, I figured that out by printf debugging powers, it wasn't what I would have guessed (I actually expected `[c]` to be the right argument to `calculateupdates` - but the check didn't do what I needed until I was getting p1 of the workingctx like this. Sigh.)
1987–1988	Agreed. I'm just not wanting to provide a warranty that this makes sense in the future, thus the hedging.

martinvonz added inline comments.Sep 25 2019, 5:20 PM

hgext/histedit.py
1975–1978	Oh, I confused `calculateupdates()` arguments with `update()` -- `calculateupdates()` does not calculate a default ancestor, so you have to pass it in as you do here.

durin42 marked 2 inline comments as done.Sep 25 2019, 5:29 PM

durin42 added a commit: rHG699102b10530: histedit: sniff-test for untracked file conflicts before prompting for rules.

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

Closed by commit rHG699102b10530: histedit: sniff-test for untracked file conflicts before prompting for rules (authored by durin42). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			hgext/histedit.py (26 lines)
M			tests/test-histedit-non-commute-abort.t (45 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	16617		Sep 25 2019, 1:54 PM	★	★
Diff 2	16618		Sep 25 2019, 2:42 PM	★	★
Diff 3	16620	rHG699102b1053054d32bc9b0d0e8aeb699c20765d2	Sep 25 2019, 1:50 PM	★	★

	Status	Author	Revision
	Closed	durin42	D6882 histedit: sniff-test for untracked file conflicts before prompting for rules
	Closed	durin42	D6881 histedit: demonstrate breakage when `update` to a revision breaks

Diff 16620

hgext/histedit.py

	root = rr[0].node()			root = rr[0].node()

	revs = between(repo, root, topmost, state.keep)			revs = between(repo, root, topmost, state.keep)
	if not revs:			if not revs:
	raise error.Abort(_('%s is not an ancestor of working directory') %			raise error.Abort(_('%s is not an ancestor of working directory') %
	node.short(root))			node.short(root))

	ctxs = [repo[r] for r in revs]			ctxs = [repo[r] for r in revs]

				wctx = repo[None]
				# Please don't ask me why `ancestors` is this value. I figured it
				# out with print-debugging, not by actually understanding what the
				# merge code is doing. :(
				ancs = [repo['.']]
				martinvonzUnsubmitted Done Does `None` (the default value for that argument) work? It seems like it should. If it does, then that seems preferable. martinvonz: Does `None` (the default value for that argument) work? It seems like it should. If it does…
				durin42AuthorUnsubmitted Done No, because that gives me the workingctx, and I need p1 of the workingctx. Again, I figured that out by printf debugging powers, it wasn't what I would have guessed (I actually expected `[c]` to be the right argument to `calculateupdates` - but the check didn't do what I needed until I was getting p1 of the workingctx like this. Sigh.) durin42: No, because that gives me the workingctx, and I need p1 of the workingctx. Again, I figured…
				martinvonzUnsubmitted Not Done Oh, I confused `calculateupdates()` arguments with `update()` -- `calculateupdates()` does not calculate a default ancestor, so you have to pass it in as you do here. martinvonz: Oh, I confused `calculateupdates()` arguments with `update()` -- `calculateupdates()` does not…
				# Sniff-test to make sure we won't collide with untracked files in
				# the working directory. If we don't do this, we can get a
				# collision after we've started histedit and backing out gets ugly
				# for everyone, especially the user.
				for c in [ctxs[0].p1()] + ctxs:
				try:
				mergemod.calculateupdates(
				repo, wctx, c, ancs,
				# These parameters were determined by print-debugging
				# what happens later on inside histedit.
				martinvonzUnsubmitted Done They all seem right to me. I don't think `followcopies` should make a difference for untracked/ignored files. martinvonz: They all seem right to me. I don't think `followcopies` should make a difference for…
				durin42AuthorUnsubmitted Done Agreed. I'm just not wanting to provide a warranty that this makes sense in the future, thus the hedging. durin42: Agreed. I'm just not wanting to provide a warranty that this makes sense in the future, thus…
				False, # branchmerge
				martinvonzUnsubmitted Done could you pass these as keyword arguments? martinvonz: could you pass these as keyword arguments?
				durin42AuthorUnsubmitted Done Maybe, but I'll do what happens at the other callsite. I also cleaned up the TODO, as I figured it out with print-debugging. durin42: Maybe, but I'll do what happens at the other callsite. I also cleaned up the TODO, as I…
				False, # force
				False, # acceptremote
				False, # followcopies
				)
				martinvonzUnsubmitted Done Would it be enough to check if any untracked file is in any of those commits? That should be a lot simpler and cheaper. martinvonz: Would it be enough to check if any untracked file is in any of those commits? That should be a…
				durin42AuthorUnsubmitted Done Maybe? The tricky bit would be if the user has a gazillion untracked or ignored files, since both of those can come into play. durin42: Maybe? The tricky bit would be if the user has a gazillion untracked or ignored files, since…
				martinvonzUnsubmitted Done Won't they (correctly) cause a conflict with your approach too? martinvonz: Won't they (correctly) cause a conflict with your approach too?
				durin42AuthorUnsubmitted Done They'll correctly cause a conflict, but I trust the merge code to efficiently figure out what paths to look up, rather than (eg) enumerating a large build/ hierarchy that's wholly ignored. durin42: They'll correctly cause a conflict, but I trust the merge code to efficiently figure out what…
				martinvonzUnsubmitted Done Ah, now I see what you mean. There are also config options for `merge.checkunknown`, `merge.checkignored`, and `experimental.merge.checkpathconflicts` (and maybe others) that my proposal would miss. So I agree with doing it the way you have. martinvonz: Ah, now I see what you mean. There are also config options for `merge.checkunknown`, `merge.
				except error.Abort:
				raise error.Abort(
				_("untracked files in working directory conflict with files in %s") % (
				c))

	if not rules:			if not rules:
	comment = geteditcomment(ui, node.short(root), node.short(topmost))			comment = geteditcomment(ui, node.short(root), node.short(topmost))
	actions = [pick(state, r) for r in revs]			actions = [pick(state, r) for r in revs]
	rules = ruleeditor(repo, ui, actions, comment)			rules = ruleeditor(repo, ui, actions, comment)
	else:			else:
	rules = _readfile(ui, rules)			rules = _readfile(ui, rules)
	actions = parserules(rules, state)			actions = parserules(rules, state)
	warnverifyactions(ui, repo, actions, state, ctxs)			warnverifyactions(ui, repo, actions, state, ctxs)

tests/test-histedit-non-commute-abort.t

	\| summary: b			\| summary: b
	\|			\|
	o changeset: 0:cb9a9f314b8b			o changeset: 0:cb9a9f314b8b
	user: test			user: test
	date: Thu Jan 01 00:00:00 1970 +0000			date: Thu Jan 01 00:00:00 1970 +0000
	summary: a			summary: a


	Early tree conflict doesn't leave histedit in a wedged state.			Early tree conflict doesn't leave histedit in a wedged state. Note
				that we don't specify --commands here: we catch the problem before we
				even prompt the user for rules, sidestepping any dataloss issues.

	$ hg rm c			$ hg rm c
	$ hg ci -m 'remove c'			$ hg ci -m 'remove c'
	$ echo collision > c			$ echo collision > c

	$ hg histedit e860deea161a --commands - 2>&1 <<EOF			$ hg histedit e860deea161a
	> edit e860deea161a
	> pick 652413bf663e
	> pick bfa474341cc9
	> pick 1b0954ff00fc
	> EOF
	c: untracked file differs			c: untracked file differs
	abort: untracked files in working directory differ from files in requested revision			abort: untracked files in working directory conflict with files in 055a42cdd887
	[255]			[255]

	BUG: we didn't actually change p1 of the working copy, but we're in a			We should have detected the collision early enough we're not in a
	histedit state. This confuses the process very badly and leads to			histedit state, and p1 is unchanged.
	histedit stripping things it shouldn't (in obsmarker mode it inserts
	bogus prune markers in this case.)

	$ hg log -r 'p1()' -T'{node}\n'			$ hg log -r 'p1()' -T'{node}\n'
	1b0954ff00fccb15a37b679e4a35e9b01dfe685e			1b0954ff00fccb15a37b679e4a35e9b01dfe685e
	$ hg status --config ui.tweakdefaults=yes			$ hg status --config ui.tweakdefaults=yes
	? c			? c
	? e.orig			? e.orig
	# The repository is in an unfinished histedit state.

	# To continue: hg histedit --continue
	# To abort: hg histedit --abort

	$ hg histedit --continue
	652413bf663e: skipping changeset (no changes)
	bfa474341cc9: skipping changeset (no changes)
	1b0954ff00fc: skipping changeset (no changes)
	saved backup bundle to $TESTTMP/r/.hg/strip-backup/e860deea161a-a0738322-histedit.hg

	$ hg log -GTcompact
	warning: ignoring unknown working parent 1b0954ff00fc!
	o 3[tip] 055a42cdd887 1970-01-01 00:00 +0000 test
	\| d
	\|
	o 2 177f92b77385 1970-01-01 00:00 +0000 test
	\| c
	\|
	o 1 d2ae7f538514 1970-01-01 00:00 +0000 test
	\| b
	\|
	o 0 cb9a9f314b8b 1970-01-01 00:00 +0000 test
	a


	$ cd ..			$ cd ..