Re: [PATCH 2/2] git clone: don't clone into non-empty directory

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 1, 2020, 16:10 UTC
Message-ID: <CAPig+cTEpajuD98RH5xepJ+rK2f75SROrTvgjzKnyfjxbuA1AQ@mail.gmail.com>
In-Reply-To: <20200701093653.3706-3-ben@wijen.net>

On Wed, Jul 1, 2020 at 5:46 AM Ben Wijen <ben@wijen.net> wrote:

Show 15 quoted lines

> When using git clone with --separate-git-dir realgitdir and
> realgitdir already exists, it's content is destroyed.
>
> Make sure we don't clone into an existing non-empty directory.
>
> Signed-off-by: Ben Wijen <ben@wijen.net>
> ---
> diff --git a/builtin/clone.c b/builtin/clone.c
> @@ -1021,6 +1021,14 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>                 die(_("destination path '%s' already exists and is not "
>                         "an empty directory."), dir);
>
> +       if (real_git_dir) {
> +               real_dest_exists = path_exists(real_git_dir);
> +               if (real_dest_exists && !is_empty_dir(real_git_dir))

"git init --separate-git-dir=" checks only whether the path exists, and errors out if it does; it doesn't care whether the directory is empty or not. I'm wondering, therefore, if this check should be tightened to more closely align the behavior of the two commands. (Using the tighter semantic now doesn't prohibit loosening it in the future, whereas it's harder to tighten behavior which started out loose.)

> +                       die(_("destination path '%s' already exists and is not "
> +                               "an empty directory."), real_git_dir);

This message might be a bit clearer if "destination" is changed to "repository". (And drop the bit about not being empty -- if we go that route.)