git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 00/24] Retire NO_THE_INDEX_COMPATIBILITY_MACROS

From
Stefan Beller <sbeller@google.com>
Date
May 2, 2017, 22:22 UTC
Message-ID
<20170502222322.21055-1-sbeller@google.com>
This is the follow up to [1], but actually completed now.

The reasoning why this series is a good idea is in the commit message of the last patch, quoted here:

The NO_THE_INDEX_COMPATIBILITY_MACROS pre processor setting was introduced in 4aab5b46f4 (Make read-cache.c "the_index" free., 2007-04-01), stating:

    This makes all low-level functions defined in read-cache.c to
    take an explicit index_state structure as their first parameter,
    to specify which index to work on.

The reasoning is very vague, maybe indicating that having the index specified to work on is easier to for the new reader to understand what is going on.

All preceding patches worked on retiring functions that do not take an explicit index as to where to perform its work. Spelling out the reasons why we want to specify the index at each call:

1)  Ease of understanding:
   The recent patches dropped a lot of macros that may confuse new people
   diving into the code base.
2a) Spelling out global state explicitly:
   Anything you do in submodule land today needs to spawn new processes in
   the submodule. This is cumbersome and not performant. So we may want to
   have an abstraction of a repo, i.e. all repository state in one struct.
   That way we can open a submodule in-process and perform the required
   actions without spawning a process. The road to this future is a long
   road, and we have to start somewhere. Exposing the global state seems
   like a good starter point.
2b) Spelling out global state explicitly (II): (Jeff Hostetler wrote)
   In addition to (eventually) allowing multiple repos be open at the same
   time for submodules, it would also help with various multi-threading
   efforts.  For example, we have loops that do a
   "for (k = 0, k < active_nr; k++) {...}"  There is no visual clue in that
   code that it references "the_index" and therefore should be subject to
   the same locking.  Granted, this is a trivial example, but goes to the
   argument that the code has lots of subtle global variables and macros
   that make it difficult to reason about the code.
   This step would help un-hide this.
   

Review as well as critique if this is the right approach to an endgame with less globals splattered throughout the codebase is welcome.

Thanks, Stefan

[1] https://public-inbox.org/git/20170501190719.10669-1-sbeller@google.com/
Stefan Beller (24):
  cache.h: drop read_cache()
  cache.h: drop active_* macros
  cache.h: drop read_cache_from
  cache.h: drop read_cache_preload(pathspec)
  cache.h: drop read_cache_unmerged()
  unpack-trees.c: rename parameter 'the_index'
  cache.h: drop read_blob_data_from_cache
  cache.h: drop unmerge_cache[_entry_at]
  cache.h: drop resolve_undo_clear
  cache.h: drop cache_name_is_other
  cache.h: drop cache_file_exists
  cache.h: drop cache_dir_exists
  cache.h: drop is_cache_unborn(), discard_cache(), unmerged_cache()
  cache.h: drop cache_name_pos
  cache.h: drop add_cache_entry
  cache.h: drop rename_cache_entry_at
  cache.h: drop remove_file_from_cache
  cache.h: drop add_to_cache
  cache.h: drop add_file_to_cache
  cache.h: drop chmod_cache_entry
  cache.h: drop refresh_cache
  cache.h: drop ce_modified
  cache.h: drop ce_match_stat
  cache.h: retire NO_THE_INDEX_COMPATIBILITY_MACROS
 apply.c                              | 32 +++++++------
 attr.c                               |  1 -
 builtin/add.c                        | 12 ++---
 builtin/am.c                         | 26 +++++------
 builtin/blame.c                      | 19 ++++----
 builtin/check-attr.c                 |  2 +-
 builtin/check-ignore.c               |  4 +-
 builtin/checkout-index.c             | 12 ++---
 builtin/checkout.c                   | 66 +++++++++++++-------------
 builtin/clean.c                      |  4 +-
 builtin/commit.c                     | 52 ++++++++++-----------
 builtin/describe.c                   |  2 +-
 builtin/diff-files.c                 |  4 +-
 builtin/diff-index.c                 |  6 +--
 builtin/diff.c                       | 19 ++++----
 builtin/fsck.c                       | 14 +++---
 builtin/grep.c                       | 10 ++--
 builtin/ls-files.c                   | 49 +++++++++----------
 builtin/merge-index.c                | 14 +++---
 builtin/merge.c                      | 24 +++++-----
 builtin/mv.c                         | 22 ++++-----
 builtin/pull.c                       |  4 +-
 builtin/read-tree.c                  |  8 ++--
 builtin/reset.c                      | 11 +++--
 builtin/rev-parse.c                  |  2 +-
 builtin/rm.c                         | 26 +++++------
 builtin/submodule--helper.c          | 10 ++--
 builtin/update-index.c               | 91 +++++++++++++++++++-----------------
 cache.h                              | 35 --------------
 check-racy.c                         | 10 ++--
 convert.c                            |  4 +-
 diff-lib.c                           |  8 ++--
 diff.c                               | 14 +++---
 dir.c                                | 41 ++++++++--------
 entry.c                              |  3 +-
 merge-recursive.c                    | 65 +++++++++++++-------------
 merge.c                              |  8 ++--
 name-hash.c                          |  1 -
 pathspec.c                           | 16 +++----
 read-cache.c                         |  5 +-
 rerere.c                             | 38 +++++++--------
 revision.c                           | 22 ++++-----
 sequencer.c                          | 29 ++++++------
 sha1_name.c                          | 22 ++++-----
 submodule.c                          | 20 ++++----
 t/helper/test-dump-cache-tree.c      |  4 +-
 t/helper/test-dump-untracked-cache.c |  2 +-
 t/helper/test-lazy-init-name-hash.c  | 20 ++++----
 t/helper/test-read-cache.c           |  4 +-
 t/helper/test-scrap-cache-tree.c     |  4 +-
 t/t2107-update-index-basic.sh        |  2 +-
 tree.c                               | 10 ++--
 unpack-trees.c                       |  9 ++--
 wt-status.c                          | 24 +++++-----
 54 files changed, 471 insertions(+), 495 deletions(-)
