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

[Ben Peart <peartben@gmail.com>]

On 4/18/2017 9:23 PM, Junio C Hamano wrote:
> 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?

That's fine.  I'll flip it back in the next spin and enable multiple 
pools of sub-processes.  We'll still need 4/8 but it will be a smaller 
change.

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