| Message ID | cover-00.12-00000000000-20221118T112205Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | tree-wide: chip away at USE_THE_INDEX_COMPATIBILITY_MACROS | expand |
Hi Ævar On 18/11/2022 11:30, Ævar Arnfjörð Bjarmason wrote: > As 3/12 here notes we've been undergoing a sloooow migration away from > functions that provide "the_index" for you since 2007, with the last > major move away from some of them being in 2019. The commit cited in 3/12 actually introduced most of these macros. What it did was remove the dependency on `the_index` from cache.c. Maybe I'm missing some context but I struggling to see the benefit in removing the macros with out removing the dependency on `the_index`. Doing that would require much more manual work as it cannot be automated with coccinelle but it would provide a tangible benefit which I don't see here. Best Wishes Phillip
On Fri, Nov 18 2022, Phillip Wood wrote: > Hi Ævar > > On 18/11/2022 11:30, Ævar Arnfjörð Bjarmason wrote: >> As 3/12 here notes we've been undergoing a sloooow migration away from >> functions that provide "the_index" for you since 2007, with the last >> major move away from some of them being in 2019. > > The commit cited in 3/12 actually introduced most of these > macros. What it did was remove the dependency on `the_index` from > cache.c. Maybe I'm missing some context but I struggling to see the > benefit in removing the macros with out removing the dependency on > `the_index`. Doing that would require much more manual work as it > cannot be automated with coccinelle but it would provide a tangible > benefit which I don't see here. I didn't cover this myself as I figured the linked-to commits & context covered it: But no, there is no great benefit to doing this, it's just janitorial work. My proximate motivation for this is having written this to libify some code in a parallel series: [1]. Does that in itself justify this? No, far from it. But we've had this supposed online migration for years, I tihnk we shouldn't rush to do those sorts of things needlessly (e.g. let's avoid conflicts with other in-flight stuff, if it can be helped), but leaving it for another 4-10 years would also suck. 1. https://lore.kernel.org/git/patch-v9-09.12-f29343197eb-20221118T110058Z-avarab@gmail.com/
As 3/12 here notes we've been undergoing a sloooow migration away from functions that provide "the_index" for you since 2007, with the last major move away from some of them being in 2019. This series has a large diffstat, but should be disproportionately easy to review as I wrote almost none of the code: coccinelle did :) I.e. after some trivial cleanup in 1-2/12 the C code changes in 3/12 are mostly generated from a new index-compatibility.cocci migration. The commits that deviate from that are: * 04/12: Make a function that always returns "0" return "void" instead. * 09-10/12: Add a "USE_THE_INDEX_VARIABLE", for those leftover users that only need "the_index", but not the compatility macros. * 12/12: Manual follow-up to the cocci-applied 11/12. The only exceptions to this are the changes to cache.h itself, and moving things around in contrib/coccinelle/* as we can fully migrate some things, so they move from a "pending" rule to a regular coccinelle rule. At the start of this the patch to migrate all of this would have been ~2k lines, after it's down to below ~1k. I've carefully crafted this so that it doesn't have conflicts with anything that's in-flight, or is likely to develop such conflicts (but I could always get unlucky). If there are any the resolution should be trivially validated by running the cocci rule on "theirs" and keeping that version. Branch & passing CI at: https://github.com/avar/git/tree/avar/narrow-down-the-index-compatibilty-macros Ævar Arnfjörð Bjarmason (12): cache.h: remove unused "the_index" compat macros builtin/{grep,log}.: don't define "USE_THE_INDEX_COMPATIBILITY_MACROS" cocci & cache.h: remove rarely used "the_index" compat macros read-cache API & users: make discard_index() return void cocci: add a index-compatibility.pending.cocci cocci & cache.h: apply a selection of "pending" index-compatibility cocci & cache.h: apply variable section of "pending" index-compatibility cocci: apply "pending" index-compatibility to "t/helper/*.c" {builtin/*,repository}.c: add & use "USE_THE_INDEX_VARIABLE" cache.h & test-tool.h: add & use "USE_THE_INDEX_VARIABLE" cocci: apply "pending" index-compatibility to some "builtin/*.c" builtin/*: remove or amend "USE_THE_INDEX_COMPATIBILITY_MACROS" add-interactive.c | 8 +- add-patch.c | 3 +- builtin/add.c | 21 ++--- builtin/am.c | 4 +- builtin/cat-file.c | 2 +- builtin/check-attr.c | 4 +- builtin/check-ignore.c | 4 +- builtin/checkout-index.c | 19 ++-- builtin/checkout.c | 55 ++++++------ builtin/clean.c | 6 +- builtin/clone.c | 4 +- builtin/commit.c | 20 ++--- builtin/describe.c | 7 +- builtin/diff-files.c | 3 +- builtin/diff-index.c | 5 +- builtin/diff-tree.c | 4 +- builtin/difftool.c | 2 +- builtin/fsck.c | 16 ++-- builtin/grep.c | 1 - builtin/log.c | 1 - builtin/merge-index.c | 16 ++-- builtin/merge-ours.c | 3 +- builtin/merge-tree.c | 2 +- builtin/merge.c | 13 +-- builtin/mv.c | 25 +++--- builtin/pull.c | 6 +- builtin/read-tree.c | 6 +- builtin/rebase.c | 6 +- builtin/reset.c | 20 +++-- builtin/rev-parse.c | 4 +- builtin/rm.c | 18 ++-- builtin/stash.c | 8 +- builtin/submodule--helper.c | 14 +-- builtin/update-index.c | 72 +++++++-------- cache.h | 30 ++----- contrib/coccinelle/index-compatibility.cocci | 87 +++++++++++++++++++ .../index-compatibility.pending.cocci | 78 +++++++++++++++++ read-cache.c | 4 +- repository.c | 2 +- sequencer.c | 15 ++-- t/helper/test-dump-cache-tree.c | 5 +- t/helper/test-dump-split-index.c | 1 + t/helper/test-dump-untracked-cache.c | 4 +- t/helper/test-fast-rebase.c | 4 +- t/helper/test-lazy-init-name-hash.c | 21 ++--- t/helper/test-read-cache.c | 5 +- t/helper/test-scrap-cache-tree.c | 9 +- t/helper/test-write-cache.c | 6 +- 48 files changed, 417 insertions(+), 256 deletions(-) create mode 100644 contrib/coccinelle/index-compatibility.cocci create mode 100644 contrib/coccinelle/index-compatibility.pending.cocci