Re: [PATCH 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback

From: Stefan Beller <sbeller@google.com>
Date: Jul 18, 2018, 19:19 UTC
Message-ID: <CAGZ79kZ9oeqidNMppOx+PNfp-4RdDhLzvQHhm45Ceni=g1Q_7Q@mail.gmail.com>
In-Reply-To: <xmqqd0vkjsx3.fsf@gitster-ct.c.googlers.com>

Show 15 quoted lines

> > @@ -317,7 +317,7 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,
> >  int for_each_tag_ref(each_ref_fn fn, void *cb_data);
> >  int for_each_branch_ref(each_ref_fn fn, void *cb_data);
> >  int for_each_remote_ref(each_ref_fn fn, void *cb_data);
> > -int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data);
> > +int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data);
>
> With a signature change like this, any change that introduces new
> call to for_each_replace_ref() using eac_ref_fn() would get
> compilation error, so this is minimally correct.
>
> Two things that bothersome are that
>
>  - for_each_tag/branch/remote_ref() and for_each_replace_ref() now
>    work and look quite differently.

Yes, this series is not finished; we need to convert/upgrade for_each_tag et al, too.

>  - existing users of for_each_replace_ref() who were all happy
>    working in the_repository have to pass it explicitly, even
>    thought they do not have any need to.

All callbacks that are passed to for_each_replace_ref now operate on 'r' instead of the_repository, and that actually fixes a bug (commit message is lacking), but the cover letter hints at:

    While building this series, I got some test failures in the
    non-the_repository tests. These issues are related to missing
    references to an arbitrary repository (instead of the_repository)
    and some uninitialized values in the tests. Stefan already sent
    a patch to address this [2], and I've included those commits
    (along with a small tweak [3]). These are only included for
    convenience.

> In this case, even if you introduced for_each_replace_ref_in_repo(),
> making for_each_replace_ref() a thin wrapper that always uses
> the_repository is a bit more cumbersome than just a simple macro.

Yes, but such a callback would do the *wrong* subtle thing in some cases as you really want to work in the correct repository for e.g. looking up commits.

> But it *is* doable (you'd need to use a wrapping structure around
> cb_data), and a developer who case about maintainability during API
> transition would have taken pains to do so.  A bit dissapointing.

My original patches were RFC-ish and a trade off as for the reflog only there is nothing in flight to care about.

Given that we would want to upgrade all the ref callbacks, we have to take this route, I think.

Thanks, Stefan