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

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

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




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