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?

In D9698#146561, @joerg.sonnenberger wrote:

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).

In D9698#146562, @joerg.sonnenberger wrote:

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.

In D9698#146566, @joerg.sonnenberger wrote:

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.

In D9698#146597, @martinvonz wrote:

In D9698#146566, @joerg.sonnenberger wrote:

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.