[v3,04/13] run-command: make `exists_in_PATH()` non-static
diff mbox series

Message ID 20200208090704.26506-5-mirucam@gmail.com
State New
Headers show
Series
  • Finish converting git bisect to C part 1
Related show

Commit Message

Miriam Rubio Feb. 8, 2020, 9:06 a.m. UTC
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.

`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(-)

Comments

René Scharfe Feb. 8, 2020, 10:55 a.m. UTC | #1
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é
Miriam Rubio Feb. 9, 2020, 9:59 a.m. UTC | #2
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é
Taylor Blau Feb. 9, 2020, 11:16 p.m. UTC | #3
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
Miriam Rubio Feb. 10, 2020, 4:05 p.m. UTC | #4
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é

Patch
diff mbox series

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