Skip to content

refactor: rename is_directory() to dir_exists() and use it in clone.c #271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

refactor: rename is_directory() to dir_exists() and use it in clone.c #271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion abspath.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* symlink to a directory, we do not want to say it is a directory when
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"John Lin via GitGitGadget" <[email protected]> writes:

> 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.

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".

> 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").

> 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

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.

* dealing with tracked content in the working tree.
*/
int is_directory(const char *path)
int dir_exists(const char *path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Maybe the commit message could talk about the value of reconciling functionality, about consistency in naming (definitely mention that there is a file_exists() already), and it should probably talk about the differences of the is_directory() and the dir_exists() function, and that the former is a superset of the latter, but the latter has the better name.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I'll work on it. Thank you.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already updated the commit message. Since I am not a native speaker, please feel free to correct me if you find the sentences weird.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Please note that I am also not a native speaker...)

{
struct stat st;
return (!stat(path, &st) && S_ISDIR(st.st_mode));
Expand Down
2 changes: 1 addition & 1 deletion builtin/am.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/*
Expand Down
6 changes: 0 additions & 6 deletions builtin/clone.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion builtin/mv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 *));
Expand Down
10 changes: 5 additions & 5 deletions builtin/rebase.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions builtin/submodule--helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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)"),
Expand Down
6 changes: 3 additions & 3 deletions builtin/worktree.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion cache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion daemon.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
4 changes: 2 additions & 2 deletions diff-no-index.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion dir.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion gettext.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion rerere.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
6 changes: 3 additions & 3 deletions sha1-file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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)) {
Expand Down
4 changes: 2 additions & 2 deletions submodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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 */
Expand Down
2 changes: 1 addition & 1 deletion trace2/tr2_dst.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion worktree.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down