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

From: Ben Peart <peartben@gmail.com>
Date: Apr 18, 2017, 16:38 UTC
Message-ID: <48448c2c-378d-0d87-2f99-32095326f323@gmail.com>
In-Reply-To: <xmqqmvbfk8va.fsf@gitster.mtv.corp.google.com>

On 4/16/2017 11:31 PM, Junio C Hamano wrote:

Show 13 quoted lines

> 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.