This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9697

shelve: rewrite check for unknown shelf to delete
ClosedPublic

Authored by martinvonz on Jan 8 2021, 3:36 PM.

Download Raw Diff

Details

Reviewers

pulkit

Group Reviewers

hg-reviewers

Commits

rHG161313f9c467: shelve: rewrite check for unknown shelf to delete
rHG1d492b8e7b98: shelve: rewrite check for unknown shelf to delete
rHG640b1bfef7b5: shelve: rewrite check for unknown shelf to delete

Summary

The code would try to delete the shelf's .patch file and if that
raised an exception, it would convert it to an error.Abort. This
patch rewrites it so the check is done upfront. I find it easier to
read that way. It's now clear enough that I removed the comment
explaining it as well.

As Joerg pointed out during review, another differences is that the
old code would move a .hg file without its .patch friend to backup
before it realized that the .patch file was missing. The new code
will error out earlier and not move the .hg file, which seems like
an improvement. That should only matter on corrupt .hg/shelved/
directories, however.

Diff Detail

Repository

rHG Mercurial

Branch

default

Lint

No Linters Available

Unit

No Unit Test Coverage

Event Timeline

martinvonz created this revision.Jan 8 2021, 3:36 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptJan 8 2021, 3:36 PM

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

martinvonz added a child revision: D9698: shelve: trust caller of shelvedfile.opener() to check that the file exists.Jan 8 2021, 3:36 PM

The old code would check if a shelve file exists or has the patch extension. If so, it would create the backup directory and then try to move it to the backup directory. Non-existing file errors would be turned into error.Abort.
The new code checks for the existence of the patch file extension first and then tries to move all existing shelve files to the (potentially created) backup directory.

I don't think your description covers the #$%#$% of the old code.

In D9697#146558, @joerg.sonnenberger wrote:

The old code would check if a shelve file exists or has the patch extension. If so, it would create the backup directory and then try to move it to the backup directory. Non-existing file errors would be turned into error.Abort.
The new code checks for the existence of the patch file extension first and then tries to move all existing shelve files to the (potentially created) backup directory.
I don't think your description covers the #$%#$% of the old code.

I *think* you're joking that the old code is bad in ways I didn't describe and you're not asking for any actual changes to the commit message, but let me know if I misread your comment :)

Well, there are two important differences:
(1) The backup directory was always created before.
(2) The .hg file was moved before and we bail out before moving the .patch file.

So the description is wrong at least in so far as s,delete,move to backup, but it might be useful to mention the second part as well. Maybe like:
"A shelf could have been broken by manual removal of the .patch file. The old code would move the .hg file if it exists and then bail out when it discovers the missing .patch file. Change this to check for the patch before doing any modifications"

In D9697#146565, @joerg.sonnenberger wrote:

Well, there are two important differences:
(1) The backup directory was always created before.
(2) The .hg file was moved before and we bail out before moving the .patch file.
So the description is wrong at least in so far as s,delete,move to backup, but it might be useful to mention the second part as well. Maybe like:
"A shelf could have been broken by manual removal of the .patch file. The old code would move the .hg file if it exists and then bail out when it discovers the missing .patch file. Change this to check for the patch before doing any modifications"

Ah, good point that the old code moved the .hg file to backup even if there was no .patch file. I agree that my patch improves it by not making changes before checking that the .patch file exists. However, that would only matter if .hg/ was corrupt, and, as I said elsewhere, I don't think we care much how the tool behaves on a corrupt repo (I personally care very little). I'll update the description to mention it, though.

martinvonz edited the summary of this revision. (Show Details)Jan 8 2021, 6:03 PM

martinvonz updated this revision to Diff 24704.Jan 11 2021, 1:04 PM

pulkit accepted this revision.Jan 16 2021, 4:12 AM

This revision is now accepted and ready to land.Jan 16 2021, 4:12 AM

martinvonz added a commit: rHG640b1bfef7b5: shelve: rewrite check for unknown shelf to delete.Jan 16 2021, 5:56 AM

Closed by commit rHG640b1bfef7b5: shelve: rewrite check for unknown shelf to delete (authored by martinvonz). · Explain Why

This revision was automatically updated to reflect the committed changes.

Starting with this patch, I have failure on the following test both on CI and locally:

