Miriam Rubio <mirucam@xxxxxxxxx> writes:
> Added a new function path_exists() that works the same as file_exists()
> but with better descriptive name.
"I did this" before justifying why it is a good thing is not a good
description.
builtin/clone.c has a static funciton dir_exists() that
checks if a given path exists on the filesystem. It returns
true (and it is correct for it to return true) when the
given path exists as a non-directory (e.g. a regular file).
This is confusing. What the caller wants to check, and what
this function wants to return, is if the path exists, so
rename it to path_exists().
would make sense (and follows our convention to command the codebase
to "become like so").
> New calls should use path_exists() instead of file_exists().
This is not a good suggestion in general, and I do not want to see
such a statement here or (more importantly) not in the header file.
Calls that want to see if the path exists, regardless of type,
should use path_exists() instead of file_exists(). Other calls that
should be checking if a regular file exists there should continue to
use file_exists(). Once we finished sweeping code, one of two
things could happen:
(1) there remains no caller to file_exists()---it turns out that
everybody wanted to check if there is something at the given
path, no matter what type of filesystem entity it is. In such
a case, we can safely remove file_exists().
(2) there are legitimate callers to file_exists()---these callers
used "does stat() succeed?" without checking if the filesystem
entity at the path is indeed a regular file, but that was what
they wanted to do. In such a case, we can tighten the check in
file_exists() to also make sure that we saw a regular file.
If you audited all existing callers of file_exists() as a part of
preparing this topic, and if the result is (1) above, then I think
this single patch as the whole of this topic is OK. But if so, the
proposed log message should state that fact to justify the above
statement. Also we may want to remove the implementation of
file_exists() and replace it with
#define file_exists(path) path_exists(path)
in dir.h if we were to go that route.
I actually suspect that you would rather one to go in the other
direction, i.e. to narrow the scope of this topic and change the
dir_exists() to path_exists() inside builtin/clone.c, leaving the
function file-scope static, and stop there. Unless/until all the
existing callers of file_exists() have been audited and we know all
of (or at least "most of") them want to ask "does anything exist at
this path?", that would be the more sensible thing to do.
Thanks.
