Re: Re: [PATCH v2 1/8] findcmd: parameterize path variable in functions

[Matheus Afonso Martins Moreira]

> If you are talking about the current specific case of path_value, as
> Chet has replied, path_value doesn't return a newly allocated block.
> It just returns a pointer to an existing variable cell owned and
> managed by the global variable context whose root is static. It
> doesn't return or move an ownership of the pointer.

Another reason I wrote it that way because it allowed me
to be productive despite my relatively limited knowledge
of the codebase.

As someone who has just encountered this codebase
and is still not fully familiar with it, I thought it was best
to err on the side of caution. I've put in effort into making
sense of memory managenent abstractions like savestring
but it's still a work in progress.

> If you are talking about just the coding style in general, it seems
> like just your personal style.

It is. However, it doesn't seem to be incompatible with
the style employed in the bash codebase. Everything
is quite well organized and named, it's really good.
My intention was only to improve it even further.

> the patch just encapsulates it in another function h(x) := f(g(x))
> and the logical structure is the same.

Yes, but then you have a nice API which requires less knowledge.
It does not require the programmer to know about f and g.
This kind of thing could be useful for new contributors who are
looking to make improvements despite limited knowledge.
Such as myself.

Another reason why I added the function:
I searched for that function in the course of
developing the patch and didn't find it.
So I added it.

> the choice would depend on many factors
> including the maintenance cost
> and the preference of the maintainer.

Then I would need a _definitive_ decision from Chet Ramey, the maintainer.

I am paying attention to his emails.
For example, this email factored heavily into my choices:

https://lists.gnu.org/archive/html/bug-bash/2024-05/msg00116.html

> it should be implemented using a call to find_in_path()
> with the appropriate path string and flags.

The first thing I did after reading that was look up this find_in_path
function which I had somehow missed. I found that indeed it's the
right solution and agreed with him. However, I also found that it
takes as its string parameter a colon-separated list of directories,
not the name of the PATH-like variable that string is bound to.
I wanted to dereference a specific variable and then pass the
value to this function. I noticed that there was an internal function
which does pretty much exactly what I wanted so I simply created
an external function for it.

I realize that he also said:

> I feel like a feature like BASH_SOURCE_PATH
> should take less than a dozen lines of code
> in source_builtin().

I interpreted that as a preference for shorter code
without implying that other solutions (such as mine) are
inappropriate. In other words, maybe they would feel
differently if the code I submit is good enough.

> The final choice of how the code is maintained
> would be determined by the maintainer

Of course.

> If one observes the Bash codebase, one notices that
> it is not maintained in your style.

That's not my observation at all.

I felt very comfortable working with the bash codebase.
Everything is organized, well named and neatly separated
into logical functions and modules. This is exactly how I
like to organize my own projects.

I felt so at home it emboldened me to try to improve
its general organization. That's why I sent patches
to that effect.

I didn't think it was unreasonable to create the new
find_in_path_var function because there was the
very similar find_in_path function just above it.
They even accomplish the same things:
expose the static functions as a public interface
while maintaining the freedom to change the internals.
They do essentially the same thing, but the new function
takes the name of a bash variable as parameter instead.

Nor did I think it was unreasonable to split code up
into logical chunks by placing them into helper functions.
Those static helpers already exist all over the codebase.
Even in builtins/source.def: uw_maybe_pop_dollar_vars.
So I don't really understand why there are objections to them.

If that's unacceptable, the maintainer just has to say the word.
I'll eliminate all this code in the v3 patch set. I just don't see
why it would be unacceptable though.

  -- Matheus