| Message ID | 20191027163038.47409-1-mirucam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [Outreachy] dir: add new function `path_exists()` | expand |
Miriam Rubio <mirucam@gmail.com> 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.
Reviewed-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
diff --git a/archive.c b/archive.c index a8da0fcc4f..8110a50f17 100644 --- a/archive.c +++ b/archive.c @@ -338,7 +338,7 @@ static int reject_entry(const struct object_id *oid, struct strbuf *base, return ret; } -static int path_exists(struct archiver_args *args, const char *path) +static int archive_path_exists(struct archiver_args *args, const char *path) { const char *paths[] = { path, NULL }; struct path_exists_context ctx; @@ -358,7 +358,7 @@ static void parse_pathspec_arg(const char **pathspec, struct archiver_args *ar_args) { /* - * must be consistent with parse_pathspec in path_exists() + * must be consistent with parse_pathspec in archive_path_exists() * Also if pathspec patterns are dependent, we're in big * trouble as we test each one separately */ @@ -368,7 +368,7 @@ static void parse_pathspec_arg(const char **pathspec, ar_args->pathspec.recursive = 1; if (pathspec) { while (*pathspec) { - if (**pathspec && !path_exists(ar_args, *pathspec)) + if (**pathspec && !archive_path_exists(ar_args, *pathspec)) die(_("pathspec '%s' did not match any files"), *pathspec); pathspec++; } diff --git a/builtin/clone.c b/builtin/clone.c index c46ee29f0a..20ab535784 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); -} - int cmd_clone(int argc, const char **argv, const char *prefix) { int is_bundle = 0, is_local; @@ -981,7 +975,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) dir = guess_dir_name(repo_name, is_bundle, option_bare); strip_trailing_slashes(dir); - dest_exists = dir_exists(dir); + dest_exists = path_exists(dir); if (dest_exists && !is_empty_dir(dir)) die(_("destination path '%s' already exists and is not " "an empty directory."), dir); @@ -992,7 +986,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) work_tree = NULL; else { work_tree = getenv("GIT_WORK_TREE"); - if (work_tree && dir_exists(work_tree)) + if (work_tree && path_exists(work_tree)) die(_("working tree '%s' already exists."), work_tree); } @@ -1020,7 +1014,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) } if (real_git_dir) { - if (dir_exists(real_git_dir)) + if (path_exists(real_git_dir)) junk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL; junk_git_dir = real_git_dir; } else { diff --git a/dir.c b/dir.c index 61f559f980..638a783b65 100644 --- a/dir.c +++ b/dir.c @@ -2353,6 +2353,12 @@ int read_directory(struct dir_struct *dir, struct index_state *istate, return dir->nr; } +int path_exists(const char *path) +{ + struct stat sb; + return !stat(path, &sb); +} + int file_exists(const char *f) { struct stat sb; diff --git a/dir.h b/dir.h index 2fbdef014f..376fa93321 100644 --- a/dir.h +++ b/dir.h @@ -286,6 +286,9 @@ void clear_pattern_list(struct pattern_list *pl); void clear_directory(struct dir_struct *dir); int repo_file_exists(struct repository *repo, const char *path); +int path_exists(const char *); + +/* New calls should use path_exists(). */ int file_exists(const char *); int is_inside_dir(const char *dir);
Added a new function path_exists() that works the same as file_exists() but with better descriptive name. New calls should use path_exists() instead of file_exists(). The dir_exists() function in builtin/clone.c is marked as static, so nobody can use it outside builtin/clone.c and can be replaced by new function path_exists(). The static function path_exists() in archive.c have been renamed as archive_path_exists() to avoid name collision. Signed-off-by: Miriam Rubio <mirucam@gmail.com> --- archive.c | 6 +++--- builtin/clone.c | 12 +++--------- dir.c | 6 ++++++ dir.h | 3 +++ 4 files changed, 15 insertions(+), 12 deletions(-)