git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: gitgitgadget@gmail.com, git <git@vger.kernel.org>,
	"Jakub Narębski" <jnareb@gmail.com>
Subject: Re: [PATCH 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback
Date: Wed, 18 Jul 2018 12:19:05 -0700	[thread overview]
Message-ID: <CAGZ79kZ9oeqidNMppOx+PNfp-4RdDhLzvQHhm45Ceni=g1Q_7Q@mail.gmail.com> (raw)
In-Reply-To: <xmqqd0vkjsx3.fsf@gitster-ct.c.googlers.com>

> > @@ -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

  reply	other threads:[~2018-07-18 19:19 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-18 15:15 [PATCH 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities Derrick Stolee via GitGitGadget
2018-07-18 15:15 ` [PATCH 1/8] refs.c: migrate internal ref iteration to pass thru repository argument Stefan Beller via GitGitGadget
2018-07-18 15:15 ` [PATCH 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback Stefan Beller via GitGitGadget
2018-07-18 18:32   ` Junio C Hamano
2018-07-18 19:19     ` Stefan Beller [this message]
2018-07-18 20:13       ` Junio C Hamano
2018-07-18 15:15 ` [PATCH 3/8] commit-graph: update design document Derrick Stolee via GitGitGadget
2018-07-29 13:44   ` Jakub Narebski
2018-07-18 15:15 ` [PATCH 4/8] test-repository: properly init repo Derrick Stolee via GitGitGadget
2018-07-29 21:07   ` Jakub Narebski
2018-07-18 15:15 ` [PATCH 5/8] commit-graph: not compatible with replace objects Derrick Stolee via GitGitGadget
2018-07-18 19:46   ` Jeff King
2018-07-18 19:48     ` Jeff King
2018-07-18 19:52     ` Derrick Stolee
2018-07-20 16:45       ` Derrick Stolee
2018-07-20 16:49         ` Stefan Beller
2018-07-20 16:57           ` Derrick Stolee
2018-07-29 21:00   ` Jakub Narebski
2018-07-18 15:15 ` [PATCH 6/8] commit-graph: not compatible with grafts Derrick Stolee via GitGitGadget
2018-07-29 22:04   ` Jakub Narebski
2018-07-18 15:15 ` [PATCH 7/8] commit-graph: not compatible with uninitialized repo Derrick Stolee via GitGitGadget
2018-07-29 22:46   ` Jakub Narebski
2018-07-18 15:15 ` [PATCH 8/8] commit-graph: close_commit_graph before shallow walk Derrick Stolee via GitGitGadget
2018-07-30 19:24   ` Jakub Narebski
2018-07-18 15:22 ` [PATCH] DO-NOT-MERGE: write and read commit-graph always Derrick Stolee
2018-07-30 14:39   ` Jakub Narebski
2018-07-30 17:06     ` Stefan Beller
2018-07-31 16:54       ` Jakub Narebski
2018-07-31 17:40   ` Elijah Newren
2018-07-29 13:13 ` [PATCH 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities Jakub Narebski
2018-07-29 19:27   ` Eric Sunshine
2018-08-20 18:24 ` [PATCH v2 " Derrick Stolee
2018-08-20 18:24   ` [PATCH v2 1/8] refs.c: migrate internal ref iteration to pass thru repository argument Derrick Stolee
2018-08-20 18:24   ` [PATCH v2 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback Derrick Stolee
2018-08-20 18:24   ` [PATCH v2 3/8] commit-graph: update design document Derrick Stolee
2018-08-20 18:24   ` [PATCH v2 4/8] test-repository: properly init repo Derrick Stolee
2018-08-20 18:24   ` [PATCH v2 5/8] commit-graph: not compatible with replace objects Derrick Stolee
2018-08-21 17:45     ` Junio C Hamano
2018-08-21 18:39       ` Derrick Stolee
2018-08-20 18:24   ` [PATCH v2 6/8] commit-graph: not compatible with grafts Derrick Stolee
2018-08-20 18:24   ` [PATCH v2 7/8] commit-graph: not compatible with uninitialized repo Derrick Stolee
2018-08-20 18:24   ` [PATCH v2 8/8] commit-graph: close_commit_graph before shallow walk Derrick Stolee
2018-08-20 19:06   ` [PATCH v2 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities Stefan Beller
2018-08-21 17:51     ` Junio C Hamano
2018-08-21 18:35       ` Derrick Stolee
2018-08-20 19:37   ` Stefan Beller
2018-08-20 19:54     ` Derrick Stolee

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='CAGZ79kZ9oeqidNMppOx+PNfp-4RdDhLzvQHhm45Ceni=g1Q_7Q@mail.gmail.com' \
    --to=sbeller@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=jnareb@gmail.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).