{"thread":{"id":"52200","subject":"[PATCH 0/1] refactor: rename is_directory() to dir_exists() and use it in clone.c","startedAt":"2019-11-06T07:28:40Z","lastAt":"2019-11-06T10:59:35Z","messageCount":3,"participants":["John Lin via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"385539","messageId":"pull.271.git.1573025315.gitgitgadget@gmail.com","threadId":"52200","inReplyTo":null,"subject":"[PATCH 0/1] refactor: rename is_directory() to dir_exists() and use it in clone.c","fromName":"John Lin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-06T07:28:34Z","receivedAt":"2019-11-06T07:28:40Z","isPatch":true,"sender":{"key":"johnlinp@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1290376?v=4"},"body":"Fixes #230\n\nJohn Lin (1):\n  refactor: rename is_directory() to dir_exists() and use it in clone.c\n\n abspath.c                   |  2 +-\n builtin/am.c                |  2 +-\n builtin/clone.c             |  6 ------\n builtin/mv.c                |  2 +-\n builtin/rebase.c            | 10 +++++-----\n builtin/submodule--helper.c |  4 ++--\n builtin/worktree.c          |  6 +++---\n cache.h                     |  2 +-\n daemon.c                    |  2 +-\n diff-no-index.c             |  4 ++--\n dir.c                       |  2 +-\n gettext.c                   |  2 +-\n rerere.c                    |  2 +-\n sha1-file.c                 |  6 +++---\n submodule.c                 |  4 ++--\n trace2/tr2_dst.c            |  2 +-\n worktree.c                  |  2 +-\n 17 files changed, 27 insertions(+), 33 deletions(-)\n\n\nbase-commit: da72936f544fec5a335e66432610e4cef4430991\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-271%2Fjohnlinp%2Freconcile-dir-exists-and-is-directory-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-271/johnlinp/reconcile-dir-exists-and-is-directory-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/271\n-- \ngitgitgadget\n"},{"id":"385540","messageId":"b36fdf6a92b3e5392ae45bf5e8a9132c54675d27.1573025315.git.gitgitgadget@gmail.com","threadId":"52200","inReplyTo":"pull.271.git.1573025315.gitgitgadget@gmail.com","subject":"[PATCH 1/1] refactor: rename is_directory() to dir_exists() and use it in clone.c","fromName":"John Lin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-06T07:28:35Z","receivedAt":"2019-11-06T07:28:43Z","isPatch":true,"sender":{"key":"johnlinp@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1290376?v=4"},"body":"From: John Lin <johnlinp@gmail.com>\n\nThe original is_directory() checks whether the given path exists as\na directory, which makes dir_exists() a more suitable name.\nHowever, there is already an existing function called dir_exists(),\nwhile it doesn't check if the path is a directory.\n\nWe decided to do the following:\n- remove the original dir_exists()\n- rename the original is_directory() to dir_exists()\n- use the new dir_exists() where the original dir_exists() is called\n\nHope it can reduce some confusion.\n\nSigned-off-by: John Lin <johnlinp@gmail.com>\n---\n abspath.c                   |  2 +-\n builtin/am.c                |  2 +-\n builtin/clone.c             |  6 ------\n builtin/mv.c                |  2 +-\n builtin/rebase.c            | 10 +++++-----\n builtin/submodule--helper.c |  4 ++--\n builtin/worktree.c          |  6 +++---\n cache.h                     |  2 +-\n daemon.c                    |  2 +-\n diff-no-index.c             |  4 ++--\n dir.c                       |  2 +-\n gettext.c                   |  2 +-\n rerere.c                    |  2 +-\n sha1-file.c                 |  6 +++---\n submodule.c                 |  4 ++--\n trace2/tr2_dst.c            |  2 +-\n worktree.c                  |  2 +-\n 17 files changed, 27 insertions(+), 33 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex 9857985329..13bd92eca5 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -5,7 +5,7 @@\n  * symlink to a directory, we do not want to say it is a directory when\n  * dealing with tracked content in the working tree.\n  */\n-int is_directory(const char *path)\n+int dir_exists(const char *path)\n {\n \tstruct stat st;\n \treturn (!stat(path, &st) && S_ISDIR(st.st_mode));\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 8181c2aef3..f872125fc7 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -576,7 +576,7 @@ static int detect_patch_format(const char **paths)\n \t/*\n \t * We default to mbox format if input is from stdin and for directories\n \t */\n-\tif (!*paths || !strcmp(*paths, \"-\") || is_directory(*paths))\n+\tif (!*paths || !strcmp(*paths, \"-\") || dir_exists(*paths))\n \t\treturn PATCH_FORMAT_MBOX;\n \n \t/*\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex c46ee29f0a..f89938bf94 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;\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex be15ba7044..194e1618a0 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -152,7 +152,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t * \"git mv directory no-such-dir/\".\n \t */\n \tflags = KEEP_TRAILING_SLASH;\n-\tif (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1]))\n+\tif (argc == 1 && dir_exists(argv[0]) && !dir_exists(argv[1]))\n \t\tflags = 0;\n \tdest_path = internal_prefix_pathspec(prefix, argv + argc, 1, flags);\n \tsubmodule_gitfile = xcalloc(argc, sizeof(char *));\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 4a20582e72..c66cdf729b 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -275,7 +275,7 @@ static int init_basic_state(struct replay_opts *opts, const char *head_name,\n {\n \tFILE *interactive;\n \n-\tif (!is_directory(merge_dir()) && mkdir_in_gitdir(merge_dir()))\n+\tif (!dir_exists(merge_dir()) && mkdir_in_gitdir(merge_dir()))\n \t\treturn error_errno(_(\"could not create temporary %s\"), merge_dir());\n \n \tdelete_reflog(\"REBASE_HEAD\");\n@@ -1068,7 +1068,7 @@ static int run_am(struct rebase_options *opts)\n \t\treturn move_to_original_branch(opts);\n \t}\n \n-\tif (is_directory(opts->state_dir))\n+\tif (dir_exists(opts->state_dir))\n \t\trebase_write_basic_state(opts);\n \n \treturn status;\n@@ -1529,13 +1529,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif(file_exists(buf.buf))\n \t\tdie(_(\"It looks like 'git am' is in progress. Cannot rebase.\"));\n \n-\tif (is_directory(apply_dir())) {\n+\tif (dir_exists(apply_dir())) {\n \t\toptions.type = REBASE_AM;\n \t\toptions.state_dir = apply_dir();\n-\t} else if (is_directory(merge_dir())) {\n+\t} else if (dir_exists(merge_dir())) {\n \t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"%s/rewritten\", merge_dir());\n-\t\tif (is_directory(buf.buf)) {\n+\t\tif (dir_exists(buf.buf)) {\n \t\t\toptions.type = REBASE_PRESERVE_MERGES;\n \t\t\toptions.flags |= REBASE_INTERACTIVE_EXPLICIT;\n \t\t} else {\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 2c2395a620..8df36e06b4 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1096,7 +1096,7 @@ static void deinit_submodule(const char *path, const char *prefix,\n \tdisplaypath = get_submodule_displaypath(path, prefix);\n \n \t/* remove the submodule work tree (unless the user already did it) */\n-\tif (is_directory(path)) {\n+\tif (dir_exists(path)) {\n \t\tstruct strbuf sb_rm = STRBUF_INIT;\n \t\tconst char *format;\n \n@@ -1105,7 +1105,7 @@ static void deinit_submodule(const char *path, const char *prefix,\n \t\t * NEEDSWORK: instead of dying, automatically call\n \t\t * absorbgitdirs and (possibly) warn.\n \t\t */\n-\t\tif (is_directory(sub_git_dir))\n+\t\tif (dir_exists(sub_git_dir))\n \t\t\tdie(_(\"Submodule work tree '%s' contains a .git \"\n \t\t\t      \"directory (use 'rm -rf' if you really want \"\n \t\t\t      \"to remove it including all of its history)\"),\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 4de44f579a..a69a1e5612 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -75,7 +75,7 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n \tsize_t len;\n \tssize_t read_result;\n \n-\tif (!is_directory(git_path(\"worktrees/%s\", id))) {\n+\tif (!dir_exists(git_path(\"worktrees/%s\", id))) {\n \t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: not a valid directory\"), id);\n \t\treturn 1;\n \t}\n@@ -738,7 +738,7 @@ static void validate_no_submodules(const struct worktree *wt)\n \tstruct strbuf path = STRBUF_INIT;\n \tint i, found_submodules = 0;\n \n-\tif (is_directory(worktree_git_path(wt, \"modules\"))) {\n+\tif (dir_exists(worktree_git_path(wt, \"modules\"))) {\n \t\t/*\n \t\t * There could be false positives, e.g. the \"modules\"\n \t\t * directory exists but is empty. But it's a rare case and\n@@ -799,7 +799,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n \tif (is_main_worktree(wt))\n \t\tdie(_(\"'%s' is a main working tree\"), av[0]);\n-\tif (is_directory(dst.buf)) {\n+\tif (dir_exists(dst.buf)) {\n \t\tconst char *sep = find_last_dir_sep(wt->path);\n \n \t\tif (!sep)\ndiff --git a/cache.h b/cache.h\nindex 04cabaac11..596de2db38 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1274,7 +1274,7 @@ static inline int is_absolute_path(const char *path)\n {\n \treturn is_dir_sep(path[0]) || has_dos_drive_prefix(path);\n }\n-int is_directory(const char *);\n+int dir_exists(const char *);\n char *strbuf_realpath(struct strbuf *resolved, const char *path,\n \t\t      int die_on_error);\n const char *real_path(const char *path);\ndiff --git a/daemon.c b/daemon.c\nindex 9d2e0d20ef..050c1ffacf 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1455,7 +1455,7 @@ int cmd_main(int argc, const char **argv)\n \tif (strict_paths && (!ok_paths || !*ok_paths))\n \t\tdie(\"option --strict-paths requires a whitelist\");\n \n-\tif (base_path && !is_directory(base_path))\n+\tif (base_path && !dir_exists(base_path))\n \t\tdie(\"base-path '%s' does not exist or is not a directory\",\n \t\t    base_path);\n \ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 7814eabfe0..7f6e17fc76 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -221,8 +221,8 @@ static void fixup_paths(const char **path, struct strbuf *replacement)\n \tif (path[0] == file_from_standard_input ||\n \t    path[1] == file_from_standard_input)\n \t\treturn;\n-\tisdir0 = is_directory(path[0]);\n-\tisdir1 = is_directory(path[1]);\n+\tisdir0 = dir_exists(path[0]);\n+\tisdir1 = dir_exists(path[1]);\n \tif (isdir0 == isdir1)\n \t\treturn;\n \tif (isdir0) {\ndiff --git a/dir.c b/dir.c\nindex 61f559f980..fd22bc1866 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2100,7 +2100,7 @@ static int treat_leading_path(struct dir_struct *dir,\n \t\t\tbaselen = cp - path;\n \t\tstrbuf_setlen(&sb, 0);\n \t\tstrbuf_add(&sb, path, baselen);\n-\t\tif (!is_directory(sb.buf))\n+\t\tif (!dir_exists(sb.buf))\n \t\t\tbreak;\n \t\tif (simplify_away(sb.buf, sb.len, pathspec))\n \t\t\tbreak;\ndiff --git a/gettext.c b/gettext.c\nindex 35d2c1218d..c02f6675fa 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -183,7 +183,7 @@ void git_setup_gettext(void)\n \n \tuse_gettext_poison(); /* getenv() reentrancy paranoia */\n \n-\tif (!is_directory(podir)) {\n+\tif (!dir_exists(podir)) {\n \t\tfree(p);\n \t\treturn;\n \t}\ndiff --git a/rerere.c b/rerere.c\nindex 3e51fdfe58..1d6a4b8df2 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -873,7 +873,7 @@ static int is_rerere_enabled(void)\n \tif (!rerere_enabled)\n \t\treturn 0;\n \n-\trr_cache_exists = is_directory(git_path_rr_cache());\n+\trr_cache_exists = dir_exists(git_path_rr_cache());\n \tif (rerere_enabled < 0)\n \t\treturn rr_cache_exists;\n \ndiff --git a/sha1-file.c b/sha1-file.c\nindex 188de57634..d0aca36abf 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -448,7 +448,7 @@ static int alt_odb_usable(struct raw_object_store *o,\n \tstruct object_directory *odb;\n \n \t/* Detect cases where alternate disappeared */\n-\tif (!is_directory(path->buf)) {\n+\tif (!dir_exists(path->buf)) {\n \t\terror(_(\"object directory %s does not exist; \"\n \t\t\t\"check .git/objects/info/alternates\"),\n \t\t      path->buf);\n@@ -699,11 +699,11 @@ char *compute_alternate_path(const char *path, struct strbuf *err)\n \t\tref_git = xstrdup(repo);\n \t}\n \n-\tif (!repo && is_directory(mkpath(\"%s/.git/objects\", ref_git))) {\n+\tif (!repo && dir_exists(mkpath(\"%s/.git/objects\", ref_git))) {\n \t\tchar *ref_git_git = mkpathdup(\"%s/.git\", ref_git);\n \t\tfree(ref_git);\n \t\tref_git = ref_git_git;\n-\t} else if (!is_directory(mkpath(\"%s/objects\", ref_git))) {\n+\t} else if (!dir_exists(mkpath(\"%s/objects\", ref_git))) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tseen_error = 1;\n \t\tif (get_common_dir(&sb, ref_git)) {\ndiff --git a/submodule.c b/submodule.c\nindex 0f199c5137..870f35cd56 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -174,7 +174,7 @@ int add_submodule_odb(const char *path)\n \tret = strbuf_git_path_submodule(&objects_directory, path, \"objects/\");\n \tif (ret)\n \t\tgoto done;\n-\tif (!is_directory(objects_directory.buf)) {\n+\tif (!dir_exists(objects_directory.buf)) {\n \t\tret = -1;\n \t\tgoto done;\n \t}\n@@ -1647,7 +1647,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tif (!git_dir)\n \t\tgit_dir = buf.buf;\n \tif (!is_git_directory(git_dir)) {\n-\t\tif (is_directory(git_dir))\n+\t\tif (dir_exists(git_dir))\n \t\t\tdie(_(\"'%s' not recognized as a git repository\"), git_dir);\n \t\tstrbuf_release(&buf);\n \t\t/* The submodule is not checked out, so it is not modified */\ndiff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c\nindex ae052a07fe..fe0b4d94ff 100644\n--- a/trace2/tr2_dst.c\n+++ b/trace2/tr2_dst.c\n@@ -337,7 +337,7 @@ int tr2_dst_get_trace_fd(struct tr2_dst *dst)\n \t}\n \n \tif (is_absolute_path(tgt_value)) {\n-\t\tif (is_directory(tgt_value))\n+\t\tif (dir_exists(tgt_value))\n \t\t\treturn tr2_dst_try_auto_path(dst, tgt_value);\n \t\telse\n \t\t\treturn tr2_dst_try_path(dst, tgt_value);\ndiff --git a/worktree.c b/worktree.c\nindex 5b4793caa3..e2d2e2dbf1 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -290,7 +290,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,\n \tstrbuf_addf(&wt_path, \"%s/.git\", wt->path);\n \n \tif (is_main_worktree(wt)) {\n-\t\tif (is_directory(wt_path.buf)) {\n+\t\tif (dir_exists(wt_path.buf)) {\n \t\t\tret = 0;\n \t\t\tgoto done;\n \t\t}\n-- \ngitgitgadget\n"},{"id":"385580","messageId":"xmqqbltptgmo.fsf@gitster-ct.c.googlers.com","threadId":"52200","inReplyTo":"b36fdf6a92b3e5392ae45bf5e8a9132c54675d27.1573025315.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] refactor: rename is_directory() to dir_exists() and use it in clone.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-06T10:59:27Z","receivedAt":"2019-11-06T10:59:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Lin via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: John Lin <johnlinp@gmail.com>\n>\n> The original is_directory() checks whether the given path exists as\n> a directory, which makes dir_exists() a more suitable name.\n\nWhy?  For a function that takes a path and asked \"is this a\ndirectory\" that returns \"yes/no\", \"is_directory(path)\" is just as a\nnatural name as, if not more than, \"dir_exists(path)\".\n\nIOW, \"a more suitable name\" needs a lot stronger justification than\n\"it subjectively sounds better to me\".\n\n> However, there is already an existing function called dir_exists(),\n> while it doesn't check if the path is a directory.\n\nHave you considered the possibility that it is deliberate (iow,\nmaking it check may break the existing callers)?\n\nbultin/clone.c wants to use its dir_exists() to see if *anything*\nexists at the path already, so that it can issue an error message\nand die when there is a non directory (e.g. a regular file), or a\nnon-empty directory, when it is told to create a repository.\n\nIt is a misnomer, sure, but that does not mean it is OK to break the\nexisting callers by suddenly saying \"no there is not\" when a user\ntries to say \"git clone $URL ~/.bashrc\" (which used to fail because\nthe given destination is a file and it's \"dir_exists()\" said \"oh,\nthere is something there already so you cannot mkdir there\", but\nwith this patch it would say \"nah, that thing is a file, so there is\nno directory tehre\").\n\n> We decided to do the following:\n> - remove the original dir_exists()\n> - rename the original is_directory() to dir_exists()\n> - use the new dir_exists() where the original dir_exists() is called\n\nFor a patch that touches all over the code like this, a summary of\nwhat was done, like the above, is useful in evaluating how sensible\nit is that the author wanted to do.  But it should also need to come\nwith \"WHY\".  Why does this patch do these three things?  Or, why\n\"we\" (who are they?) decided to do so?\n\n\"We decided to do the following\" does not answer that question, and\nwe can see from the patch text that the author decided to do them;\notherwise this patch would not exist ;-).\n\nThanks.\n"}]}