refactor: rename is_directory() to dir_exists() and use it in clone.c #271
Conversation
| @@ -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) | |||
There was a problem hiding this comment.
Looks good to me! Maybe the commit message could talk about the value of reconciling functionality, about consistency in naming (definitely mention that there is a file_exists() already), and it should probably talk about the differences of the is_directory() and the dir_exists() function, and that the former is a superset of the latter, but the latter has the better name.
There was a problem hiding this comment.
Sure. I'll work on it. Thank you.
There was a problem hiding this comment.
I already updated the commit message. Since I am not a native speaker, please feel free to correct me if you find the sentences weird.
There was a problem hiding this comment.
(Please note that I am also not a native speaker...)
johnlinp
force-pushed
the
reconcile-dir-exists-and-is-directory
branch
from
dd3c38d to
a896c32
Compare
| @@ -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) | |||
|
@johnlinp did you want to |
The original is_directory() checks whether the given path exists as a directory, which makes dir_exists() a more suitable name. However, there is already an existing function called dir_exists(), while it doesn't check if the path is a directory. 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 Hope it can reduce some confusion. Signed-off-by: John Lin <johnlinp@gmail.com>
johnlinp
force-pushed
the
reconcile-dir-exists-and-is-directory
branch
from
a896c32 to
b36fdf6
Compare
|
/submit |
|
Submitted as pull.271.git.1573025315.gitgitgadget@gmail.com |
|
@johnlinp did you see all those build failures? E.g. https://dev.azure.com/gitgitgadget/git/_build/results?buildId=20204&view=logs&jobId=fd490c07-0b22-5182-fac9-6d67fe1e939b&taskId=ce91d5d6-0c55-50f5-8ab9-6695c03ab102&lineStart=223&lineEnd=228&colStart=1&colEnd=29 It would appear that at least one call site in |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -5,7 +5,7 @@ | |||
| * symlink to a directory, we do not want to say it is a directory when | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"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.
Fixes #230