| Message ID | pull.587.git.1584997990694.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix dir sep handling of GIT_ASKPASS on Windows | expand |
"András Kucsma via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Andras Kucsma <r0maikx02b@gmail.com> > > On Windows with git installed through cygwin, GIT_ASKPASS failed to run > for relative and absolute paths containing only backslashes as directory > separators. > > The reason was that git assumed that if there are no forward slashes in > the executable path, it has to search for the executable on the PATH. Also if I were reading the discussion correctly, there was a doubt about locate_in_PATH() that may not work on Windows for at least two reasons. Is it OK to ignore these issues, and if so why? I know if you have a full path, a broken locate_in_PATH() would be skipped and won't cause an immediate issue, but this change to make the code realize that "a\\b" is not asking to search in %PATH% feels just a beginning of a fix, not the whole fix, at least to me. > The fix is to look for OS specific directory separators, not just > forward slashes. Yes, but it is quite unfortunate that you would use a function that has to scan the string to the end because it asks for the last one. Perhaps introduce -------------------------------------------------- #ifndef has_dir_sep static inline int git_has_dir_sep(const char *path) { return !!strchr(path, '/'); } #define has_dir_sep(path) git_has_dir_sep(path) #endif -------------------------------------------------- in <git-compat-util.h>, with a replacement definition in <compat/win32/path-utils.h> that may read -------------------------------------------------- #define has_dir_sep(path) win32_has_dir_sep(path) static inline int has_dir_sep(const char *path) { /* * See how long the non-separator part of the given path is, and * if and only if it covers the whole path (i.e. path[len] is NUL), * there is no separator in the path---otherwise there is a separaptor. */ size_t len = strcspn(path, "/\\"); return !!path[len]; } -------------------------------------------------- and use that instead? > diff --git a/run-command.c b/run-command.c > index f5e1149f9b3..9fcc12ebf9c 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -421,12 +421,12 @@ static int prepare_cmd(struct argv_array *out, const struct child_process *cmd) > } > > /* > - * If there are no '/' characters in the command then perform a path > - * lookup and use the resolved path as the command to exec. If there > - * are '/' characters, we have exec attempt to invoke the command > - * directly. > + * If there are no dir separator characters in the command then perform > + * a path lookup and use the resolved path as the command to exec. If > + * there are dir separator characters, we have exec attempt to invoke > + * the command directly. > */ > - if (!strchr(out->argv[1], '/')) { > + if (find_last_dir_sep(out->argv[1]) == NULL) { > char *program = locate_in_PATH(out->argv[1]); > if (program) { > free((char *)out->argv[1]); > > base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925
diff --git a/run-command.c b/run-command.c index f5e1149f9b3..9fcc12ebf9c 100644 --- a/run-command.c +++ b/run-command.c @@ -421,12 +421,12 @@ static int prepare_cmd(struct argv_array *out, const struct child_process *cmd) } /* - * If there are no '/' characters in the command then perform a path - * lookup and use the resolved path as the command to exec. If there - * are '/' characters, we have exec attempt to invoke the command - * directly. + * If there are no dir separator characters in the command then perform + * a path lookup and use the resolved path as the command to exec. If + * there are dir separator characters, we have exec attempt to invoke + * the command directly. */ - if (!strchr(out->argv[1], '/')) { + if (find_last_dir_sep(out->argv[1]) == NULL) { char *program = locate_in_PATH(out->argv[1]); if (program) { free((char *)out->argv[1]);