Re: [PATCH 00/12] Remove more index compatibility macros

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 6, 2021, 03:55 UTC
Message-ID: <xmqqczyiwwgl.fsf@gitster.c.googlers.com>
In-Reply-To: <pull.830.git.1609506428.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> My strategy for update-index was to create static globals "repo" and
> "istate" that point to the_repository and the_index, respectively. Then, I
> was able to remove macros one-by-one without changing method prototypes
> within the file.

Knee-jerk reaction: swapping one pair of global with another? Would that give us enough upside? It may allow some codepaths involved to work on an in-core index instance that is different from the_index, but does not make them reentrant.

Do we now have callers that actually pass an in-core index instance that is different from the_index, and more importantly, that fail loudly if the codepaths involved in this conversion forgets to update some accesses to the_index not to the specified one?

If not, ...

> In total, this allows us to remove four of the compatibility macros because
> they are no longer used.

... a conversion like this, removing the use of the compatibility macros for the sake of removing them, invites future headaches by leaving untested code churn behind with potential bugs that will only get discovered after somebody actually starts making calls with the non-default in-core index instances.

I've come to know the competence of you well enough to trust your patches like patches from other proficient, prolific and prominent contributors (I won't name names, but you know who you are), but we are all human and are prone to introduce bugs.

That's all my knee-jerk impression before actually reading the series through, though. I'll certainloy know more after reading them.

Thanks.

Show 27 quoted lines

> Derrick Stolee (12):
>   merge-index: drop index compatibility macros
>   mv: remove index compatibility macros
>   rm: remove compatilibity macros
>   update-index: drop the_index, the_repository
>   update-index: use istate->cache over active_cache
>   update-index: use index->cache_nr over active_nr
>   update-index: use istate->cache_changed
>   update-index: use index_name_pos() over cache_name_pos()
>   update-index: use remove_file_from_index()
>   update-index: use add_index_entry()
>   update-index: replace several compatibility macros
>   update-index: remove ce_match_stat(), all macros
>
>  Documentation/technical/racy-git.txt |   6 +-
>  builtin/merge-index.c                |  33 +++---
>  builtin/mv.c                         |  42 ++++----
>  builtin/rm.c                         |  56 ++++++-----
>  builtin/update-index.c               | 145 ++++++++++++++-------------
>  cache.h                              |   4 -
>  6 files changed, 149 insertions(+), 137 deletions(-)
>
>
> base-commit: 71ca53e8125e36efbda17293c50027d31681a41f
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-830%2Fderrickstolee%2Findex-compatibility-1-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-830/derrickstolee/index-compatibility-1-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/830