Re: [PATCH] compaudit: avoid defining a global getent fallback

["Daniel Shahaf" <d.s@xxxxxxxxxxxxxxxxxx>]

Xichen Zeng wrote on Sat, 31 Oct 2026 03:38 +00:00:
> On systems without an external getent command, compaudit defines a 
> global getent fallback function. This function remains after compaudit 
> returns and overwrites any existing user-defined function of the same name.

Agreed that's a problem: compaudit shouldn't be in the business of
shadowing system commands or clobbering user-defined functions.

Quick review below.

> +++ b/Completion/compaudit
> @@ -15,16 +15,6 @@ compaudit() {                           # Define and then call
>  emulate -L zsh
>  setopt extendedglob
>  
> -[[ -n $commands[getent] ]] || getent() {
> -  if [[ $1 = hosts ]]; then
> -    sed 's/#.*//' /etc/$1 | grep -w $2
> -  elif [[ $2 = <-> ]]; then
> -    grep ":$2:[^:]*$" /etc/$1
> -  else
> -    grep "^$2:" /etc/$1
> -  fi
> -}
> -
>  # The positional parameters are the directories to check, else fpath.
>  if (( $# )); then
>    local _compdir=''
> @@ -132,12 +122,19 @@ _i_wdirs=( ${^fpath}(N-f:g+w:,-f:o+w:,-^${_i_owners})
>  # that this has not happened, and pick the best group.
>  
>  if (( $#_i_wdirs )); then
> -  local GROUP GROUPMEM _i_pw _i_gid
> +  local GROUP GROUPMEM _i_pw _i_gid _i_group
>    if ((UID == EUID )); then
> -    getent group $LOGNAME | IFS=: read GROUP _i_pw _i_gid GROUPMEM
> +    _i_group=$LOGNAME

This introduces an assumption that $LOGNAME is a valid group name (or
number), which is not correct on all systems.  E.g., it's not correct on
www.zsh.org.

>    else
> -    getent group $EGID | IFS=: read GROUP _i_pw _i_gid GROUPMEM
> +    _i_group=$EGID

So, the type of $_i_group is "either group name (string) or group
number".  It'd be nice to avoid this, if possible, but I wouldn't
consider it a merge blocker.

>    fi
> +  if [[ -n $commands[getent] ]]; then
> +    getent group "$_i_group"

Shouldn't this codepath also be taken when a «getent» function is defined?

> +  elif [[ $_i_group = <-> ]]; then
> +    grep ":$_i_group:[^:]*$" /etc/group

If this were new code, I would suggest to:

- Use braces here in case we ever define a «:[» modifier.

- Count fields from the start of the line, rather than from the end.

However, both of these are preexisting issues from the wrapper function
this patch removes.

> +  else
> +    grep "^$_i_group:" /etc/group
> +  fi | IFS=: read GROUP _i_pw _i_gid GROUPMEM
>  
>    if [[ $GROUP == $LOGNAME && ( -z $GROUPMEM || $GROUPMEM == $LOGNAME ) ]]
>    then

Have you considered renaming the local «getent» function to, e.g.,
_compaudit_internal__getent, and then update the callsites to use that?

Cheers,

Daniel