Failed test-shelve2.t#phasebased#abortcommand#continuecommand: output changed
Failed test-shelve2.t#phasebased#abortcommand#continueflag: output changed
Failed test-shelve2.t#phasebased#abortflag#continuecommand: output changed
Failed test-shelve2.t#phasebased#abortflag#continueflag: output changed
Failed test-shelve2.t#stripbased#abortcommand#continuecommand: output changed
Failed test-shelve2.t#stripbased#abortcommand#continueflag: output changed
Failed test-shelve2.t#stripbased#abortflag#continuecommand: output changed
Failed test-shelve2.t#stripbased#abortflag#continueflag: output changed

https://foss.heptapod.net/octobus/mercurial-devel/-/jobs/152595

In D9697#148250, @marmoute wrote:

Starting with this patch, I have failure on the following test both on CI and locally:

Failed test-shelve2.t#phasebased#abortcommand#continuecommand: output changed
Failed test-shelve2.t#phasebased#abortcommand#continueflag: output changed
Failed test-shelve2.t#phasebased#abortflag#continuecommand: output changed
Failed test-shelve2.t#phasebased#abortflag#continueflag: output changed
Failed test-shelve2.t#stripbased#abortcommand#continuecommand: output changed
Failed test-shelve2.t#stripbased#abortcommand#continueflag: output changed
Failed test-shelve2.t#stripbased#abortflag#continuecommand: output changed
Failed test-shelve2.t#stripbased#abortflag#continueflag: output changed

https://foss.heptapod.net/octobus/mercurial-devel/-/jobs/152595

Thanks for letting me know and sorry about the failure. It actually started with D9718, and it stops failing with D9720. The problem was that the error was ValueError: not enough values to unpack (expected 2, got 1) on py3 and ValueError: need more than 1 value to unpack on py2. Since it only affected py2 and only failed for a while during the series, I don't think it's a big deal, but I'll fix it anyway since you reported it. I'll rewrite the queued commits myself. I'll use ValueError: * (glob).

martinvonz added a commit: rHG1d492b8e7b98: shelve: rewrite check for unknown shelf to delete.Jan 16 2021, 1:31 PM

In D9697#148253, @martinvonz wrote:

Since it only affected py2 and only failed for a while during the series, I don't think it's a big deal, but I'll fix it anyway since you reported it.

Am not sure what you are talking about since the like I provide show failure on tip (at that time) of the default branch for both python 2 *and* python 3.

That latest pipeline run stil show the same error, on the same tests, for both python2 and python3. And I can reproduce them locally.

https://foss.heptapod.net/octobus/mercurial-devel/-/pipelines/16590

(It also show a handful of other error that are solved by: D9640 and D9789)

This is now the one thing that pervent CI to be green

https://foss.heptapod.net/octobus/mercurial-devel/-/pipelines/16607

In D9697#148302, @marmoute wrote:

In D9697#148253, @martinvonz wrote:

Since it only affected py2 and only failed for a while during the series, I don't think it's a big deal, but I'll fix it anyway since you reported it.

Am not sure what you are talking about since the like I provide show failure on tip (at that time) of the default branch for both python 2 *and* python 3.

