[systemd-devel] [PATCH 1/2] systemd-helpers

[Dave Reisner]

On Sat, Jan 10, 2015 at 01:33:05PM +0100, Carlos Morata Castillo wrote:
> Hi,
> 
> As stated here, we should use a library for bash autocompletions (maybe even with include guards).
> http://cgit.freedesktop.org/systemd/systemd/commit/shell-completion/bash/localectl?id=a72d698d0d9ff9c158155b44cdc77376df31a317
> 
> Explanation:
> 
> Using autotools make the autocompletions to /usr/share/bash-completions/completions (as the exit from pkg-config --variable=completionsdir
> bash-completion).
> We ended up having multiple function definitions and even messing around with the global bash function namespace, as the functions are called
> __get_something being possible to redefine other binary bash function. 

Why the exception for busctl and systemctl?

You aren't actually solving any problems with redefinition in this
patch. You're actually making the problem worse. Instead of redefining
*some* common functions on completion load, you're forcing *all* common
functions in systemd-helpers to be redefined (even if only 1 is used). I
don't really see this as a problem, though, and you don't explain why
you seem to think it is.

How is sourcing this library supposed to work? Each file contains:

  #Common functions
  . ./systemd-helpers

But that file won't be in $PWD when completions are loaded. Sourcing
works like any other command -- relative paths will be relative to the
current process's $PWD, not relative to the disk file calling source.
Without cd'ing into /usr/share/bash-completions/completions, this
doesn't work (and please don't cd from completions).

I'm not against deduplicating this common code, but you'd be better off
using m4_include to preprocess the completions at build time.

Cheers,
dR