Re: [PATCH cygport] Add customization support for announce command

[Adam Dinwoodie <adam@dinwoodie.org>]

On Tue, Apr 30, 2024 at 12:27:35PM +0200, Christian Franke via Cygwin-apps wrote:
> Jon Turney wrote:
> > On 10/03/2024 16:33, Christian Franke via Cygwin-apps wrote:
> > > +    /bin/bash -c "cd ${top} || exit 1
> > > +${HOMEPAGE+HOMEPAGE=${HOMEPAGE@Q}}
> > > +P=${P@Q}; PF=${PF@Q}; PN=${PN@Q}; PR=${PR@Q}; PV=(${PV[*]@Q})
> > > +${SMTP_SENDER+SMTP_SENDER=${SMTP_SENDER@Q}}
> > > +${SMTP_SERVER+SMTP_SERVER=${SMTP_SERVER@Q}}
> > > +${SMTP_SERVER_PORT+SMTP_SERVER_PORT=${SMTP_SERVER_PORT@Q}}
> > > +${SMTP_ENCRYPTION+SMTP_ENCRYPTION=${SMTP_ENCRYPTION@Q}}
> > > +${SMTP_USER+SMTP_USER=${SMTP_USER@Q}}
> > > +${SMTP_PASS+SMTP_PASS=${SMTP_PASS@Q}}
> > > +${cmd}
> > > +"         $0 ${msg} || error "Command '\${${cmdvar}} ${msg}'
> > > (cwd=${top}) failed"
> > > +}
> > 
> > Sorry I didn't notice this before, and I am terrible at writing shell,
> > but perhaps you could share the reasoning behind writing this as above,
> > and not as, e.g.
> > 
> > (cd ${top} && env BLAH ${cmd})
> > 
> > avoiding all the verbiage in the description of ANNOUNCE_EDITOR about it
> > being fed into 'bash -c' (and hence getting evaluated twice??) rather
> > than just run?
> > 
> > 
> 
> None of the mentioned variables are exported to the environment by cygport.
> I wanted to keep this fact in the subshell. Therefore the assignments are
> added to the script instead of passing via env(ironment). The latter won't
> even work with the PV variable because arrays could not be exported.
> 
> Variables would not be evaluated twice. For example in the rare case that
> someone uses something like
> 
> SMTP_SERVER="smtp.$(hostname -d)"
> 
> in cygport.conf, this would immediately expand to
> SMTP_SERVER="smtp.some.domain". The above
> 
> ${SMTP_SERVER+SMTP_SERVER=${SMTP_SERVER@Q}}
> 
> would expand to
> 
> SMTP_SERVER=${SMTP_SERVER@Q}
> 
> and then to
> 
> SMTP_SERVER='smtp.some.domain'
> 
> (The @Q bash extension ensures proper quoting).

Using a subshell created by ( ... ) would achieve the behaviour you're
after, without requiring nearly so much quote handling.  The code Jon
has pulled out could be rewritten as below; using ( ... ) would mean
that everything happens in a subshell and the exports don't affect the
rest of the environment.

```
(
	cd "$top"
	export HOMEPAGE P PF PN PR PV SMTP_SENDER SMTP_SERVER SMTP_SERVER_PORT SMTP_ENCRYPTION SMTP_USER SMTP_PASS
	"$cmd" "$msg" || error "Command $cmd $msg (cwd=${top}) failed"
)
```

I've removed the array handling for $PV, as it's not an array; possibly
you've confused it with $PVP?  In any case, there is no way to pass an
array to "$cmd" unless "$cmd" is itself a Bash function, as there's no
standard way to store anything other than strings in environment
variables.

I've also removed the `|| return 1` part, since cygport runs with `set
-e` enabled so you only want to catch non-zero return codes if you want
specific error handling.

Finally, I've also added "$msg" to the arguments to "$cmd"; that seems
to be missing and seems to be critical to this working at all!

Alternatively, if you really wanted to avoid the export statement, the
below will achieve the same thing; the only use of the subshell at this
point is to avoid the `cd` changing the working directory for the rest
of the script.

```
(
	cd "$top"
	HOMEPAGE="$HOMEPAGE" P="$P" PF="$PF" PN="$PN" PR="$PR" PV="$PV" SMTP_SENDER="$SMTP_SENDER" SMTP_SERVER="$SMTP_SERVER" SMTP_SERVER_PORT="$SMTP_SERVER_PORT" SMTP_ENCRYPTION="$SMTP_ENCRYPTION" SMTP_USER="$SMTP_USER" SMTP_PASS="$SMTP_PASS" "$cmd" "$msg" || error "Command $cmd $msg (cwd=${top}) failed"
)
```

Jon suggested using `env`, although I don't think it provides any
function here that isn't already provided by built-in Bash function.

Personally, I'd rewrite the whole __pkg_announce_run_cmd_on_msg function
as below, although some of this is just my own stylistic preferences (in
particular, I try to avoid `eval` if at all possible, and here `local -n`
works just fine):

```
__pkg_announce_run_cmd_on_msg() {
	local cmdvar="$1"
	local -n cmd="$1"
	local msg="$2"

	if [[ ! "$cmd" ]]; then
		inform "$cmdvar is empty"
		return 0
	fi

	echo
	inform "Launching '$cmd $msg'"

	(
		cd "$top"
		export HOMEPAGE P PF PN PR PV SMTP_SENDER SMTP_SERVER \
			SMTP_SERVER_PORT SMTP_ENCRYPTION SMTP_USER \
			SMTP_PASS
		"$cmd" "$msg" || error "Command '$cmd $msg' (cwd=${top}) failed"
	)
}
```