On Thu, Oct 24, 2019 at 11:27:45AM +0200, Miriam Rubio wrote:
> The dir_exists() function in builtin/clone.c is marked as static, so
> nobody can use it outside builtin/clone.c.
>
> There is also is_directory() which obviously tries to do the very same, but it uses a name that few developers will think of when they see file_exists() and look for the equivalent function to see whether a given directory exists.
>
> Let's reconcile these functions by renaming is_directory() to dir_exists() and using it also in builtin/clone.c.
Please wrap the proposed log message at about 70 characters width;
that way it will look much better in 'git log' in a standard 80 char
wide terminal.
I think this is a cleanup worth doing, but...
> diff --git a/abspath.c b/abspath.c
> index 9857985329..13bd92eca5 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -5,7 +5,7 @@
> * symlink to a directory, we do not want to say it is a directory when
> * dealing with tracked content in the working tree.
> */
> -int is_directory(const char *path)
> +int dir_exists(const char *path)
> {
> struct stat st;
> return (!stat(path, &st) && S_ISDIR(st.st_mode));
Note the '&& S_ISDIR(st.st_mode)', making sure that the given path is
in fact a directory. Good.
> diff --git a/builtin/clone.c b/builtin/clone.c
> index c46ee29f0a..f89938bf94 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -899,12 +899,6 @@ static void dissociate_from_references(void)
> free(alternates);
> }
>
> -static int dir_exists(const char *path)
> -{
> - struct stat sb;
> - return !stat(path, &sb);
But look at this, it only checks that the given path exists, but it
could be a regular file or any other kind of path other than a
directory as well!
So this function clearly doesn't do what it's name suggests. That's
bad.
Unfortunately, it gets worse: some of its callsites in
'builtin/clone.c' do expect it to check the existence of _any_ path,
not just a directory.
The first callsite is:
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);
I think this actually means path_exists(): if a file, or any other
kind of path with the given name were to exist, then we should die()
showing this error message, but after changing dir_exists() to make
sure that the path is indeed a directory we won't:
# create a 'git' _file_
$ >git
# current git master:
$ git clone https://github.com/git/git
fatal: destination path 'git' already exists and is not an empty directory.
# with this patch:
$ ~/src/git/git clone https://github.com/git/git
fatal: could not create work tree dir 'git': File exists
So the command still fails, which is good, but with a different error
message. The test suite doesn't catch this, because the test case
looking at this scenario ('clone to an existing path' in
'./t5601-clone.sh') only checks that 'git clone' fails, but it doesn't
check whether it failed with the right error message.
Now, that other error message comes after a failed mkdir() call later
on which should have created the work tree. So it begs the question
what would happen when a file is in the way of a bare clone:
$ >git.git
$ git clone --bare https://github.com/git/git
fatal: destination path 'git.git' already exists and is not an empty directory.
$ ~/src/git/git clone --bare https://github.com/git/git
Cloning into bare repository 'git.git'...
fatal: invalid gitfile format: /home/szeder/src/git/tmp/git.git
Then the next callsite looks like it meant path_exists() as well, but
I didn't try to make it fail or show different behavior:
work_tree = getenv("GIT_WORK_TREE");
if (work_tree && dir_exists(work_tree))
die(_("working tree '%s' already exists."), work_tree);
And there is a third callsite, but I'm not sure what it is about, and,
consequently, what is really meant with dir_exists() here:
if (real_git_dir) {
if (dir_exists(real_git_dir))
junk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;
