From: Ben Peart Date: Thu, 20 Apr 2017 17:24:45 GMT Subject: Re: [PATCH v5 4/8] convert: Separate generic structures and variables from the filter specific ones Message-ID: In-Reply-To: On 4/18/2017 9:23 PM, Junio C Hamano wrote: > Ben Peart writes: > >> On 4/16/2017 11:31 PM, Junio C Hamano wrote: >>> Lars Schneider 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); >