Re: [RFC or so] Add HASH_LOOKUP option

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Aug 23,  8:25pm, Mikael Magnusson wrote:
}
} > I think there may be  problem with this in the event that the "hash"
} > command has been used to deliberately insert an entry into the hash
} > table.  That's a documented mechanism for overriding path search.
} > Also I think you've missed that execute() also uses the hash table;
} > did you intend that "command foo" ignores NO_HASHLOOKUP?
} 
} As I said, I didn't check everywhere yet to see if there are more
} duplicated codepaths, so no, that is not the intent. However, command
} foo for me doesn't appear to ignore it for me.

execute() gets called for all WC_SIMPLE commands that aren't builtins
or functions, including the "command" and "exec" precommand modifiers:

    /* for command -p, search the default path */ 
    if (defpath) {
        ...
    } else {
   
	if ((cn = (Cmdnam) cmdnamtab->getnode(cmdnamtab, arg0))) {
            ...
        }
        ... usual path search ...
    }

} The fact that 'command' supposedly does a hash lookup while 'command
} -p' doesn't seems to be rather undocumented too.

It'd hardly be possible to search the default POSIX $PATH via a hash
lookup, given that the hash table is filled from what is most commonly
a non-default $path.
 
} > It starts (if the command is not already in the hash table and HASHCMDS
} > is set) by doing hashcmd(), which performs a search of only absolute
} > directory names in the path (not, e.g., ".", "..", or other relative
} > names).  That search itself seems a bit off because if *arg0 == '/'
} > then it's going to prepend the path component anyway. [*]
} 
} That's the first fishy thing I noticed, why is this for loop over
} $path copied to at least four places, two of which are in the same
} function, with slight variations?
 
My recollection (which I haven't bothered to augment by attempting to
search the list archives) is that the path is searched first so that
the hash table is populated so spelling corrections can be suggested,
or something like that.  The first such search, though, has to skip
relative directories, lest "." cause NO_PATH_DIRS to be violated.

Then it's necessary to repeat the search for just the directories that
previously were skipped, in case one of them appears earlier in the
path than the directory where the command was previously found.  So
that's two variations.

Then execute() has to search either the default $PATH (third variant)
or the normal $path (fourth variant), but doesn't have to worry about
separating absolute and relative directories; it can just use whichever
it finds first, just like the final fallback attempt in findcmd().
 
} I guess in this specific function I could move my isset(HASHLOOKUP) to
} be in the & HASHED if() instead of the cn one. Which is to say if the
} whole thing isn't broken in the first place.

Except for the part about putting goofy stuff in the hash table in a
few edge cases, I think it's not actually broken, just disorganized.

} > [**] The HASHED bit means the command was deliberately inserted into
} > the hash table with the "hash" builtin, rather than found by search.
} > In this case the both findcmd() and execute() are forced to believe
} > what the hash table tells them.
} 
} Could this be used, theoretically, to still allow users to override
} the path, and still do a full path search for search hashed entries?

I'm not sure what you mean, but on my best guess:  If you poke into
the hash table with "hash foo=/xyz/foo" then the "foo" command will
run /xyz/foo even if ${^path}/foo(N) is non-empty.

However, and this might be considered a bug, changing the value of
$path discards the entire hash table, including entries that have been
explicitly inserted with "hash".

} Another thing I was vaguely wondering about is how is HASH_CMDS forced
} on by CORRECT? I was unable to grep up anything about it.

It isn't actually forced on by CORRECT, but during the actual event of
correcting, spckword() calls cmdnamtab->filltable() which has the same
effect.