Re: [PATCH v7 1/2] exec_cmd: expose is_executable function

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Brandon Williams wrote:

> Move the logic for 'is_executable()' from help.c to exec_cmd.c and
> expose it so that callers from outside help.c can access the function.
> This is to enable run-command to be able to query if a file is
> executable in a future patch.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  exec_cmd.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  exec_cmd.h |  1 +
>  help.c     | 42 ------------------------------------------
>  3 files changed, 43 insertions(+), 42 deletions(-)

Makes sense.  Seems like as fine place for it.  (exec_cmd.c is mostly
an implementation detail of run_command, to hold the logic for
executing a git command.  This is another run_command helper, and it
doesn't feel illogical to find it there.  Another alternative place to
put t would be in run-command.c.)

> diff --git a/exec_cmd.c b/exec_cmd.c
> index fb94aeba9..6d9481e26 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -149,3 +149,45 @@ int execl_git_cmd(const char *cmd,...)
>  	argv[argc] = NULL;
>  	return execv_git_cmd(argv);
>  }
> +
> +int is_executable(const char *name)

nit: it's a good practice to find a logical place for a new function in
the existing file, instead of defaulting to the end.  That way, the file
is easier to read sequentially, and if two different patches want to
add a function to the same file then they are less likely to conflict.

This could go before prepare_git_cmd, for example.

With or without such a tweak,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

diff --git i/exec_cmd.c w/exec_cmd.c
index 6d9481e26d..601fbc43bc 100644
--- i/exec_cmd.c
+++ w/exec_cmd.c
@@ -104,6 +104,48 @@ void setup_path(void)
 	strbuf_release(&new_path);
 }
 
+int is_executable(const char *name)
+{
+	struct stat st;
+
+	if (stat(name, &st) || /* stat, not lstat */
+	    !S_ISREG(st.st_mode))
+		return 0;
+
+#if defined(GIT_WINDOWS_NATIVE)
+	/*
+	 * On Windows there is no executable bit. The file extension
+	 * indicates whether it can be run as an executable, and Git
+	 * has special-handling to detect scripts and launch them
+	 * through the indicated script interpreter. We test for the
+	 * file extension first because virus scanners may make
+	 * it quite expensive to open many files.
+	 */
+	if (ends_with(name, ".exe"))
+		return S_IXUSR;
+
+{
+	/*
+	 * Now that we know it does not have an executable extension,
+	 * peek into the file instead.
+	 */
+	char buf[3] = { 0 };
+	int n;
+	int fd = open(name, O_RDONLY);
+	st.st_mode &= ~S_IXUSR;
+	if (fd >= 0) {
+		n = read(fd, buf, 2);
+		if (n == 2)
+			/* look for a she-bang */
+			if (!strcmp(buf, "#!"))
+				st.st_mode |= S_IXUSR;
+		close(fd);
+	}
+}
+#endif
+	return st.st_mode & S_IXUSR;
+}
+
 const char **prepare_git_cmd(struct argv_array *out, const char **argv)
 {
 	argv_array_push(out, "git");
@@ -149,45 +191,3 @@ int execl_git_cmd(const char *cmd,...)
 	argv[argc] = NULL;
 	return execv_git_cmd(argv);
 }
-
-int is_executable(const char *name)
-{
-	struct stat st;
-
-	if (stat(name, &st) || /* stat, not lstat */
-	    !S_ISREG(st.st_mode))
-		return 0;
-
-#if defined(GIT_WINDOWS_NATIVE)
-	/*
-	 * On Windows there is no executable bit. The file extension
-	 * indicates whether it can be run as an executable, and Git
-	 * has special-handling to detect scripts and launch them
-	 * through the indicated script interpreter. We test for the
-	 * file extension first because virus scanners may make
-	 * it quite expensive to open many files.
-	 */
-	if (ends_with(name, ".exe"))
-		return S_IXUSR;
-
-{
-	/*
-	 * Now that we know it does not have an executable extension,
-	 * peek into the file instead.
-	 */
-	char buf[3] = { 0 };
-	int n;
-	int fd = open(name, O_RDONLY);
-	st.st_mode &= ~S_IXUSR;
-	if (fd >= 0) {
-		n = read(fd, buf, 2);
-		if (n == 2)
-			/* look for a she-bang */
-			if (!strcmp(buf, "#!"))
-				st.st_mode |= S_IXUSR;
-		close(fd);
-	}
-}
-#endif
-	return st.st_mode & S_IXUSR;
-}
diff --git i/exec_cmd.h w/exec_cmd.h
index 48dd18a0d4..5e8200b952 100644
--- i/exec_cmd.h
+++ w/exec_cmd.h
@@ -7,11 +7,11 @@ extern void git_set_argv_exec_path(const char *exec_path);
 extern void git_extract_argv0_path(const char *path);
 extern const char *git_exec_path(void);
 extern void setup_path(void);
+extern int is_executable(const char *name);
 extern const char **prepare_git_cmd(struct argv_array *out, const char **argv);
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
 LAST_ARG_MUST_BE_NULL
 extern int execl_git_cmd(const char *cmd, ...);
 extern char *system_path(const char *path);
-extern int is_executable(const char *name);
 
 #endif /* GIT_EXEC_CMD_H */