On 4/16/2017 11:31 PM, Junio C Hamano wrote:
Lars Schneider <larsxschneider@xxxxxxxxx> 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.