Re: [PATCH v5 4/8] convert: Separate generic structures and variables from the filter specific ones

[Ben Peart <peartben@gmail.com>]

On 4/16/2017 11:31 PM, Junio C Hamano wrote:
> Lars Schneider <larsxschneider@gmail.com> writes:
>
>>> -static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd)
>>> +static struct subprocess_entry *find_multi_file_filter_entry(const char *cmd)
>> I am curious why you removed the hashmap parameter (here and in other pars of this patch).
>> I know the parameter is not strictly necessary as the hashmap is a global variable anyways.
>> However, I think it eases code maintainability in the long run if a function is "as pure as
>> possible" (IOW does rely on global state as less as possible).
> If the original relied on a global hashmap and this update kept the
> code like so, I wouldn't mind the end result of this series
> (i.e. rely on it being global).  But that is not the case.  It is
> making the code worse by stopping passing the hashmap through the
> callchain.
I debated this myself but there were a couple of things that pushed me 
to this simpler model.  First, it simplified the interface a little as 
you don't need an explicit init call with state to detect if it has 
already been initialized and you don't have to pass the hashmap into the 
various functions.  Since it was already a global, I didn't feel like 
this made it any worse.

Second, since the hashmap key is the exact command that was executed if 
you ever had two hashmaps with the same key, you'd end up with two 
copies of the same background process running.  I couldn't come up with 
any scenario where you would want two copies of the exact same command 
running at the same time so again went with the simpler model.

While I was typing this, I realized that since the background process is 
a versioned interface, if there were two different parts of the code 
using background processes and if they both wanted to run the same 
background process and if they wanted to use different versions of the 
interface, then you could want two different copies.

That said, I tend to build for things that are needed now rather than 
those that might be needed in the future.  If I've missed a scenario, 
I'm happy to change the interface.