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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 19, 2017, 01:23 UTC
Message-ID: <xmqqa87ddwbe.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <48448c2c-378d-0d87-2f99-32095326f323@gmail.com>

Ben Peart <peartben@gmail.com> writes:

Show 14 quoted lines

> 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);