| Message ID | 20200208090704.26506-5-mirucam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Finish converting git bisect to C part 1 | expand |
Am 08.02.20 um 10:06 schrieb Miriam Rubio: > From: Pranit Bauva <pranit.bauva@gmail.com> > > Removes the `static` keyword from `exists_in_PATH()` function > and declares the function in `run-command.h` file. > The function will be used in bisect_visualize() in a later > commit. I couldn't find the mentioned later commit in this series. Do you actually still need to export exists_in_PATH()? And if yes: locate_in_PATH() splits PATH by colon. That means it doesn't work on Windows, where the paths are separated by semicolons. exists_in_PATH() wraps it, so it shares that limitation. Wouldn't that cause issues for your use? René
Hi, El sáb., 8 feb. 2020 a las 11:55, René Scharfe (<l.s.r@web.de>) escribió: > > Am 08.02.20 um 10:06 schrieb Miriam Rubio: > > From: Pranit Bauva <pranit.bauva@gmail.com> > > > > Removes the `static` keyword from `exists_in_PATH()` function > > and declares the function in `run-command.h` file. > > The function will be used in bisect_visualize() in a later > > commit. > > I couldn't find the mentioned later commit in this series. Do you > actually still need to export exists_in_PATH()? This series is part of a bigger patch series (https://public-inbox.org/git/20200120143800.900-1-mirucam@gmail.com/) that has been split as a suggestion of a reviewer in order to be easily reviewed. This first part is formed of preparatory/clean-up patches and all `bisect.c` libification work. The actual patch is one of the preparatory patches and the function will be used in the upcoming patch series. > > And if yes: locate_in_PATH() splits PATH by colon. That means it > doesn't work on Windows, where the paths are separated by semicolons. > exists_in_PATH() wraps it, so it shares that limitation. Wouldn't that > cause issues for your use? Thank you, for point that out. I will check this. Best, Miriam > > René
On Sat, Feb 08, 2020 at 10:06:55AM +0100, Miriam Rubio wrote: > From: Pranit Bauva <pranit.bauva@gmail.com> > > Removes the `static` keyword from `exists_in_PATH()` function > and declares the function in `run-command.h` file. > The function will be used in bisect_visualize() in a later > commit. There may be some odd line-wrapping going on here. > `exists_in_PATH()` and `locate_in_PATH()` functions don't > depend on file-local variables. > > Mentored by: Christian Couder <chriscool@tuxfamily.org> > Mentored by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> > Signed-off-by: Miriam Rubio <mirucam@gmail.com> > --- > run-command.c | 2 +- > run-command.h | 11 +++++++++++ > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/run-command.c b/run-command.c > index f5e1149f9b..4df975178d 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -210,7 +210,7 @@ static char *locate_in_PATH(const char *file) > return NULL; > } > > -static int exists_in_PATH(const char *file) > +int exists_in_PATH(const char *file) > { > char *r = locate_in_PATH(file); > int found = r != NULL; > diff --git a/run-command.h b/run-command.h > index 592d9dc035..7c8e206d97 100644 > --- a/run-command.h > +++ b/run-command.h > @@ -172,6 +172,17 @@ void child_process_clear(struct child_process *); > > int is_executable(const char *name); > > +/** > + * Returns if a $PATH given by parameter is found or not (it is NULL). This > + * function uses locate_in_PATH() function that emulates the path search that > + * execvp would perform. Memory used to store the resultant path is freed by > + * the function. > + * > + * The caller should ensure that $PATH contains no directory > + * separators. This line-wrapping caught my eye: it looks like 'separators' would fit on the line before, or at least that this paragraph and the one above it are wrapped at two different widths. I'm not sure that it really matters, since it looks like the wrapping in this file isn't entirely consistent, but I figured that I'd point it out nonetheless. > + */ > +int exists_in_PATH(const char *); Have you considered naming this argument? It's somewhat clear from the documentation, but I don't see a reason not to name it (especially since other declarations in this file *do* name their parameters). What about: int exists_in_PATH(const char *file); To match its definition in 'run-command.c'? (Admittedly, if I had written this I'd probably call it 'entry', but they should at least be consistent). > + > /** > * Start a sub-process. Takes a pointer to a `struct child_process` > * that specifies the details and returns pipe FDs (if requested). > -- > 2.25.0 > Thanks, Taylor
Hi René, El lun., 10 feb. 2020 a las 16:43, René Scharfe (<l.s.r@web.de>) escribió: > > Hello Miriam, > > did you remove the Git mailing list from cc: intentionally? No, no. Maybe I clicked reply, instead reply all. Sorry. It is included in CC now. > > Am 10.02.20 um 12:16 schrieb Miriam R.: > > El dom., 9 feb. 2020 a las 10:59, Miriam R. (<mirucam@gmail.com>) escribió: > >> El sáb., 8 feb. 2020 a las 11:55, René Scharfe (<l.s.r@web.de>) escribió: > >>> And if yes: locate_in_PATH() splits PATH by colon. That means it > >>> doesn't work on Windows, where the paths are separated by semicolons. > >>> exists_in_PATH() wraps it, so it shares that limitation. Wouldn't that > >>> cause issues for your use? > >> > >> Thank you, for point that out. I will check this. > > > > This function is used only to test if gitk exists. But as gitk does not work > > on Windows, then I think it is all ok because the evaluation is going to > > return false and nothing has to be changed. > > Now I'm confused, because I do use gitk on Windows. The third > screenshot on https://gitforwindows.org/ shows it as well, so you don't > have to just take my word on it. Ohh, as I'm not a Windows user, and after some google searches it seems to me it doesn't. It is clear that I was not asking the correct things, hehe. Thank you for the clarification. > > > Also, as this patch is not really needed for this part1, I will move > > it to the upcoming patch series. > > That makes sense. > > I guess it's to used in the C equivalent of the Shell function > bisect_visualize(), which resolves gitk using type, calls it if it's > found and falls back to git log if it isn't. Exactly. >Why not try to exec gitk > and fall back to git log if that fails? > It seems a good solution for me :) Thank you for reviewing. Best, Miriam. > René
diff --git a/run-command.c b/run-command.c index f5e1149f9b..4df975178d 100644 --- a/run-command.c +++ b/run-command.c @@ -210,7 +210,7 @@ static char *locate_in_PATH(const char *file) return NULL; } -static int exists_in_PATH(const char *file) +int exists_in_PATH(const char *file) { char *r = locate_in_PATH(file); int found = r != NULL; diff --git a/run-command.h b/run-command.h index 592d9dc035..7c8e206d97 100644 --- a/run-command.h +++ b/run-command.h @@ -172,6 +172,17 @@ void child_process_clear(struct child_process *); int is_executable(const char *name); +/** + * Returns if a $PATH given by parameter is found or not (it is NULL). This + * function uses locate_in_PATH() function that emulates the path search that + * execvp would perform. Memory used to store the resultant path is freed by + * the function. + * + * The caller should ensure that $PATH contains no directory + * separators. + */ +int exists_in_PATH(const char *); + /** * Start a sub-process. Takes a pointer to a `struct child_process` * that specifies the details and returns pipe FDs (if requested).