{"thread":{"id":"60725","subject":"[PATCH] git-p4: stop reaching into the refdb","startedAt":"2024-01-11T13:47:32Z","lastAt":"2024-01-11T21:20:14Z","messageCount":2,"participants":["Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"486629","messageId":"33d6a062ec56be33ed50a42a420be0b023f6f4cf.1704980814.git.ps@pks.im","threadId":"60725","inReplyTo":null,"subject":"[PATCH] git-p4: stop reaching into the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-11T13:47:27Z","receivedAt":"2024-01-11T13:47:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The git-p4 tool creates a bunch of temporary branches that share a\ncommon prefix \"refs/git-p4-tmp/\". These branches get cleaned up via\ngit-update-ref(1) after the import has finished. Once done, we try to\nmanually remove the now supposedly-empty \".git/refs/git-p4-tmp/\"\ndirectory.\n\nThis last step can fail in case there still are any temporary branches\naround that we failed to delete because `os.rmdir()` refuses to delete a\nnon-empty directory. It can thus be seen as kind of a sanity check to\nverify that we really did delete all temporary branches. Another failure\nmode though is when the directory didn't exist in the first place, which\ncan be the case when using an alternate ref backend like the upcoming\n\"reftable\" backend.\n\nConvert the code to instead use git-for-each-ref(1) to verify that there\nare no more temporary branches around. This works alright with alternate\nref backends while retaining the sanity check that we really did prune\nall temporary branches.\n\nThis is a modification in behaviour for the \"files\" backend because the\nempty directory does not get deleted anymore. But arguably we should not\ncare about such implementation details of the ref backend anyway, and\nthis should not cause any user-visible change in behaviour.\n\nSigned-off-by: Patrick Steinhardt \n---\n git-p4.py | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 0eb3bb4c47..3ea1c405e5 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -4251,7 +4251,8 @@ def run(self, args):\n if self.tempBranches != []:\n for branch in self.tempBranches:\n read_pipe([\"git\", \"update-ref\", \"-d\", branch])\n- os.rmdir(os.path.join(os.environ.get(\"GIT_DIR\", \".git\"), self.tempBranchLocation))\n+ if len(read_pipe([\"git\", \"for-each-ref\", self.tempBranchLocation])) > 0:\n+ die(\"There are unexpected temporary branches\")\n \n # Create a symbolic ref p4/HEAD pointing to p4/ to allow\n # a convenient shortcut refname \"p4\".\n-- \n2.43.GIT\n\n"},{"id":"486658","messageId":"xmqqa5pbftys.fsf@gitster.g","threadId":"60725","inReplyTo":"33d6a062ec56be33ed50a42a420be0b023f6f4cf.1704980814.git.ps@pks.im","subject":"Re: [PATCH] git-p4: stop reaching into the refdb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-11T21:20:11Z","receivedAt":"2024-01-11T21:20:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> The git-p4 tool creates a bunch of temporary branches that share a\n> common prefix \"refs/git-p4-tmp/\". These branches get cleaned up via\n> git-update-ref(1) after the import has finished. Once done, we try to\n> manually remove the now supposedly-empty \".git/refs/git-p4-tmp/\"\n> directory.\n>\n> This last step can fail in case there still are any temporary branches\n> around that we failed to delete because `os.rmdir()` refuses to delete a\n> non-empty directory. It can thus be seen as kind of a sanity check to\n> verify that we really did delete all temporary branches.\n\nWow, thanks for being very careful. I would just have said \"there\nis no need for such rmdir---what's a single empty unused directory\nbetween friends, which will be reused later when you run 'git p4'\nagain?\" without thinking things through.\n\n> This is a modification in behaviour for the \"files\" backend because the\n> empty directory does not get deleted anymore. But arguably we should not\n> care about such implementation details of the ref backend anyway, and\n> this should not cause any user-visible change in behaviour.\n\nIndependently, it would be sensible to improve the files backend so\nthat it removes a subdirectory outside the hierarchies that are\ncreated by refs/files-backend.c:files_init_db() when it becomes\nempty (#leftoverbits).\n\nAnd such a change would also have triggered the error from\nos.rmdir(), so this patch is doubly welcomed.\n\nThanks.\n\n\n\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> git-p4.py | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 0eb3bb4c47..3ea1c405e5 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -4251,7 +4251,8 @@ def run(self, args):\n> if self.tempBranches != []:\n> for branch in self.tempBranches:\n> read_pipe([\"git\", \"update-ref\", \"-d\", branch])\n> - os.rmdir(os.path.join(os.environ.get(\"GIT_DIR\", \".git\"), self.tempBranchLocation))\n> + if len(read_pipe([\"git\", \"for-each-ref\", self.tempBranchLocation])) > 0:\n> + die(\"There are unexpected temporary branches\")\n> \n> # Create a symbolic ref p4/HEAD pointing to p4/ to allow\n> # a convenient shortcut refname \"p4\".\n"}]}