git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Stefan Beller <sbeller@google.com>
To: gitster@pobox.com, git@jeffhostetler.com
Cc: git@vger.kernel.org, Stefan Beller <sbeller@google.com>
Subject: [PATCH 00/24] Retire NO_THE_INDEX_COMPATIBILITY_MACROS
Date: Tue,  2 May 2017 15:22:58 -0700	[thread overview]
Message-ID: <20170502222322.21055-1-sbeller@google.com> (raw)

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


             reply	other threads:[~2017-05-02 22:23 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-02 22:22 Stefan Beller [this message]
2017-05-02 22:22 ` [PATCH 01/24] cache.h: drop read_cache() Stefan Beller
2017-05-02 22:23 ` [PATCH 02/24] cache.h: drop active_* macros Stefan Beller
2017-05-02 22:23 ` [PATCH 03/24] cache.h: drop read_cache_from Stefan Beller
2017-05-02 22:23 ` [PATCH 04/24] cache.h: drop read_cache_preload(pathspec) Stefan Beller
2017-05-02 22:23 ` [PATCH 05/24] cache.h: drop read_cache_unmerged() Stefan Beller
2017-05-02 22:23 ` [PATCH 06/24] unpack-trees.c: rename parameter 'the_index' Stefan Beller
2017-05-02 22:23 ` [PATCH 07/24] cache.h: drop read_blob_data_from_cache Stefan Beller
2017-05-02 22:23 ` [PATCH 08/24] cache.h: drop unmerge_cache[_entry_at] Stefan Beller
2017-05-02 22:23 ` [PATCH 09/24] cache.h: drop resolve_undo_clear Stefan Beller
2017-05-02 22:23 ` [PATCH 10/24] cache.h: drop cache_name_is_other Stefan Beller
2017-05-02 22:23 ` [PATCH 11/24] cache.h: drop cache_file_exists Stefan Beller
2017-05-02 22:23 ` [PATCH 12/24] cache.h: drop cache_dir_exists Stefan Beller
2017-05-02 22:23 ` [PATCH 13/24] cache.h: drop is_cache_unborn(), discard_cache(), unmerged_cache() Stefan Beller
2017-05-02 22:23 ` [PATCH 14/24] cache.h: drop cache_name_pos Stefan Beller
2017-05-02 22:23 ` [PATCH 15/24] cache.h: drop add_cache_entry Stefan Beller
2017-05-02 22:23 ` [PATCH 16/24] cache.h: drop rename_cache_entry_at Stefan Beller
2017-05-02 22:23 ` [PATCH 17/24] cache.h: drop remove_file_from_cache Stefan Beller
2017-05-02 22:23 ` [PATCH 18/24] cache.h: drop add_to_cache Stefan Beller
2017-05-02 22:23 ` [PATCH 19/24] cache.h: drop add_file_to_cache Stefan Beller
2017-05-02 22:23 ` [PATCH 20/24] cache.h: drop chmod_cache_entry Stefan Beller
2017-05-02 22:23 ` [PATCH 21/24] cache.h: drop refresh_cache Stefan Beller
2017-05-02 22:23 ` [PATCH 22/24] cache.h: drop ce_modified Stefan Beller
2017-05-02 22:23 ` [PATCH 23/24] cache.h: drop ce_match_stat Stefan Beller
2017-05-02 22:23 ` [PATCH 24/24] cache.h: retire NO_THE_INDEX_COMPATIBILITY_MACROS Stefan Beller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170502222322.21055-1-sbeller@google.com \
    --to=sbeller@google.com \
    --cc=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).