Re: [PATCH] git-p4: stop reaching into the refdb

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 11, 2024, 21:20 UTC
Message-ID: <xmqqa5pbftys.fsf@gitster.g>
In-Reply-To: <33d6a062ec56be33ed50a42a420be0b023f6f4cf.1704980814.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 10 quoted lines

> The git-p4 tool creates a bunch of temporary branches that share a
> common prefix "refs/git-p4-tmp/". These branches get cleaned up via
> git-update-ref(1) after the import has finished. Once done, we try to
> manually remove the now supposedly-empty ".git/refs/git-p4-tmp/"
> directory.
>
> This last step can fail in case there still are any temporary branches
> around that we failed to delete because `os.rmdir()` refuses to delete a
> non-empty directory. It can thus be seen as kind of a sanity check to
> verify that we really did delete all temporary branches.

Wow, thanks for being very careful. I would just have said "there is no need for such rmdir---what's a single empty unused directory between friends, which will be reused later when you run 'git p4' again?" without thinking things through.

> This is a modification in behaviour for the "files" backend because the
> empty directory does not get deleted anymore. But arguably we should not
> care about such implementation details of the ref backend anyway, and
> this should not cause any user-visible change in behaviour.

Independently, it would be sensible to improve the files backend so that it removes a subdirectory outside the hierarchies that are created by refs/files-backend.c:files_init_db() when it becomes empty (#leftoverbits).

And such a change would also have triggered the error from os.rmdir(), so this patch is doubly welcomed.

Thanks.

Show 19 quoted lines

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  git-p4.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 0eb3bb4c47..3ea1c405e5 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -4251,7 +4251,8 @@ def run(self, args):
>          if self.tempBranches != []:
>              for branch in self.tempBranches:
>                  read_pipe(["git", "update-ref", "-d", branch])
> -            os.rmdir(os.path.join(os.environ.get("GIT_DIR", ".git"), self.tempBranchLocation))
> +            if len(read_pipe(["git", "for-each-ref", self.tempBranchLocation])) > 0:
> +                   die("There are unexpected temporary branches")
>  
>          # Create a symbolic ref p4/HEAD pointing to p4/<branch> to allow
>          # a convenient shortcut refname "p4".