{"thread":{"id":"52132","subject":"[Outreachy] [PATCH] dir: add new function `path_exists()`","startedAt":"2019-10-27T16:30:46Z","lastAt":"2019-10-28T09:46:01Z","messageCount":3,"participants":["Miriam Rubio","Junio C Hamano","Carlo Arenas"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"384931","messageId":"20191027163038.47409-1-mirucam@gmail.com","threadId":"52132","inReplyTo":null,"subject":"[Outreachy] [PATCH] dir: add new function `path_exists()`","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2019-10-27T16:30:38Z","receivedAt":"2019-10-27T16:30:46Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Added a new function path_exists() that works the same as file_exists()\nbut with better descriptive name.\nNew calls should use path_exists() instead of file_exists().\n\nThe dir_exists() function in builtin/clone.c is marked as static, so\nnobody can use it outside builtin/clone.c and can be replaced by new\nfunction path_exists().\n\nThe static function path_exists() in archive.c have been renamed as\narchive_path_exists() to avoid name collision.\n\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\n---\n archive.c       |  6 +++---\n builtin/clone.c | 12 +++---------\n dir.c           |  6 ++++++\n dir.h           |  3 +++\n 4 files changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex a8da0fcc4f..8110a50f17 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -338,7 +338,7 @@ static int reject_entry(const struct object_id *oid, struct strbuf *base,\n \treturn ret;\n }\n \n-static int path_exists(struct archiver_args *args, const char *path)\n+static int archive_path_exists(struct archiver_args *args, const char *path)\n {\n \tconst char *paths[] = { path, NULL };\n \tstruct path_exists_context ctx;\n@@ -358,7 +358,7 @@ static void parse_pathspec_arg(const char **pathspec,\n \t\tstruct archiver_args *ar_args)\n {\n \t/*\n-\t * must be consistent with parse_pathspec in path_exists()\n+\t * must be consistent with parse_pathspec in archive_path_exists()\n \t * Also if pathspec patterns are dependent, we're in big\n \t * trouble as we test each one separately\n \t */\n@@ -368,7 +368,7 @@ static void parse_pathspec_arg(const char **pathspec,\n \tar_args->pathspec.recursive = 1;\n \tif (pathspec) {\n \t\twhile (*pathspec) {\n-\t\t\tif (**pathspec && !path_exists(ar_args, *pathspec))\n+\t\t\tif (**pathspec && !archive_path_exists(ar_args, *pathspec))\n \t\t\t\tdie(_(\"pathspec '%s' did not match any files\"), *pathspec);\n \t\t\tpathspec++;\n \t\t}\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex c46ee29f0a..20ab535784 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -899,12 +899,6 @@ static void dissociate_from_references(void)\n \tfree(alternates);\n }\n \n-static int dir_exists(const char *path)\n-{\n-\tstruct stat sb;\n-\treturn !stat(path, &sb);\n-}\n-\n int cmd_clone(int argc, const char **argv, const char *prefix)\n {\n \tint is_bundle = 0, is_local;\n@@ -981,7 +975,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tdir = guess_dir_name(repo_name, is_bundle, option_bare);\n \tstrip_trailing_slashes(dir);\n \n-\tdest_exists = dir_exists(dir);\n+\tdest_exists = path_exists(dir);\n \tif (dest_exists && !is_empty_dir(dir))\n \t\tdie(_(\"destination path '%s' already exists and is not \"\n \t\t\t\"an empty directory.\"), dir);\n@@ -992,7 +986,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\twork_tree = NULL;\n \telse {\n \t\twork_tree = getenv(\"GIT_WORK_TREE\");\n-\t\tif (work_tree && dir_exists(work_tree))\n+\t\tif (work_tree && path_exists(work_tree))\n \t\t\tdie(_(\"working tree '%s' already exists.\"), work_tree);\n \t}\n \n@@ -1020,7 +1014,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (real_git_dir) {\n-\t\tif (dir_exists(real_git_dir))\n+\t\tif (path_exists(real_git_dir))\n \t\t\tjunk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;\n \t\tjunk_git_dir = real_git_dir;\n \t} else {\ndiff --git a/dir.c b/dir.c\nindex 61f559f980..638a783b65 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2353,6 +2353,12 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n \treturn dir->nr;\n }\n \n+int path_exists(const char *path)\n+{\n+    struct stat sb;\n+    return !stat(path, &sb);\n+}\n+\n int file_exists(const char *f)\n {\n \tstruct stat sb;\ndiff --git a/dir.h b/dir.h\nindex 2fbdef014f..376fa93321 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -286,6 +286,9 @@ void clear_pattern_list(struct pattern_list *pl);\n void clear_directory(struct dir_struct *dir);\n \n int repo_file_exists(struct repository *repo, const char *path);\n+int path_exists(const char *);\n+\n+/* New calls should use path_exists(). */\n int file_exists(const char *);\n \n int is_inside_dir(const char *dir);\n-- \n2.21.0 (Apple Git-122)\n\n"},{"id":"384960","messageId":"xmqqy2x5a9sf.fsf@gitster-ct.c.googlers.com","threadId":"52132","inReplyTo":"20191027163038.47409-1-mirucam@gmail.com","subject":"Re: [Outreachy] [PATCH] dir: add new function `path_exists()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-28T02:22:40Z","receivedAt":"2019-10-28T02:22:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miriam Rubio <mirucam@gmail.com> writes:\n\n> Added a new function path_exists() that works the same as file_exists()\n> but with better descriptive name.\n\n\"I did this\" before justifying why it is a good thing is not a good\ndescription.\n\n\tbuiltin/clone.c has a static funciton dir_exists() that\n\tchecks if a given path exists on the filesystem.  It returns\n\ttrue (and it is correct for it to return true) when the\n\tgiven path exists as a non-directory (e.g. a regular file).\n\n\tThis is confusing.  What the caller wants to check, and what\n\tthis function wants to return, is if the path exists, so\n\trename it to path_exists().\n\nwould make sense (and follows our convention to command the codebase\nto \"become like so\").\n\n> New calls should use path_exists() instead of file_exists().\n\nThis is not a good suggestion in general, and I do not want to see\nsuch a statement here or (more importantly) not in the header file.\nCalls that want to see if the path exists, regardless of type,\nshould use path_exists() instead of file_exists().  Other calls that\nshould be checking if a regular file exists there should continue to\nuse file_exists().  Once we finished sweeping code, one of two\nthings could happen:\n\n (1) there remains no caller to file_exists()---it turns out that\n     everybody wanted to check if there is something at the given\n     path, no matter what type of filesystem entity it is.  In such\n     a case, we can safely remove file_exists().\n\n (2) there are legitimate callers to file_exists()---these callers\n     used \"does stat() succeed?\" without checking if the filesystem\n     entity at the path is indeed a regular file, but that was what\n     they wanted to do.  In such a case, we can tighten the check in\n     file_exists() to also make sure that we saw a regular file.\n\nIf you audited all existing callers of file_exists() as a part of\npreparing this topic, and if the result is (1) above, then I think\nthis single patch as the whole of this topic is OK.  But if so, the\nproposed log message should state that fact to justify the above\nstatement.  Also we may want to remove the implementation of\nfile_exists() and replace it with\n\n\t#define file_exists(path) path_exists(path)\n\nin dir.h if we were to go that route.\n\nI actually suspect that you would rather one to go in the other\ndirection, i.e. to narrow the scope of this topic and change the\ndir_exists() to path_exists() inside builtin/clone.c, leaving the\nfunction file-scope static, and stop there.  Unless/until all the\nexisting callers of file_exists() have been audited and we know all\nof (or at least \"most of\") them want to ask \"does anything exist at\nthis path?\", that would be the more sensible thing to do.\n\nThanks.\n"},{"id":"384970","messageId":"CAPUEspjGUXC-rbF2gpeOfYcajm4mtGRiVNc+Bc3++JgapDLzxg@mail.gmail.com","threadId":"52132","inReplyTo":"20191027163038.47409-1-mirucam@gmail.com","subject":"Re: [Outreachy] [PATCH] dir: add new function `path_exists()`","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-10-28T09:45:48Z","receivedAt":"2019-10-28T09:46:01Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Reviewed-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n"}]}