Re: [RFC PATCH 1/1] completion: load completion file for external subcommand

From: Florian Gamböck <mail@floga.de>
Date: Apr 9, 2018, 13:36 UTC
Message-ID: <20180409133627.GA25078@arbeitsviech>
In-Reply-To: <xmqqlgdw67rc.fsf@gitster-ct.c.googlers.com>

On 2018-04-09 18:36, Junio C Hamano wrote:

Show 8 quoted lines

> Florian Gamböck <mail@floga.de> writes:
> 
> > Good point. I could go even further and ditch the if-construct:
> >
> >    ! declare -f $completion_func && declare -f __load_completion &&
> >        __load_completion "git-$command"
> 
> I personally find that a lot harder to read than if/then/fi.

Then I mis-understood you the first time. It sounded a bit as if you wanted to avoid if-fi. After all, the rest of this code block also uses quite long &&-chains.

Then we are back at the first question:

Show 14 quoted lines

> >  	local completion_func="_git_${command//-/_}"
> > +	if ! declare -f $completion_func >/dev/null 2>/dev/null; then
> > +		declare -f __load_completion >/dev/null 2>/dev/null &&
> > +			__load_completion "git-$command"
> 
> wouldn't the above be easier to read if it were
> 
> 	if ! declare ... $completion_func ... && declare -f __load_completion
> 	then
> 		__load_completion "git-$command"
> 	fi
> 
> or is there a reason why it is better to &&-chain the check for
> __load_completion with its use?

As for readability, I would prefer my first approach then with the following reasoning: Checking the existence of a function and actually calling it can be seen as a unit. Either the function does not exist or you call it. You could even create a function like "call_if_exists", that does exactly this. Either way, at the end of the line you are smarter than before. As for the if-statement, this describes the reason why you even want to load an external file. If the function in question ($completion_func) already exists, we do not want to load it again. If you chain the statement with the existence check of __load_completion, you make it look like those two functions are somehow related, which is not the case.

To put it in words: "If $completion_func does not already exist, then load another file (if you know how to do that)." versus "If $completion_func does not yet exist and you know how to load another file, then load another file." The difference is subtle, but I think the first sentence better describes the intention.

Does my reasoning make sense? I mean, the result will be exactly the same, we are clearly only talking about readability here.