[Outreachy][PATCH] abspath: reconcile `dir_exists()` and `is_directory()`

[Outreachy][PATCH] abspath: reconcile `dir_exists()` and `is_directory()`

[Miriam Rubio]

The dir_exists() function in builtin/clone.c is marked as static, so
nobody can use it outside builtin/clone.c.

There is also is_directory() which obviously tries to do the very same, but it uses a name that few developers will think of when they see file_exists() and look for the equivalent function to see whether a given directory exists.

Let's reconcile these functions by renaming is_directory() to dir_exists() and using it also in builtin/clone.c.

Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 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 +-
 compat/mingw.c              |  2 +-
 daemon.c                    |  2 +-
 diff-no-index.c             |  4 ++--
 dir.c                       |  2 +-
 gettext.c                   |  2 +-
 rerere.c                    |  2 +-
 sequencer.c                 |  2 +-
 sha1-file.c                 |  8 ++++----
 submodule.c                 |  4 ++--
 trace2/tr2_dst.c            |  2 +-
 worktree.c                  |  2 +-
 19 files changed, 30 insertions(+), 36 deletions(-)

diff --git a/abspath.c b/abspath.c
index 9857985329..13bd92eca5 100644
--- a/abspath.c
+++ b/abspath.c
@@ -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));
diff --git a/builtin/am.c b/builtin/am.c
index 8181c2aef3..f872125fc7 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -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;
 
 	/*
diff --git a/builtin/clone.c b/builtin/clone.c
index c46ee29f0a..f89938bf94 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;
diff --git a/builtin/mv.c b/builtin/mv.c
index be15ba7044..194e1618a0 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -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 *));
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 4a20582e72..c66cdf729b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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 {
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 2c2395a620..8df36e06b4 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -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)"),
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 4de44f579a..a69a1e5612 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -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)
diff --git a/cache.h b/cache.h
index 04cabaac11..596de2db38 100644
--- a/cache.h
+++ b/cache.h
@@ -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);
diff --git a/compat/mingw.c b/compat/mingw.c
index 6b765d936c..9e391a2a74 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2352,7 +2352,7 @@ static void setup_windows_environment(void)
 			strbuf_addstr(&buf, tmp);
 			if ((tmp = getenv("HOMEPATH"))) {
 				strbuf_addstr(&buf, tmp);
-				if (is_directory(buf.buf))
+				if (dir_exists(buf.buf))
 					setenv("HOME", buf.buf, 1);
 				else
 					tmp = NULL; /* use $USERPROFILE */
diff --git a/daemon.c b/daemon.c
index 9d2e0d20ef..050c1ffacf 100644
--- a/daemon.c
+++ b/daemon.c
@@ -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 --git a/diff-no-index.c b/diff-no-index.c
index 7814eabfe0..7f6e17fc76 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -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) {
diff --git a/dir.c b/dir.c
index 61f559f980..fd22bc1866 100644
--- a/dir.c
+++ b/dir.c
@@ -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;
diff --git a/gettext.c b/gettext.c
index 35d2c1218d..c02f6675fa 100644
--- a/gettext.c
+++ b/gettext.c
@@ -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;
 	}
diff --git a/rerere.c b/rerere.c
index 3e51fdfe58..1d6a4b8df2 100644
--- a/rerere.c
+++ b/rerere.c
@@ -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;
 
diff --git a/sequencer.c b/sequencer.c
index 9d5964fd81..ee27b7b1cd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2813,7 +2813,7 @@ int sequencer_skip(struct repository *r, struct replay_opts *opts)
 
 	if (skip_single_pick())
 		return error(_("failed to skip the commit"));
-	if (!is_directory(git_path_seq_dir()))
+	if (!dir_exists(git_path_seq_dir()))
 		return 0;
 
 	return sequencer_continue(r, opts);
diff --git a/sha1-file.c b/sha1-file.c
index 188de57634..7fcf89d431 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -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)) {
@@ -821,7 +821,7 @@ static int refs_from_alternate_cb(struct object_directory *e,
 
 	/* Is this a git repository with refs? */
 	strbuf_addstr(&path, "/refs");
-	if (!is_directory(path.buf))
+	if (!dir_exists(path.buf))
 		goto out;
 	strbuf_setlen(&path, base_len);
 
diff --git a/submodule.c b/submodule.c
index 0f199c5137..870f35cd56 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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 */
diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c
index ae052a07fe..fe0b4d94ff 100644
--- a/trace2/tr2_dst.c
+++ b/trace2/tr2_dst.c
@@ -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);
diff --git a/worktree.c b/worktree.c
index 5b4793caa3..e2d2e2dbf1 100644
--- a/worktree.c
+++ b/worktree.c
@@ -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;
 		}
