This is an archive of the discontinued Mercurial Phabricator instance.

shelve: rewrite check for unknown shelf to delete
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.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.

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"

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

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

marmoute added a comment.EditedJan 16 2021, 7:24 PM

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)

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.