| Message ID | 20210407173334.68222-2-mirucam@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Finish converting git bisect to C part 4 | expand |
On 2021-04-07 19:33:30+0200, Miriam Rubio <mirucam@gmail.com> 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. > > `exists_in_PATH()` and `locate_in_PATH()` functions don't > depend on file-local variables. Isn't this implementation detail? I think we shouldn't include them in the commit message. > > 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 | 10 ++++++++++ > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/run-command.c b/run-command.c > index be6bc128cd..210b8858f7 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -211,7 +211,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 d08414a92e..a520ad1342 100644 > --- a/run-command.h > +++ b/run-command.h > @@ -179,6 +179,16 @@ 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. I think this documentation focused too much in implementation detail, locate_in_PATH is still an internal linkage symbol at this stage. I think its mention here doesn't improve anything. Further more, "a $PATH given by parameter" is not what this function does, the function check if a given "file" is found in "$PATH" or not. I would copy 2 first paragraphs of locate_in_PATH's documentation, and append the documentation for return values instead:
Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: > On 2021-04-07 19:33:30+0200, Miriam Rubio <mirucam@gmail.com> 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. >> >> `exists_in_PATH()` and `locate_in_PATH()` functions don't >> depend on file-local variables. > > Isn't this implementation detail? I think we shouldn't include them in > the commit message. I also was scratching my head about the statement. What the sentence says is not incorrect per-se, but it was not clear what the relevance is to mention it. I suspect that it may have wanted to say "because they do not depend on any file scope statics to keep state or base their computation on, it is safe to expose them as a generally reusable public helper functions", and if so, "that's an irrelevant implementation detail" would not be a valid objection against mentioning it, but as written in the original, the sentence as a mere statement of the fact does not seem to help readers. >> +/** >> + * 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. > > I think this documentation focused too much in implementation detail, > locate_in_PATH is still an internal linkage symbol at this stage. > I think its mention here doesn't improve anything. I totally agree with this. What it does is more important. If you have to describe how it does it, it is often because you need to warn callers due to a curious implementation glitch (e.g. "this uses 4-slot rotating internal buffer, so do not expect its return value to stay stable after many calls"). In such a case, of course describing how it does it is important to help callers avoid pitfalls, but for this function, I do not see a need for that.
diff --git a/run-command.c b/run-command.c index be6bc128cd..210b8858f7 100644 --- a/run-command.c +++ b/run-command.c @@ -211,7 +211,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 d08414a92e..a520ad1342 100644 --- a/run-command.h +++ b/run-command.h @@ -179,6 +179,16 @@ 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 *file); + /** * Start a sub-process. Takes a pointer to a `struct child_process` * that specifies the details and returns pipe FDs (if requested).