| Message ID | cover-0.6-00000000000-20221215T095335Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | remove USE_THE_INDEX_COMPATIBILITY_MACROS | expand |
Hi Ævar On 15/12/2022 09:58, Ævar Arnfjörð Bjarmason wrote: > My recent now-landed topic[1] to remove most use of > "USE_THE_INDEX_COMPATIBILITY_MACROS" was merged in 041df69edd3 (Merge > branch 'ab/fewer-the-index-macros', 2022-11-28). > > It left out use of the macros that would have conflicted with > in-flight changes, but as those topics have landed we can now complete > the migration. > > As before this is almost entirely a matter of applying the existing > "pending" coccinelle rules, the exceptions being 1/6, and the *.h > changes where we remove the macro definitions (the macro users being > edited by coccinelle). > > The 4-5/6 then handle some edge cases we had left (but the code change > itself is done by coccinelle). I've only given these patches a quick scan, but I think they look good. None of the callers that are converted here are in library code so using the_index makes perfect sense. Best Wishes Phillip > 1. https://lore.kernel.org/git/cover-v2-00.11-00000000000-20221119T125550Z-avarab@gmail.com/ > > Ævar Arnfjörð Bjarmason (6): > builtin/rm.c: use narrower "USE_THE_INDEX_VARIABLE" > cocci & cache.h: fully apply "active_nr" part of index-compatibility > cocci & cache.h: apply pending "index_cache_pos" rule > cocci & cache-tree.h: migrate "write_cache_as_tree" to "*_index_*" > cache-tree API: remove redundant update_main_cache_tree() > cocci & cache.h: remove "USE_THE_INDEX_COMPATIBILITY_MACROS" > > builtin/am.c | 6 ++-- > builtin/commit.c | 18 +++++----- > builtin/merge.c | 8 ++--- > builtin/mv.c | 8 +++-- > builtin/rm.c | 2 +- > builtin/stash.c | 11 +++--- > builtin/update-index.c | 4 +-- > builtin/write-tree.c | 5 +-- > cache-tree.h | 15 -------- > cache.h | 12 +------ > contrib/coccinelle/index-compatibility.cocci | 36 ++++++++++++++----- > .../index-compatibility.pending.cocci | 24 ------------- > 12 files changed, 62 insertions(+), 87 deletions(-) > delete mode 100644 contrib/coccinelle/index-compatibility.pending.cocci >
On Mon, Dec 19 2022, Phillip Wood wrote: > Hi Ævar > > On 15/12/2022 09:58, Ævar Arnfjörð Bjarmason wrote: >> My recent now-landed topic[1] to remove most use of >> "USE_THE_INDEX_COMPATIBILITY_MACROS" was merged in 041df69edd3 (Merge >> branch 'ab/fewer-the-index-macros', 2022-11-28). >> It left out use of the macros that would have conflicted with >> in-flight changes, but as those topics have landed we can now complete >> the migration. >> As before this is almost entirely a matter of applying the existing >> "pending" coccinelle rules, the exceptions being 1/6, and the *.h >> changes where we remove the macro definitions (the macro users being >> edited by coccinelle). >> The 4-5/6 then handle some edge cases we had left (but the code >> change >> itself is done by coccinelle). > > I've only given these patches a quick scan, but I think they look > good. None of the callers that are converted here are in library code > so using the_index makes perfect sense. Thanks for the review. That's correct, although even if that were the case that wouldn't be an issue with this migration, as we'd have been using "the_index" before, just indirectly through a macro. That wasn't the case here, but I do I have another similar migration for migrating "the_repository" wrappers. In those cases there's surely instances where e.g. we really should be using a "r" argument instead, but I've opted to leave that question out, as it would make the coccinelle rules involved & diffs much harder to deal with. And because in the end the result is the same if viewed with "cc -E", i.e. these are just the macro shims we've been meaning to stop using for a while.
On 19/12/2022 15:11, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Dec 19 2022, Phillip Wood wrote: > >> Hi Ævar >> >> On 15/12/2022 09:58, Ævar Arnfjörð Bjarmason wrote: >>> My recent now-landed topic[1] to remove most use of >>> "USE_THE_INDEX_COMPATIBILITY_MACROS" was merged in 041df69edd3 (Merge >>> branch 'ab/fewer-the-index-macros', 2022-11-28). >>> It left out use of the macros that would have conflicted with >>> in-flight changes, but as those topics have landed we can now complete >>> the migration. >>> As before this is almost entirely a matter of applying the existing >>> "pending" coccinelle rules, the exceptions being 1/6, and the *.h >>> changes where we remove the macro definitions (the macro users being >>> edited by coccinelle). >>> The 4-5/6 then handle some edge cases we had left (but the code >>> change >>> itself is done by coccinelle). >> >> I've only given these patches a quick scan, but I think they look >> good. None of the callers that are converted here are in library code >> so using the_index makes perfect sense. > > Thanks for the review. > > That's correct, although even if that were the case that wouldn't be an > issue with this migration, as we'd have been using "the_index" before, > just indirectly through a macro. Indeed, I'm just not convinced that it is worth removing the macro in library code without changing to take a struct istate (I don't see the existence of the macro itself as a problem as I think it is just a symptom of the real problem) but I seem to be in the minority on that point. Best Wishes Phillip > That wasn't the case here, but I do I have another similar migration for > migrating "the_repository" wrappers. > > In those cases there's surely instances where e.g. we really should be > using a "r" argument instead, but I've opted to leave that question out, > as it would make the coccinelle rules involved & diffs much harder to > deal with. > > And because in the end the result is the same if viewed with "cc -E", > i.e. these are just the macro shims we've been meaning to stop using for > a while. >
Phillip Wood <phillip.wood123@gmail.com> writes: >> That's correct, although even if that were the case that wouldn't >> be an issue with this migration, as we'd have been using >> "the_index" before, just indirectly through a macro. > > Indeed, I'm just not convinced that it is worth removing the macro in > library code without changing to take a struct istate (I don't see the > existence of the macro itself as a problem as I think it is just a > symptom of the real problem) but I seem to be in the minority on that > point. True. Many subcommands need to deal only with the_index and no other index, so for the implementations of the top-level subcommands that work only in a single repository, the macros are not by themselves problems. The deeper parts of the system that we want to reuse by libifying of course eventually need to learn to take an arbitrary "istate" and NO_THE_INDEX_COMPATIBILITY_MACROS mechanism (and its successor USE_THE_INDEX_COMPATIBILITY_MACROS, probably) was a great approach for that goal.
On Tue, Dec 20 2022, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >>> That's correct, although even if that were the case that wouldn't >>> be an issue with this migration, as we'd have been using >>> "the_index" before, just indirectly through a macro. >> >> Indeed, I'm just not convinced that it is worth removing the macro in >> library code without changing to take a struct istate (I don't see the >> existence of the macro itself as a problem as I think it is just a >> symptom of the real problem) but I seem to be in the minority on that >> point. > > True. > > Many subcommands need to deal only with the_index and no other > index, so for the implementations of the top-level subcommands that > work only in a single repository, the macros are not by themselves > problems. The deeper parts of the system that we want to reuse by > libifying of course eventually need to learn to take an arbitrary > "istate" and NO_THE_INDEX_COMPATIBILITY_MACROS mechanism (and its > successor USE_THE_INDEX_COMPATIBILITY_MACROS, probably) was a great > approach for that goal. I'm not sure what to make of this comment & this series not having been picked up (perhaps I'm reading too much into that), that you'd like to keep USE_THE_INDEX_COMPATIBILITY_MACROS? This side-thread is discussing a theoretical issue. Phillip was saying (if I understand him correctly) that if we were using "the_index" in libraries we should fix that, I was agreeing, but saying that if that were the case this series would still be a good step forward, we could fix those issues later. It looks like we disagreed on the "later" part of that. But in any case, as noted at the start of this thread there are no such library users, so it's a moot point.