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

[Junio C Hamano <gitster@pobox.com>]

Ben Peart <peartben@gmail.com> writes:

> On 4/16/2017 11:31 PM, Junio C Hamano wrote:
>> Lars Schneider <larsxschneider@gmail.com> writes:
>>
>>> 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.
>
> ...  Since it was already a global, I didn't feel
> like this made it any worse.

The code before your series can easily lose the globals with the
attached patch, _exactly_ because it is prepared to be reusable by a
new caller that supplies its own hashmap by passing it through the
callchain.  The child-process machinery Lars made for his conversion
thing, which you are trying to split out to make it reusable, can be
used by somebody other than apply_multi_file_filter() if you do not
lose the hashmap; what the new caller needs to do is to supply its
own hashmap so that they do not interact with the set of processes
used by Lars's conversion machinery.

If we want to lose the global _after_ applying this patch 4/8, don't
we have to essentially _undo_ 4/8?  How can it not be seen as making
it worse?


 convert.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/convert.c b/convert.c
index 8d652bf27c..ff831e85b8 100644
--- a/convert.c
+++ b/convert.c
@@ -503,9 +503,6 @@ struct cmd2process {
 	struct child_process process;
 };
 
-static int cmd_process_map_initialized;
-static struct hashmap cmd_process_map;
-
 static int cmd2process_cmp(const struct cmd2process *e1,
 			   const struct cmd2process *e2,
 			   const void *unused)
@@ -682,6 +679,9 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 	struct strbuf filter_status = STRBUF_INIT;
 	const char *filter_type;
 
+static int cmd_process_map_initialized;
+static struct hashmap cmd_process_map;
+
 	if (!cmd_process_map_initialized) {
 		cmd_process_map_initialized = 1;
 		hashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);