Re: [PATCH v2 1/4] run-command: make `exists_in_PATH()` non-static

[Junio C Hamano <gitster@pobox.com>]

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