Re: [RFC PATCH 00/19] Bring more repository handles into our code base

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 12, 2018, 18:50 UTC
Message-ID: <20181012185037.GA52080@aiede.svl.corp.google.com>
In-Reply-To: <20181011211754.31369-1-sbeller@google.com>

Hi,

Stefan Beller wrote:

Show 6 quoted lines

> This applies on nd/the-index (b3c7eef9b05) and is the logical continuation of
> the object store series, which I sent over the last year.
>
> The previous series did take a very slow and pedantic approach,
> using a #define trick, see cfc62fc98c for details, but it turns out,
> that it doesn't work:

Thanks for the heads up --- this will remind me to review this new series more carefully, since it differs from what was reviewed before.

I think this will be easiest to review with --function-context. I can generate that diff locally, so no need to resend.

Show 7 quoted lines

>    When changing the signature of widely used functions, it burdens the
>    maintainer in resolving the semantic conflicts.
>
>    In the orginal approach this was called a feature, as then we can ensure
>    that not bugs creep into the code base during the merge window (while such
>    a refactoring series wanders from pu to master). It turns out this
>    was not well received and was just burdensome.

I don't agree with this characterization.

The question of who resolves conflicts is separate from the question of whether conflicts appear, which is in turn separate from the question of whether the build breaks.

I consider making the build break when a caller tries to use a half-converted function too early to be a very useful feature. There is a way to do that in C++ that allows decoupled conversions, but the C version forced an ordering of the conversions. It seems that the pain was caused by the combination of

 1. that coupling, which forced an ordering on the conversions and
    prevented us from ordering the patches in an order based on
    convenience of integration (unlike e.g. the "struct object_id"
    series which was able to proceed by taking a batch covering a
    quiet area of the tree at a time)

 2. as you mentioned, removal of old API at the same time of addition
 of new API forced callers across the tree to update at once

 3. the lack of having decided how to handle the anticipated churn

Now most of the conversions are done (thanks much for that) so the ordering (1) is not the main remaining pain point. Thanks for tackling the other two in this series.

I want future API changes to be easier. That means tackling the following questions up front:

 i. Where does this fit in Rusty's API rating scheme
    <http://sweng.the-davies.net/Home/rustys-api-design-manifesto>?
    Does misuse (or misconverted callers) break the build, break
    visibly at runtime, or are the effects more subtle?

 ii. Is there good test coverage for the new API?  Are there tests
     that need to be migrated?

 iii. Is there a way to automatically migrate callers, or does this
      require manual, error-prone work (thanks for tackling that in
      this one.)

 iv. How are we planning to handle multiple patches in flight?  Will
     the change produce merge conflicts?  How can others on the list
     help the maintainer with integrating this set of changes?

 iv. Is the ending point cleaner than where we started?

The #define trick you're referring to was a way of addressing (i).

[...]

>  79 files changed, 571 insertions(+), 278 deletions(-)

Most of the increase is in the coccinelle file and in improved documentation.

It appears that some patches use a the_index-style NO_THE_REPOSITORY_COMPATIBILITY_MACROS backward compatibility synonym and others don't. Can you say a little more about this aspect of the approach? Would the compatibility macros go away eventually?

Thanks, Jonathan