Re: [PATCH 1/1] refactor: rename is_directory() to dir_exists() and use it in clone.c

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

"John Lin via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: John Lin <johnlinp@gmail.com>
>
> The original is_directory() checks whether the given path exists as
> a directory, which makes dir_exists() a more suitable name.

Why?  For a function that takes a path and asked "is this a
directory" that returns "yes/no", "is_directory(path)" is just as a
natural name as, if not more than, "dir_exists(path)".

IOW, "a more suitable name" needs a lot stronger justification than
"it subjectively sounds better to me".

> However, there is already an existing function called dir_exists(),
> while it doesn't check if the path is a directory.

Have you considered the possibility that it is deliberate (iow,
making it check may break the existing callers)?

bultin/clone.c wants to use its dir_exists() to see if *anything*
exists at the path already, so that it can issue an error message
and die when there is a non directory (e.g. a regular file), or a
non-empty directory, when it is told to create a repository.

It is a misnomer, sure, but that does not mean it is OK to break the
existing callers by suddenly saying "no there is not" when a user
tries to say "git clone $URL ~/.bashrc" (which used to fail because
the given destination is a file and it's "dir_exists()" said "oh,
there is something there already so you cannot mkdir there", but
with this patch it would say "nah, that thing is a file, so there is
no directory tehre").

> We decided to do the following:
> - remove the original dir_exists()
> - rename the original is_directory() to dir_exists()
> - use the new dir_exists() where the original dir_exists() is called

For a patch that touches all over the code like this, a summary of
what was done, like the above, is useful in evaluating how sensible
it is that the author wanted to do.  But it should also need to come
with "WHY".  Why does this patch do these three things?  Or, why
"we" (who are they?) decided to do so?

"We decided to do the following" does not answer that question, and
we can see from the patch text that the author decided to do them;
otherwise this patch would not exist ;-).

Thanks.