Re: [PATCH 0/5] Start of a journey: drop NO_THE_INDEX_COMPATIBILITY_MACROS

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 4, 2017, 19:19 UTC
Message-ID: <20170504191927.GB15203@aiede.svl.corp.google.com>
In-Reply-To: <xmqqa86tbamh.fsf@gitster.mtv.corp.google.com>

Hi,

Junio C Hamano wrote:

> Duy Nguyen <pclouds@gmail.com> writes:

Show 17 quoted lines

>> I attempted the same thing once or twice in the past, had the same
>> impression and dropped it. But I think it's good to get rid of cache_*
>> macros, at least outside builtin/ directory.
>
> One thing that needs to be kept in mind is that a mechanical
> replacement of active_cache[] to the_index.cache[] in the
> infrastructure code does not get us closer to such an improvement.
> In fact, such a patch only has negative value (keep reading until
> the end of the paragraph that begins with "One thing is certain" to
> understand why).
>
> The entry point to a codechain for an important infrastructure code
> that can currently only work with the_index needs to be identified.
> There may be many.  They need to be made to take an index_state
> instance and to pass it through the callchain.  That kind of change
> would be an improvement, and will get us closer to mark the
> resulting code with NO_THE_INDEX_COMP thing.

Thanks for articulating this.  I agree with this general goal.

[...]

Show 8 quoted lines

> But one thing is certain. Many users of the API, just like a lot of
> builtin/ code works only with the_index today, will not have to work
> with a non-default repository or a non-default index_state.  Only
> the more advanced and complex "multi-repo" Porcelains would have to
> care.  Keeping active_cache[], active_nr, etc. abstraction would
> isolate the simpler callers from the inevitable code churn we would
> need while getting the "repository" abstraction right.  This is why
> I see that you understood the issues well when you said "builtiln/".

One small change we could make is to reverse the sense of the NO_THE_INDEX_COMPATIBILITY_MACROS macro. That way, new library code authors have to define USE_THE_INDEX_COMPATIBILITY_MACROS at the top of the file, providing a hint to reviewers that they are using the_index implicitly instead of relying on explicit repository or index parameters.

More generally, if we are willing to follow through then I see Stefan's change as a step in the right direction. Sure, it replaces calls like read_cache with read_the_index(&the_index, ...) which does not change semantics and may not look like progress. But:

- uses of 'the_index' are easy to grep for and it is easy to
  understand that they are using global state.  In builtins, this is
  not important (which may be an argument for making builtins get
  USE_THE_INDEX_COMPATIBILITY_MACROS implicitly instead and
  restricting this change to library code), but in libraries it
  communicates what is happening to developers looking at the code.
  It is like a /* NEEDSWORK */ comment, except in code instead of
  comments.

  See Go's context.TODO <https://golang.org/pkg/context/#TODO> for a
  similar example in another set of projects.

- it makes code consistently use the term "index" instead of mixing
  "index" and "cache".  This makes code easier to understand for new
  contributors.

- a minor thing: it means more of git is using functions instead of
  macros.  IDEs, grep habits, etc cope better with functions.  That is
  minor, though: the compatibility macros could easily be replaced
  with compatibility inline functions to achieve the same effect.

[...]

> Also, dropping compatibility macros at the end of the series is
> unfriendly to fellow developers with WIPs that depend on the
> traditional way of doing things.

Agreed with this as well --- for widely used APIs like these, it is more friendly to decouple adapting callers (in a separate patch) from the patch that removes the API.

That is, one way to do what this series attempts would be the following:

 1. rename variables that shadow the_index.

 2. add coccinelle patches (or one coccinelle patch) to
    contrib/coccinelle implementing *_cache -> *_index migration.
    Is there a way to do this without making it fail "make coccicheck"?

 3. migrate library code (but not builtins) using that semantic patch

 4. make compatibility macros opt-in instead of opt-out
    (USE_THE_INDEX_COMPATIBILITY_MACROS). Opt-in all builtins.

 5. optional: change the compatibility macros to compatibility inline
    functions, perhaps.
 6. optional: rename *_cache to *_the_index for clarity, perhaps.
 7. optional: migrate builtins, if there is a way to make the patches
    for that look sensible.

Thoughts? Jonathan