This is an archive of the discontinued Mercurial Phabricator instance.

shelve: trust caller of shelvedfile.opener() to check that the file exists
ClosedPublic

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

Details

Summary

The only place we call shelvedfile.opener() is when we're about to
apply a bundle. The file should always exist. If it doesn't, the
.hg/ directory is corrupt and we don't provide any guarantees about
supporting corrupt repos (besides, telling the user that the shelve
doesn't exist when hg shelve --list lists it is not very helpful).

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

Testcase:

mkdir -p .hg/shelved
touch .hg/shelved/default-xxx.patch
hg shelve -l
hg unshelve default-xxx

It is listed and the error mode changed. I'm not against the change, but I think we should test for this case? Alternatively, "hg shelve -l" should test for both ".patch" and ".hg" to exist?

I generally like the rest of the series. I would recommend making "hg shelve -l" and "hg unshelve" tighter first by consider a shelf valid only if both .hg and .patch exist. The refactoring other works fine?

Testcase:

mkdir -p .hg/shelved
touch .hg/shelved/default-xxx.patch
hg shelve -l
hg unshelve default-xxx

It is listed and the error mode changed.

Right, that's a test for a corrupt .hg/ directory, and behaves just like I said in the parenthetical part. As I said, I think that we don't generally try hard to handle corrupt .hg/.

I'm not against the change, but I think we should test for this case? Alternatively, "hg shelve -l" should test for both ".patch" and ".hg" to exist?

That should be easier to do after this series. We could perhaps be nice and error out with something like "abort: shelve foo is corrupt" and tell the user to delete it (and make sure that hg unshelve --delete works on corrupt states like that).

I generally like the rest of the series. I would recommend making "hg shelve -l" and "hg unshelve" tighter first by consider a shelf valid only if both .hg and .patch exist. The refactoring other works fine?

Our messages crossed. I said in the other message that it would be easier to do it after this series. I don't consider it a regression to change the handling of corrupt state from "misleading" to "traceback". I'm fine with adding that change on top, if that's okay with you.

Big question for me would be what to do with junk entries. I would make "hg shelve -l" list them with a note to remove them manually, but otherwise reject interacting with them with shelve or unshelve. If that is the acceptable before, it seems better to adjust (and test) the semantic tightening first and then refactor the logic. If the intention is to allow removing the junk with "hg shelve -d", it seems easier to refactor first and adjust the behavior afterwards.

Big question for me would be what to do with junk entries. I would make "hg shelve -l" list them with a note to remove them manually, but otherwise reject interacting with them with shelve or unshelve. If that is the acceptable before, it seems better to adjust (and test) the semantic tightening first and then refactor the logic. If the intention is to allow removing the junk with "hg shelve -d", it seems easier to refactor first and adjust the behavior afterwards.

I consider it a new feature to better support corrupt entries. For example, before this series:

# Only .hg file does not get listed, but can be deleted (moved to backup).
$ touch .hg/shelved/junk.hg
$ hg shelve -l | grep junk
$ hg shelve -d junk
$ ls .hg/shelved/junk.hg
ls: cannot access '.hg/shelved/junk.hg': No such file or directory

# Only .patch file gets listed, but then `hg unshelve` says it doesn't exist. It can be successfully deleted.
$ touch .hg/shelved/junk.patch
$ hg shelve -l | grep junk
junk            (5s ago)
$ hg unshelve
unshelving change 'junk'
abort: shelved change 'junk' not found
$ hg shelve -d junk
$ hg shelve -l | grep junk
<empty>

# Other files in the directory lead to a crash and the user cannot recover without manually deleting the file.
$ touch .hg/shelved/junk
$ hg shelve -l
[...]
ValueError: not enough values to unpack (expected 2, got 1)
$ hg shelve -d junk
abort: shelved change 'junk' not found

I suspect code I'm removing in this patch was added before the higher-level check that we now have have (search for "not found" in the file). I don't think the intent was ever to support corrupt .hg/shelved/ directories.

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

Big question for me would be what to do with junk entries. I would make "hg shelve -l" list them with a note to remove them manually, but otherwise reject interacting with them with shelve or unshelve. If that is the acceptable before, it seems better to adjust (and test) the semantic tightening first and then refactor the logic. If the intention is to allow removing the junk with "hg shelve -d", it seems easier to refactor first and adjust the behavior afterwards.

I consider it a new feature to better support corrupt entries. For example, before this series:

# Only .hg file does not get listed, but can be deleted (moved to backup).
$ touch .hg/shelved/junk.hg
$ hg shelve -l | grep junk
$ hg shelve -d junk
$ ls .hg/shelved/junk.hg
ls: cannot access '.hg/shelved/junk.hg': No such file or directory
# Only .patch file gets listed, but then `hg unshelve` says it doesn't exist. It can be successfully deleted.
$ touch .hg/shelved/junk.patch
$ hg shelve -l | grep junk
junk            (5s ago)
$ hg unshelve
unshelving change 'junk'
abort: shelved change 'junk' not found
$ hg shelve -d junk
$ hg shelve -l | grep junk
<empty>
# Other files in the directory lead to a crash and the user cannot recover without manually deleting the file.
$ touch .hg/shelved/junk
$ hg shelve -l
[...]
ValueError: not enough values to unpack (expected 2, got 1)
$ hg shelve -d junk
abort: shelved change 'junk' not found

I suspect code I'm removing in this patch was added before the higher-level check that we now have have (search for "not found" in the file). I don't think the intent was ever to support corrupt .hg/shelved/ directories.

I added tests for those cases early in the stack (inserted D9718 at the bottom). I also made it so hg shelve --list only lists valid shelves at the end of the stack.

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