I checked out the commit you said it was failing at and ran tests there. It failed for me on py2 (with the different ValueError message I mentioned) there, so I assumed that that was the failure found by CI. Apparently I had two compatibility errors in my test cases :( The second one (the one that CI found) was that find does not sort its output. I'll rewrite the queued commits (again), adding | sort to the find commands.

martinvonz added a commit: rHG161313f9c467: shelve: rewrite check for unknown shelf to delete.Jan 17 2021, 2:21 AM

Revision Contents
Changeset List

			Path	Packages
M			mercurial/shelve.py (18 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	24653		Jan 8 2021, 3:36 PM	★	★
Diff 2	24704		Jan 11 2021, 1:04 PM	★	★
Diff 3	24932	rHG640b1bfef7b5917a0f61719a5fecee49eebd686a	Jan 7 2021, 3:22 PM	★	★

Commit	Parents	Author	Summary	Date
2756b6bfc02f	de33fd7dc75e	Martin von Zweigbergk		Jan 7 2021, 3:22 PM

Status	Author	Revision
Closed	martinvonz	D9744 shelve: move listshelves() to new ShelfDir class, so caller need not pass vfs
Closed	martinvonz	D9743 shelve: also create class representing whole directory of shelves
Closed	martinvonz	D9742 shelve: add a method for deleting shelf to new shelf class
Closed	martinvonz	D9741 shelve: inline ".patch" constant now that it's only used in the Shelf class
Closed	martinvonz	D9740 shelve: use listshelves() in cleanupoldbackups()
Closed	martinvonz	D9739 shelve: make listshelves() list shelves in a given vfs
Closed	martinvonz	D9738 shelve: replace repo instance in Shelf class by vfs instance
Closed	martinvonz	D9737 shelve: use listdir() instead of readdir() when we don't need stat information
Closed	martinvonz	D9720 shelve: don't crash on file with unexpected extension in .hg/shelved/
Closed	martinvonz	D9719 shelve: don't include invalid shelves in `hg shelve --list`
Closed	martinvonz	D9714 shelve: extract some repeated creation of shelf instances to variables
Closed	martinvonz	D9713 shelve: teach new shelf class to check if .shelve file exists
Closed	martinvonz	D9712 shelve: move method for creating backup to new shelf class
Closed	martinvonz	D9711 shelve: make gennames() helper generate relative backup paths
Closed	martinvonz	D9710 shelve: use listshelves() in cleanup function
Closed	martinvonz	D9709 shelve: inline shelvedfile.filename() since there are no callers outside class
Closed	martinvonz	D9708 shelve: make listshelves() return shelf names instead of filenames
Closed	martinvonz	D9707 shelve: move method for getting stat (mtime) to new shelf class
Closed	martinvonz	D9706 shelve: open patch using new shelf class instead of open()
Closed	martinvonz	D9705 shelve: move function for opening .patch file to new shelf class
Closed	martinvonz	D9704 shelve: move method for reading .hg to new shelf class
Closed	martinvonz	D9703 shelve: move method for writing bundle to new shelf class
Closed	martinvonz	D9702 shelve: move method for reading .shelve file to new shelf class
Closed	martinvonz	D9701 shelve: move method for writing .shelve to new shelf class
Closed	martinvonz	D9700 shelve: introduce class representing a shelf
Closed	martinvonz	D9699 shelve: raise more specific errors
Closed	martinvonz	D9698 shelve: trust caller of shelvedfile.opener() to check that the file exists
Closed	martinvonz	D9697 shelve: rewrite check for unknown shelf to delete
Closed	martinvonz	D9696 shelve: remove a bundlerepo method
Closed	martinvonz	D9718 tests: add tests for corrupt .hg/shelved/ directory

Diff 24653

mercurial/shelve.py



	def deletecmd(ui, repo, pats):			def deletecmd(ui, repo, pats):
	"""subcommand that deletes a specific shelve"""			"""subcommand that deletes a specific shelve"""
	if not pats:			if not pats:
	raise error.Abort(_(b'no shelved changes specified!'))			raise error.Abort(_(b'no shelved changes specified!'))
	with repo.wlock():			with repo.wlock():
	for name in pats:			for name in pats:
	try:			if not shelvedfile(repo, name, patchextension).exists():
				raise error.Abort(_(b"shelved change '%s' not found") % name)
	for suffix in shelvefileextensions:			for suffix in shelvefileextensions:
	shfile = shelvedfile(repo, name, suffix)			shfile = shelvedfile(repo, name, suffix)
	# patch file is necessary, as it should			if shfile.exists():
	# be present for any kind of shelve,
	# but the .hg file is optional as in future we
	# will add obsolete shelve with does not create a
	# bundle
	if shfile.exists() or suffix == patchextension:
	shfile.movetobackup()			shfile.movetobackup()
	except OSError as err:
	if err.errno != errno.ENOENT:
	raise
	raise error.Abort(_(b"shelved change '%s' not found") % name)
	cleanupoldbackups(repo)			cleanupoldbackups(repo)


	def listshelves(repo):			def listshelves(repo):
	"""return all shelves in repo as list of (time, filename)"""			"""return all shelves in repo as list of (time, filename)"""
	try:			try:
	names = repo.vfs.readdir(shelvedir)			names = repo.vfs.readdir(shelvedir)
	except OSError as err:			except OSError as err: