Re: [PATCH v6 1/3] ref-filter: add worktreepath atom

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 24, 2019, 18:26 UTC
Message-ID: <xmqq5zud52ut.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAC05384+KjC=4_ZF9BrxweMUjwpkaGXNqRNSnwif6yci6TxMMw@mail.gmail.com>

Nickolai Belakovski <nbelakovski@gmail.com> writes:

Show 5 quoted lines

> Yes, the parser used the atom argument in an earlier version of this
> patch, but we since moved the map out of the atom since it only needs
> to exist once globally. Even though we have a caching mechanism for
> atoms it still seemed like a logical move to explicitly keep one
> instance of the map globally.

I think that is a mistaken move from an earlier version to this one. The worktree-related stuff only becomes necessary and belongs to the %(worktreepath) atom, so unless there is a compelling reason not to, we should hang it there, instead of introducing a global. When you have --format='%(worktreepath) %(worktreepath)', you'd have only one shared instance of it in used_atom[] to ensure that we have a singleton instance.

The object_info support in the file, which is relatively new, may be what you borrowed the wrong idea of preferring globals from; I think it should be taken as an anti-pattern.