-- 
2.21.0 (Apple Git-122)

Re: [Outreachy][PATCH] abspath: reconcile `dir_exists()` and `is_directory()`

[SZEDER Gábor]

On Thu, Oct 24, 2019 at 11:27:45AM +0200, Miriam Rubio wrote:
> The dir_exists() function in builtin/clone.c is marked as static, so
> nobody can use it outside builtin/clone.c.
> 
> There is also is_directory() which obviously tries to do the very same, but it uses a name that few developers will think of when they see file_exists() and look for the equivalent function to see whether a given directory exists.
> 
> Let's reconcile these functions by renaming is_directory() to dir_exists() and using it also in builtin/clone.c.

Please wrap the proposed log message at about 70 characters width;
that way it will look much better in 'git log' in a standard 80 char
wide terminal.

I think this is a cleanup worth doing, but...

> diff --git a/abspath.c b/abspath.c
> index 9857985329..13bd92eca5 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -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));

Note the '&& S_ISDIR(st.st_mode)', making sure that the given path is
in fact a directory.  Good.

> diff --git a/builtin/clone.c b/builtin/clone.c
> index c46ee29f0a..f89938bf94 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);

But look at this, it only checks that the given path exists, but it
could be a regular file or any other kind of path other than a
directory as well!

So this function clearly doesn't do what it's name suggests.  That's
bad.

Unfortunately, it gets worse: some of its callsites in
'builtin/clone.c' do expect it to check the existence of _any_ path,
not just a directory.

The first callsite is:

    dest_exists = dir_exists(dir);
    if (dest_exists && !is_empty_dir(dir))
            die(_("destination path '%s' already exists and is not "
                    "an empty directory."), dir);

I think this actually means path_exists(): if a file, or any other
kind of path with the given name were to exist, then we should die()
showing this error message, but after changing dir_exists() to make
sure that the path is indeed a directory we won't:

  # create a 'git' _file_
  $ >git
  # current git master:
  $ git clone https://github.com/git/git
  fatal: destination path 'git' already exists and is not an empty directory.
  # with this patch:
  $ ~/src/git/git clone https://github.com/git/git
  fatal: could not create work tree dir 'git': File exists

So the command still fails, which is good, but with a different error
message.  The test suite doesn't catch this, because the test case
looking at this scenario ('clone to an existing path' in
'./t5601-clone.sh') only checks that 'git clone' fails, but it doesn't
check whether it failed with the right error message.

Now, that other error message comes after a failed mkdir() call later
on which should have created the work tree.  So it begs the question
what would happen when a file is in the way of a bare clone:

  $ >git.git
  $ git clone --bare https://github.com/git/git
  fatal: destination path 'git.git' already exists and is not an empty directory.
  $ ~/src/git/git clone --bare https://github.com/git/git
  Cloning into bare repository 'git.git'...
  fatal: invalid gitfile format: /home/szeder/src/git/tmp/git.git

Then the next callsite looks like it meant path_exists() as well, but
I didn't try to make it fail or show different behavior:

    work_tree = getenv("GIT_WORK_TREE");
    if (work_tree && dir_exists(work_tree))
            die(_("working tree '%s' already exists."), work_tree);

And there is a third callsite, but I'm not sure what it is about, and,
consequently, what is really meant with dir_exists() here:

    if (real_git_dir) {
            if (dir_exists(real_git_dir))
                    junk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;

Re: [Outreachy][PATCH] abspath: reconcile `dir_exists()` and `is_directory()`

[Jeff King]

On Thu, Oct 24, 2019 at 01:41:48PM +0200, SZEDER Gábor wrote:

> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index c46ee29f0a..f89938bf94 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);
> 
> But look at this, it only checks that the given path exists, but it
> could be a regular file or any other kind of path other than a
> directory as well!
> 
> So this function clearly doesn't do what it's name suggests.  That's
> bad.
> 
> Unfortunately, it gets worse: some of its callsites in
> 'builtin/clone.c' do expect it to check the existence of _any_ path,
> not just a directory.

