"John Lin via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > From: John Lin <johnlinp@xxxxxxxxx> > > 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.