-- 
2.13.0.rc1.39.ga6db8bfa24
Next: Stefan Beller
Message 1 of 25 in “Retire NO_THE_INDEX_COMPATIBILITY_MACROS”
  1. 00/24 Retire NO_THE_INDEX_COMPATIBILITY_MACROSStefan Beller, May 2, 2017
  2. 01/24 cache.h: drop read_cache()Stefan Beller, May 2, 2017
  3. 03/24 cache.h: drop read_cache_fromStefan Beller, May 2, 2017
  4. 02/24 cache.h: drop active_* macrosStefan Beller, May 2, 2017
  5. 06/24 unpack-trees.c: rename parameter 'the_index'Stefan Beller, May 2, 2017
  6. 04/24 cache.h: drop read_cache_preload(pathspec)Stefan Beller, May 2, 2017
  7. 07/24 cache.h: drop read_blob_data_from_cacheStefan Beller, May 2, 2017
  8. 05/24 cache.h: drop read_cache_unmerged()Stefan Beller, May 2, 2017
  9. 10/24 cache.h: drop cache_name_is_otherStefan Beller, May 2, 2017
  10. 12/24 cache.h: drop cache_dir_existsStefan Beller, May 2, 2017
  11. 11/24 cache.h: drop cache_file_existsStefan Beller, May 2, 2017
  12. 13/24 cache.h: drop is_cache_unborn(), discard_cache(), unmerged_cache()Stefan Beller, May 2, 2017
  13. 09/24 cache.h: drop resolve_undo_clearStefan Beller, May 2, 2017
  14. 08/24 cache.h: drop unmerge_cache[_entry_at]Stefan Beller, May 2, 2017
  15. 14/24 cache.h: drop cache_name_posStefan Beller, May 2, 2017
  16. 22/24 cache.h: drop ce_modifiedStefan Beller, May 2, 2017
  17. 20/24 cache.h: drop chmod_cache_entryStefan Beller, May 2, 2017
  18. 23/24 cache.h: drop ce_match_statStefan Beller, May 2, 2017
  19. 24/24 cache.h: retire NO_THE_INDEX_COMPATIBILITY_MACROSStefan Beller, May 2, 2017
  20. 21/24 cache.h: drop refresh_cacheStefan Beller, May 2, 2017
  21. 19/24 cache.h: drop add_file_to_cacheStefan Beller, May 2, 2017
  22. 18/24 cache.h: drop add_to_cacheStefan Beller, May 2, 2017
  23. 17/24 cache.h: drop remove_file_from_cacheStefan Beller, May 2, 2017
  24. 16/24 cache.h: drop rename_cache_entry_atStefan Beller, May 2, 2017
  25. 15/24 cache.h: drop add_cache_entryStefan Beller, May 2, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.