Yes, that's the reason for the funny name (and the fact that it was
never re-factored to use is_directory() in the first place). There's
some more discussion in:

  https://public-inbox.org/git/xmqqbmi9dw55.fsf@gitster.mtv.corp.google.com/

and its subthread.

-Peff

Re: [Outreachy][PATCH] abspath: reconcile `dir_exists()` and `is_directory()`

[Emily Shaffer]

On Thu, Oct 24, 2019 at 02:13:45PM -0400, Jeff King wrote:
> On Thu, Oct 24, 2019 at 01:41:48PM +0200, SZEDER Gábor wrote:
> 
> > > diff --git a/builtin/clone.c b/builtin/clone.c
> > > index c46ee29f0a..f89938bf94 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);
> > 
> > But look at this, it only checks that the given path exists, but it
> > could be a regular file or any other kind of path other than a
> > directory as well!
> > 
> > So this function clearly doesn't do what it's name suggests.  That's
> > bad.
> > 
> > Unfortunately, it gets worse: some of its callsites in
> > 'builtin/clone.c' do expect it to check the existence of _any_ path,
> > not just a directory.
> 
> Yes, that's the reason for the funny name (and the fact that it was
> never re-factored to use is_directory() in the first place). There's
> some more discussion in:
> 
>   https://public-inbox.org/git/xmqqbmi9dw55.fsf@gitster.mtv.corp.google.com/
> 
> and its subthread.

Hm. Then, is the solution to use dir_exists() for "a directory exists
here" and also add path_exists() for "literally anything exists here"?
That seems like it's still a pretty minor change. It'd be nice to
un-stick our Outreachy applicant :)

 - Emily

Re: [Outreachy][PATCH] abspath: reconcile `dir_exists()` and `is_directory()`

[Jeff King]

On Thu, Oct 24, 2019 at 01:45:00PM -0700, Emily Shaffer wrote:

> > Yes, that's the reason for the funny name (and the fact that it was
> > never re-factored to use is_directory() in the first place). There's
> > some more discussion in:
> > 
> >   https://public-inbox.org/git/xmqqbmi9dw55.fsf@gitster.mtv.corp.google.com/
> > 
> > and its subthread.
> 
> Hm. Then, is the solution to use dir_exists() for "a directory exists
> here" and also add path_exists() for "literally anything exists here"?
> That seems like it's still a pretty minor change. It'd be nice to
> un-stick our Outreachy applicant :)

Yeah, I think one path forward could be:

  - add path_exists(); this will work the same as file_exists(), but is
    a better name. Keep file_exists() for now, but put a comment that
    new calls should use path_exists().

  - use path_exists() in builtin/clone.c, ditching its custom
    dir_exists()

  - (optional) start converting file_exists() calls to path_exists(),
    after confirming what each call wants (just files, or any path)

    I one really does want to check for a regular file, then we'd need
    to figure out what the "does this regular file exist" function is
    called. I have a suspicion that there won't be any such callers, so
    we can punt on it until then.

-Peff

Re: [Outreachy][PATCH] abspath: reconcile `dir_exists()` and `is_directory()`

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, I think one path forward could be:
>
>   - add path_exists(); this will work the same as file_exists(), but is
>     a better name. Keep file_exists() for now, but put a comment that
>     new calls should use path_exists().
>
>   - use path_exists() in builtin/clone.c, ditching its custom
>     dir_exists()

Both are of immediate value ;-)

>   - (optional) start converting file_exists() calls to path_exists(),
>     after confirming what each call wants (just files, or any path)

That is of lessor urgency but the result has a good documentation
value.

Re: [Outreachy][PATCH] abspath: reconcile `dir_exists()` and `is_directory()`

[Miriam R.]

El jue., 24 oct. 2019 a las 13:41, SZEDER Gábor
(<szeder.dev@gmail.com>) escribió:
>
> On Thu, Oct 24, 2019 at 11:27:45AM +0200, Miriam Rubio wrote:
> > The dir_exists() function in builtin/clone.c is marked as static, so
> > nobody can use it outside builtin/clone.c.
> >
> > There is also is_directory() which obviously tries to do the very same, but it uses a name that few developers will think of when they see file_exists() and look for the equivalent function to see whether a given directory exists.
> >
> > Let's reconcile these functions by renaming is_directory() to dir_exists() and using it also in builtin/clone.c.
>
> Please wrap the proposed log message at about 70 characters width;
> that way it will look much better in 'git log' in a standard 80 char
> wide terminal.
>
> I think this is a cleanup worth doing, but...

