Re: [PATCH 3/4] clone: factor out dir_exists() helper
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 4, 2018, 23:47 UTC
- Message-ID
- <xmqqbmi9dw55.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20180102211014.GC22556@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 18 quoted lines
> Two parts of git-clone's setup logic check whether a > directory exists, and they both call stat directly with the > same scratch "struct stat" buffer. Let's pull that into a > helper, which has a few advantages: > > - it makes the purpose of the stat calls more obvious > > - it makes it clear that we don't care about the > information in "buf" remaining valid > > - if we later decide to make the check more robust (e.g., > complaining about non-directories), we can do it in one > place > > Note that we could just use file_exists() for this, which > has identical code. But we specifically care about > directories, so this future-proofs us against that function > later getting more picky about seeing actual files.
It leaves funny taste in my mouth to see that dir_exists() does call stat() but does not check st.st_mode to see if it is a directory, but for this particular caller, we want dest_exists() to be true even when the thing is a non-directory, so that !is_empty_dir(dir) call is made on the next line to trigger "exists but not an empty dir" error. After all, what this caller really wants to ask is "is something sitting there?" and the answer it expects under normal condition is "no, there is nothing there".
If we really want to be anal, perhaps a new helper path_exists() that cares only about existence of paths (i.e. the implementation of these two helpers they currently have), together with update to check the st.st_mode for file_exists() and dir_exists(), may help making the API set more rational, but I do not think it is worth it.
Thanks.
Show 44 quoted lines
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/clone.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 2da71db107..04b0d7283f 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -863,10 +863,15 @@ static void dissociate_from_references(void)
> free(alternates);
> }
>
> +static int dir_exists(const char *path)
> +{
> + struct stat sb;
> + return !stat(path, &sb);
> +}
> +
> int cmd_clone(int argc, const char **argv, const char *prefix)
> {
> int is_bundle = 0, is_local;
> - struct stat buf;
> const char *repo_name, *repo, *work_tree, *git_dir;
> char *path, *dir;
> int dest_exists;
> @@ -938,7 +943,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> dir = guess_dir_name(repo_name, is_bundle, option_bare);
> strip_trailing_slashes(dir);
>
> - dest_exists = !stat(dir, &buf);
> + dest_exists = dir_exists(dir);
> if (dest_exists && !is_empty_dir(dir))
> die(_("destination path '%s' already exists and is not "
> "an empty directory."), dir);
> @@ -949,7 +954,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> work_tree = NULL;
> else {
> work_tree = getenv("GIT_WORK_TREE");
> - if (work_tree && !stat(work_tree, &buf))
> + if (work_tree && dir_exists(work_tree))
> die(_("working tree '%s' already exists."), work_tree);
> }