On Mon, Oct 23, 2023 at 01:32:08AM +0000, Albretch Mueller wrote:
> On 10/22/23, Andy Smith <a...@strugglers.net> wrote:
> > Most of the points Greg makes to you are matters of correctness, not
> > matters of taste.
>
> that you called "matters of correctness" may be "visual things" to
> other people. this is something I see on a daily basis I don't try to
> "correct" my students ways, but help them learn, see things for
> themselves.
Was I not sufficiently clear in my explanations? Sorry.
Your original post contained this line of code:
cat "${IFL}" | grep "${DESC_DIR}" | tail -c+$_DESC_DIR
In the interests of learning, let's analyze this line *in full*. From
left to right:
1) Useless use of cat. In this particular example, the result is still
correct. The code is simply longer and slower than it needs to be.
2) All-caps variable name IFL. All-caps variable names are reserved, by
convention, for environment variables (e.g. PATH) and special shell
variables (e.g. IFS). I assume your variable IFL stands for "input
file". But note how close it is to IFS. I can easily imagine that
at some point in the future, if you continue using all-caps variable
names inappropriately, you might accidentally use one that collides
with an environment variable or a special shell variable.
Accidentally overwriting IFS is going to break your scripts faster
than you can imagine.
3) grep "${DESC_DIR}". Another all-caps variable name, but I won't repeat
that point. $DESC_DIR is being passed to a command that expects a
regular expression, but in your example, it's a literal string.
Therefore you should use grep -F. Or use a regular expression in
DESC_DIR, but that wouldn't be my preferred choice.
4) The tail command is not correct here. This is the primary error.
The tail needs to be replaced to make the script work. Tail operates
on whole files, not on each line of input, regardless of what options
you use.
5) You used the _DESC_DIR variable and the DESC_DIR variable together in
the same command. I can only imagine one of them was a typo for the
other.
6) The _DESC_DIR variable expansion is not quoted. Assuming that it was
meant to be DESC_DIR which contains a string with spaces and glob
characters in it, it *must* be quoted, or else it gets passed as an
unguessable number of separate arguments.
Those are the actual issues I spotted. I omitted a few in my previous
post, because I wasn't trying to be fully pedantic. I am now.
Beyond that, I pointed out (by inference) that the code you wrote was
too *limited*. If the errors are corrected, it will print the output
that you asked for. But it can't be extended easily to do anything *more*
than that.
Assuming that you actually wanted to *do* something with each line's
suffix, it makes more sense to include some sort of loop, in which you
can add arbitrary commands to do whatever task is needed.
That's why I replaced the tail with a "while read" loop instead of a
single self-contained command, such as sed, which could have done the
printing task more concisely.
Finally, one might wonder why I used this form:
while IFS= read -r line; do
....
done < <(grep ...)
instead of the simpler form:
grep ... |
while IFS= read -r line; do
...
done
This was another conscious choice to offer more flexibility for extending
the script. In the simpler form, the while loop is executed in a subshell
(because of the pipeline). When the loop terminates, any changes made in
the subshell are discarded. This makes it impossible to set persistent
variables inside the loop, and then use them afterward.
The form that I used, with the process substition <(...), runs the loop
in the main shell process, not in a subshell. With this form, it's
possible to set variables within the loop, and refer to them afterward.
If you have additional questions, feel free to ask them.
