refactor: rename is_directory() to dir_exists() and use it in clone.c
cover refactor: rename is_directory() to dir_exists() and use it in clone.c
Fixes #230
John Lin (1):
refactor: rename is_directory() to dir_exists() and use it in clone.c
abspath.c | 2 +- builtin/am.c | 2 +- builtin/clone.c | 6 ------ builtin/mv.c | 2 +- builtin/rebase.c | 10 +++++----- builtin/submodule--helper.c | 4 ++-- builtin/worktree.c | 6 +++--- cache.h | 2 +- daemon.c | 2 +- diff-no-index.c | 4 ++-- dir.c | 2 +- gettext.c | 2 +- rerere.c | 2 +- sha1-file.c | 6 +++--- submodule.c | 4 ++-- trace2/tr2_dst.c | 2 +- worktree.c | 2 +- 17 files changed, 27 insertions(+), 33 deletions(-)
base-commit: da72936f544fec5a335e66432610e4cef4430991
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-271%2Fjohnlinp%2Freconcile-dir-exists-and-is-directory-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-271/johnlinp/reconcile-dir-exists-and-is-directory-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/271
Signature · 1 line
gitgitgadget
PATCH 1/1 refactor: rename is_directory() to dir_exists() and use it in clone.c
From: John Lin <[email protected]>
The original is_directory() checks whether the given path exists as a directory, which makes dir_exists() a more suitable name. However, there is already an existing function called dir_exists(), while it doesn't check if the path is a directory.
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
Hope it can reduce some confusion.
--- abspath.c | 2 +- builtin/am.c | 2 +- builtin/clone.c | 6 ------ builtin/mv.c | 2 +- builtin/rebase.c | 10 +++++----- builtin/submodule--helper.c | 4 ++-- builtin/worktree.c | 6 +++--- cache.h | 2 +- daemon.c | 2 +- diff-no-index.c | 4 ++-- dir.c | 2 +- gettext.c | 2 +- rerere.c | 2 +- sha1-file.c | 6 +++--- submodule.c | 4 ++-- trace2/tr2_dst.c | 2 +- worktree.c | 2 +- 17 files changed, 27 insertions(+), 33 deletions(-)
abspath.c +1 -1
@@ -5,7 +5,7 @@* symlink to a directory, we do not want to say it is a directory when * dealing with tracked content in the working tree. */ -int is_directory (const char *path) +int dir_exists (const char *path) {struct stat st;return (!stat (path, &st) &&S_ISDIR (st.st_mode));
builtin/am.c +1 -1
@@ -576,7 +576,7 @@ static int detect_patch_format(const char **paths)/* * We default to mbox format if input is from stdin and for directories */ -if (!*paths || !strcmp (*paths,"-" ) ||is_directory (*paths)) +if (!*paths || !strcmp (*paths,"-" ) ||dir_exists (*paths))return PATCH_FORMAT_MBOX ;/*
builtin/clone.c +0 -6
@@ -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;
builtin/mv.c +1 -1
@@ -152,7 +152,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)* "git mv directory no-such-dir/". */ flags =KEEP_TRAILING_SLASH ; -if (argc ==1 &&is_directory (argv[0 ]) && !is_directory (argv[1 ])) +if (argc ==1 &&dir_exists (argv[0 ]) && !dir_exists (argv[1 ])) flags =0 ; dest_path =internal_prefix_pathspec (prefix, argv + argc,1 , flags); submodule_gitfile =xcalloc (argc,sizeof (char *));
builtin/rebase.c +5 -5
@@ -275,7 +275,7 @@ static int init_basic_state(struct replay_opts *opts, const char *head_name, {FILE *interactive; -if (!is_directory (merge_dir ()) &&mkdir_in_gitdir (merge_dir ())) +if (!dir_exists (merge_dir ()) &&mkdir_in_gitdir (merge_dir ()))return error_errno (_ ("could not create temporary %s" ),merge_dir ());delete_reflog ("REBASE_HEAD" );
@@ -1068,7 +1068,7 @@ static int run_am(struct rebase_options *opts)return move_to_original_branch (opts); } -if (is_directory (opts->state_dir)) +if (dir_exists (opts->state_dir))rebase_write_basic_state (opts);return status;
@@ -1529,13 +1529,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if (file_exists (buf.buf))die (_ ("It looks like 'git am' is in progress. Cannot rebase." )); -if (is_directory (apply_dir ())) { +if (dir_exists (apply_dir ())) { options.type =REBASE_AM ; options.state_dir =apply_dir (); - }else if (is_directory (merge_dir ())) { + }else if (dir_exists (merge_dir ())) {strbuf_reset (&buf);strbuf_addf (&buf,"%s/rewritten" ,merge_dir ()); -if (is_directory (buf.buf)) { +if (dir_exists (buf.buf)) { options.type =REBASE_PRESERVE_MERGES ; options.flags |=REBASE_INTERACTIVE_EXPLICIT ; }else {
builtin/submodule--helper.c +2 -2
@@ -1096,7 +1096,7 @@ static void deinit_submodule(const char *path, const char *prefix, displaypath =get_submodule_displaypath (path, prefix);/* remove the submodule work tree (unless the user already did it) */ -if (is_directory (path)) { +if (dir_exists (path)) {struct strbuf sb_rm =STRBUF_INIT ;const char *format;
@@ -1105,7 +1105,7 @@ static void deinit_submodule(const char *path, const char *prefix,* NEEDSWORK: instead of dying, automatically call * absorbgitdirs and (possibly) warn. */ -if (is_directory (sub_git_dir)) +if (dir_exists (sub_git_dir)) die(_("Submodule work tree '%s' contains a .git " "directory (use 'rm -rf' if you really want " "to remove it including all of its history)"),
builtin/worktree.c +3 -3
@@ -75,7 +75,7 @@ static int prune_worktree(const char *id, struct strbuf *reason)size_t len;ssize_t read_result; -if (!is_directory (git_path ("worktrees/%s" , id))) { +if (!dir_exists (git_path ("worktrees/%s" , id))) {strbuf_addf (reason,_ ("Removing worktrees/%s: not a valid directory" ), id);return 1 ; }
@@ -738,7 +738,7 @@ static void validate_no_submodules(const struct worktree *wt)struct strbuf path =STRBUF_INIT ;int i, found_submodules =0 ; -if (is_directory (worktree_git_path (wt,"modules" ))) { +if (dir_exists (worktree_git_path (wt,"modules" ))) {/* * There could be false positives, e.g. the "modules" * directory exists but is empty. But it's a rare case and
@@ -799,7 +799,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)die (_ ("'%s' is not a working tree" ), av[0 ]);if (is_main_worktree (wt))die (_ ("'%s' is a main working tree" ), av[0 ]); -if (is_directory (dst.buf)) { +if (dir_exists (dst.buf)) {const char *sep =find_last_dir_sep (wt->path);if (!sep)
cache.h +1 -1
@@ -1274,7 +1274,7 @@ static inline int is_absolute_path(const char *path) {return is_dir_sep (path[0 ]) ||has_dos_drive_prefix (path); } -int is_directory (const char *); +int dir_exists (const char *);char *strbuf_realpath (struct strbuf *resolved,const char *path,int die_on_error);const char *real_path (const char *path);
daemon.c +1 -1
@@ -1455,7 +1455,7 @@ int cmd_main(int argc, const char **argv)if (strict_paths && (!ok_paths || !*ok_paths))die ("option --strict-paths requires a whitelist" ); -if (base_path && !is_directory (base_path)) +if (base_path && !dir_exists (base_path))die ("base-path '%s' does not exist or is not a directory" , base_path);
diff-no-index.c +2 -2
@@ -221,8 +221,8 @@ static void fixup_paths(const char **path, struct strbuf *replacement)if (path[0 ] == file_from_standard_input || path[1 ] == file_from_standard_input)return ; - isdir0 =is_directory (path[0 ]); - isdir1 =is_directory (path[1 ]); + isdir0 =dir_exists (path[0 ]); + isdir1 =dir_exists (path[1 ]);if (isdir0 == isdir1)return ;if (isdir0) {
dir.c +1 -1
@@ -2100,7 +2100,7 @@ static int treat_leading_path(struct dir_struct *dir, baselen = cp - path;strbuf_setlen (&sb,0 );strbuf_add (&sb, path, baselen); -if (!is_directory (sb.buf)) +if (!dir_exists (sb.buf))break ;if (simplify_away (sb.buf, sb.len, pathspec))break ;
gettext.c +1 -1
@@ -183,7 +183,7 @@ void git_setup_gettext(void)use_gettext_poison ();/* getenv() reentrancy paranoia */ -if (!is_directory (podir)) { +if (!dir_exists (podir)) {free (p);return ; }
rerere.c +1 -1
@@ -873,7 +873,7 @@ static int is_rerere_enabled(void)if (!rerere_enabled)return 0 ; - rr_cache_exists =is_directory (git_path_rr_cache ()); + rr_cache_exists =dir_exists (git_path_rr_cache ());if (rerere_enabled <0 )return rr_cache_exists;
sha1-file.c +3 -3
@@ -448,7 +448,7 @@ static int alt_odb_usable(struct raw_object_store *o,struct object_directory *odb;/* Detect cases where alternate disappeared */ -if (!is_directory (path->buf)) { +if (!dir_exists (path->buf)) {error (_ ("object directory %s does not exist; " "check .git/objects/info/alternates" ), path->buf);
@@ -699,11 +699,11 @@ char *compute_alternate_path(const char *path, struct strbuf *err) ref_git =xstrdup (repo); } -if (!repo &&is_directory (mkpath ("%s/.git/objects" , ref_git))) { +if (!repo &&dir_exists (mkpath ("%s/.git/objects" , ref_git))) {char *ref_git_git =mkpathdup ("%s/.git" , ref_git);free (ref_git); ref_git = ref_git_git; - }else if (!is_directory (mkpath ("%s/objects" , ref_git))) { + }else if (!dir_exists (mkpath ("%s/objects" , ref_git))) {struct strbuf sb =STRBUF_INIT ; seen_error =1 ;if (get_common_dir (&sb, ref_git)) {
submodule.c +2 -2
@@ -174,7 +174,7 @@ int add_submodule_odb(const char *path) ret =strbuf_git_path_submodule (&objects_directory, path,"objects/" );if (ret) goto done; -if (!is_directory (objects_directory.buf)) { +if (!dir_exists (objects_directory.buf)) { ret =-1 ; goto done; }
@@ -1647,7 +1647,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)if (!git_dir) git_dir = buf.buf;if (!is_git_directory (git_dir)) { -if (is_directory (git_dir)) +if (dir_exists (git_dir))die (_ ("'%s' not recognized as a git repository" ), git_dir);strbuf_release (&buf);/* The submodule is not checked out, so it is not modified */
trace2/tr2_dst.c +1 -1
@@ -337,7 +337,7 @@ int tr2_dst_get_trace_fd(struct tr2_dst *dst) }if (is_absolute_path (tgt_value)) { -if (is_directory (tgt_value)) +if (dir_exists (tgt_value))return tr2_dst_try_auto_path (dst, tgt_value);else return tr2_dst_try_path (dst, tgt_value);
worktree.c +1 -1
@@ -290,7 +290,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,strbuf_addf (&wt_path,"%s/.git" , wt->path);if (is_main_worktree (wt)) { -if (is_directory (wt_path.buf)) { +if (dir_exists (wt_path.buf)) { ret =0 ; goto done; }
Signature · 1 line
gitgitgadget
Quoted 4 lines · John Lin via GitGitGadget
From: John Lin <[email protected]>
The original
is_directory()checks whether the given path exists as a directory, which makesdir_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".
Quoted 2 lines
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").
Quoted 4 lines
We decided to do the following:
- remove the originaldir_exists()
- rename the originalis_directory()todir_exists()
- use the newdir_exists()where the originaldir_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.