Thanks for the guidelines!

>
> > diff --git a/abspath.c b/abspath.c
> > index 9857985329..13bd92eca5 100644
> > --- a/abspath.c
> > +++ b/abspath.c
> > @@ -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));
>
> Note the '&& S_ISDIR(st.st_mode)', making sure that the given path is
> in fact a directory.  Good.
>
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index c46ee29f0a..f89938bf94 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);
>
> But look at this, it only checks that the given path exists, but it
> could be a regular file or any other kind of path other than a
> directory as well!
>
> So this function clearly doesn't do what it's name suggests.  That's
> bad.
>
> Unfortunately, it gets worse: some of its callsites in
> 'builtin/clone.c' do expect it to check the existence of _any_ path,
> not just a directory.
>
> The first callsite is:
>
>     dest_exists = dir_exists(dir);
>     if (dest_exists && !is_empty_dir(dir))
>             die(_("destination path '%s' already exists and is not "
>                     "an empty directory."), dir);
>
> I think this actually means path_exists(): if a file, or any other
> kind of path with the given name were to exist, then we should die()
> showing this error message, but after changing dir_exists() to make
> sure that the path is indeed a directory we won't:
>
>   # create a 'git' _file_
>   $ >git
>   # current git master:
>   $ git clone https://github.com/git/git
>   fatal: destination path 'git' already exists and is not an empty directory.
>   # with this patch:
>   $ ~/src/git/git clone https://github.com/git/git
>   fatal: could not create work tree dir 'git': File exists
>
> So the command still fails, which is good, but with a different error
> message.  The test suite doesn't catch this, because the test case
> looking at this scenario ('clone to an existing path' in
> './t5601-clone.sh') only checks that 'git clone' fails, but it doesn't
> check whether it failed with the right error message.
>
> Now, that other error message comes after a failed mkdir() call later
> on which should have created the work tree.  So it begs the question
> what would happen when a file is in the way of a bare clone:
>
>   $ >git.git
>   $ git clone --bare https://github.com/git/git
>   fatal: destination path 'git.git' already exists and is not an empty directory.
>   $ ~/src/git/git clone --bare https://github.com/git/git
>   Cloning into bare repository 'git.git'...
>   fatal: invalid gitfile format: /home/szeder/src/git/tmp/git.git
>
> Then the next callsite looks like it meant path_exists() as well, but
> I didn't try to make it fail or show different behavior:
>
>     work_tree = getenv("GIT_WORK_TREE");
>     if (work_tree && dir_exists(work_tree))
>             die(_("working tree '%s' already exists."), work_tree);
>
> And there is a third callsite, but I'm not sure what it is about, and,
> consequently, what is really meant with dir_exists() here:
>
>     if (real_git_dir) {
>             if (dir_exists(real_git_dir))
>                     junk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;
>
>

Here is my proposal:

- Rename is_directory() to dir_exists(), as it is the equivalent to
file_exists().
- Rename dir_exists() to path_exists() so it describes its real
behavior, and either leave it as static inside clone.c or extract it
to a common header such as that containing dir_exists().

What do you think?

Re: [Outreachy][PATCH] abspath: reconcile `dir_exists()` and `is_directory()`

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> The first callsite is:
>
>     dest_exists = dir_exists(dir);
>     if (dest_exists && !is_empty_dir(dir))
>             die(_("destination path '%s' already exists and is not "
>                     "an empty directory."), dir);

Yup.  The primary/original reason why the helper exists is to see if
we can create directory there, so the function is asking "is this
path taken?"  It might have been cleaner to do all of these without
using such a helper function and instead take the safer approach to
"try mkdir, and if we fail, complian", which is race-free.  But the
above is what we have now X-<.

Re: [Outreachy][PATCH] abspath: reconcile `dir_exists()` and `is_directory()`

[Miriam R.]

Ok, then after discussion, finally the issue tasks would be:

- Add path_exists() that will work same as file_exists(), keeping for
now the latter.
- Use path_exists() instead of dir_exists() in builtin/clone.c.

And also:
- Rename is_directory() to dir_exists(), as it is the equivalent to
path_exists()/file_exists(), isn't it?

Best,
Miriam


El vie., 25 oct. 2019 a las 4:46, Junio C Hamano (<gitster@pobox.com>) escribió:
>
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
> > The first callsite is:
> >
> >     dest_exists = dir_exists(dir);
> >     if (dest_exists && !is_empty_dir(dir))
> >             die(_("destination path '%s' already exists and is not "
> >                     "an empty directory."), dir);
>
> Yup.  The primary/original reason why the helper exists is to see if
> we can create directory there, so the function is asking "is this
> path taken?"  It might have been cleaner to do all of these without
> using such a helper function and instead take the safer approach to
> "try mkdir, and if we fail, complian", which is race-free.  But the
> above is what we have now X-<.
>

Re: [Outreachy][PATCH] abspath: reconcile `dir_exists()` and `is_directory()`

[Junio C Hamano]

"Miriam R." <mirucam@gmail.com> writes:

> Ok, then after discussion, finally the issue tasks would be:
>
> - Add path_exists() that will work same as file_exists(), keeping for
> now the latter.
> - Use path_exists() instead of dir_exists() in builtin/clone.c.

Sounds about right.

> And also:
> - Rename is_directory() to dir_exists(), as it is the equivalent to
> path_exists()/file_exists(), isn't it?

I wouldn't go there in the same series, if I were doing it.  I'd
expect that such a patch would be more noisy than it is worth if
done in a single step.  In order to avoid becoming a hindrance to
other topics in flight, an ideal series to do so would support the
same functionality with both old and new names, convert code that
use the old name to use the new name, possibly in multiple patches
to avoid unnecessary textual conflicts (i.e. some of these patches
made to areas that are seeing active development will be discarded
and need to be retried later when the area is more quiet) and then
finally the function wither the old name gets removed.

You would not want to mix the first two bullet points that are
relatively isolated with such a long transition.

Re: [Outreachy][PATCH] abspath: reconcile `dir_exists()` and `is_directory()`

[Christian Couder]

On Fri, Oct 25, 2019 at 11:43 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Miriam R." <mirucam@gmail.com> writes:
>
> > Ok, then after discussion, finally the issue tasks would be:
> >
> > - Add path_exists() that will work same as file_exists(), keeping for
> > now the latter.
> > - Use path_exists() instead of dir_exists() in builtin/clone.c.
>
> Sounds about right.
>
> > And also:
> > - Rename is_directory() to dir_exists(), as it is the equivalent to
> > path_exists()/file_exists(), isn't it?
>
> I wouldn't go there in the same series, if I were doing it.  I'd
> expect that such a patch would be more noisy than it is worth if
> done in a single step.  In order to avoid becoming a hindrance to
> other topics in flight, an ideal series to do so would support the
> same functionality with both old and new names, convert code that
> use the old name to use the new name, possibly in multiple patches
> to avoid unnecessary textual conflicts (i.e. some of these patches
> made to areas that are seeing active development will be discarded
> and need to be retried later when the area is more quiet) and then
> finally the function wither the old name gets removed.
>
> You would not want to mix the first two bullet points that are
> relatively isolated with such a long transition.

Yeah, and for a micro-project it is more than enough if you only work
on the first two bullet points.

Re: [Outreachy][PATCH] abspath: reconcile `dir_exists()` and `is_directory()`

[Miriam R.]

Ok! Thanks to everyone.

Best,
Miriam

El vie., 25 oct. 2019 a las 16:48, Christian Couder
(<christian.couder@gmail.com>) escribió:
>
> On Fri, Oct 25, 2019 at 11:43 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > "Miriam R." <mirucam@gmail.com> writes:
> >
> > > Ok, then after discussion, finally the issue tasks would be:
> > >
> > > - Add path_exists() that will work same as file_exists(), keeping for
> > > now the latter.
> > > - Use path_exists() instead of dir_exists() in builtin/clone.c.
> >
> > Sounds about right.
> >
> > > And also:
> > > - Rename is_directory() to dir_exists(), as it is the equivalent to
> > > path_exists()/file_exists(), isn't it?
> >
> > I wouldn't go there in the same series, if I were doing it.  I'd
> > expect that such a patch would be more noisy than it is worth if
> > done in a single step.  In order to avoid becoming a hindrance to
> > other topics in flight, an ideal series to do so would support the
> > same functionality with both old and new names, convert code that
> > use the old name to use the new name, possibly in multiple patches
> > to avoid unnecessary textual conflicts (i.e. some of these patches
> > made to areas that are seeing active development will be discarded
> > and need to be retried later when the area is more quiet) and then
> > finally the function wither the old name gets removed.
> >
> > You would not want to mix the first two bullet points that are
> > relatively isolated with such a long transition.
>
> Yeah, and for a micro-project it is more than enough if you only work
> on the first two bullet points.

Re: [Outreachy][PATCH] abspath: reconcile `dir_exists()` and `is_directory()`

[Miriam R.]

Dear all,
there is already a static function called path_exists() in archive.c
so project does not compile.

Maybe we could change the name of this static function and its
reference in archive.c like archive_path_exists() for example, or some
other you find more suitable.

Best,
Miriam

El vie., 25 oct. 2019 a las 17:23, Miriam R. (<mirucam@gmail.com>) escribió:
>
> Ok! Thanks to everyone.
>
> Best,
> Miriam
>
> El vie., 25 oct. 2019 a las 16:48, Christian Couder
> (<christian.couder@gmail.com>) escribió:
> >
> > On Fri, Oct 25, 2019 at 11:43 AM Junio C Hamano <gitster@pobox.com> wrote:
> > >
> > > "Miriam R." <mirucam@gmail.com> writes:
> > >
> > > > Ok, then after discussion, finally the issue tasks would be:
> > > >
> > > > - Add path_exists() that will work same as file_exists(), keeping for
> > > > now the latter.
> > > > - Use path_exists() instead of dir_exists() in builtin/clone.c.
> > >
> > > Sounds about right.
> > >
> > > > And also:
> > > > - Rename is_directory() to dir_exists(), as it is the equivalent to
> > > > path_exists()/file_exists(), isn't it?
> > >
> > > I wouldn't go there in the same series, if I were doing it.  I'd
> > > expect that such a patch would be more noisy than it is worth if
> > > done in a single step.  In order to avoid becoming a hindrance to
> > > other topics in flight, an ideal series to do so would support the
> > > same functionality with both old and new names, convert code that
> > > use the old name to use the new name, possibly in multiple patches
> > > to avoid unnecessary textual conflicts (i.e. some of these patches
> > > made to areas that are seeing active development will be discarded
> > > and need to be retried later when the area is more quiet) and then
> > > finally the function wither the old name gets removed.
> > >
> > > You would not want to mix the first two bullet points that are
> > > relatively isolated with such a long transition.
> >
> > Yeah, and for a micro-project it is more than enough if you only work
> > on the first two bullet points.

Re: [Outreachy][PATCH] abspath: reconcile `dir_exists()` and `is_directory()`

[Christian Couder]

Hi Miriam,

On Sat, Oct 26, 2019 at 5:31 PM Miriam R. <mirucam@gmail.com> wrote:
>
> Dear all,
> there is already a static function called path_exists() in archive.c
> so project does not compile.
>
> Maybe we could change the name of this static function and its
> reference in archive.c like archive_path_exists() for example, or some
> other you find more suitable.

Yeah, I think renaming the function archive_path_exists() in a
preparatory patch would be a good way to move forward on this.

Best,
Christian.

Re: [Outreachy][PATCH] abspath: reconcile `dir_exists()` and `is_directory()`

[Miriam R.]

El sáb., 26 oct. 2019 a las 20:05, Christian Couder
(<christian.couder@gmail.com>) escribió:
>
> Hi Miriam,
>
> On Sat, Oct 26, 2019 at 5:31 PM Miriam R. <mirucam@gmail.com> wrote:
> >
> > Dear all,
> > there is already a static function called path_exists() in archive.c
> > so project does not compile.
> >
> > Maybe we could change the name of this static function and its
> > reference in archive.c like archive_path_exists() for example, or some
> > other you find more suitable.
>
> Yeah, I think renaming the function archive_path_exists() in a
> preparatory patch would be a good way to move forward on this.
>
> Best,
> Christian.

Great!
Thank you, Christian.