All of lore.kernel.org
 help / color / mirror / Atom feed
From: Brandon Williams <bmwill@google.com>
To: Duy Nguyen <pclouds@gmail.com>
Cc: Stefan Beller <sbeller@google.com>,
	Git Mailing List <git@vger.kernel.org>,
	Michael Haggerty <mhagger@alum.mit.edu>
Subject: Re: [PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API
Date: Fri, 27 Jul 2018 10:19:41 -0700	[thread overview]
Message-ID: <20180727171941.GA109508@google.com> (raw)
In-Reply-To: <CACsJy8Ae3sZvOQ3irQM+hv0fCRchGi8995kvLZBadbaphRo-3A@mail.gmail.com>

On 07/27, Duy Nguyen wrote:
> On Fri, Jul 27, 2018 at 2:40 AM Stefan Beller <sbeller@google.com> wrote:
> >
> > Currently the refs API takes a 'ref_store' as an argument to specify
> > which ref store to iterate over; however it is more useful to specify
> > the repository instead (or later a specific worktree of a repository).
> 
> There is no 'later'. worktrees.c already passes a worktree specific
> ref store. If you make this move you have to also design a way to give
> a specific ref store now.
> 
> Frankly I still dislike the decision to pass repo everywhere,
> especially when refs code already has a nice ref-store abstraction.
> Some people frown upon back pointers. But I think adding a back
> pointer in ref-store, pointing back to the repository is the right
> move.

I don't quite understand why the refs code would need a whole repository
and not just the ref-store it self.  I thought the refs code was self
contained enough that all its state was based on the passed in
ref-store.  If its not, then we've done a terrible job at avoiding
layering violations (well actually we're really really bad at this in
general, and I *think* we're trying to make this better though the
object store/index refactoring).

If anything I would expect that the actual ref-store code would remain
untouched by any refactoring and that instead the higher-level API that
hasn't already been converted to explicitly use a ref-store (and instead
just calls the underlying impl with get_main_ref_store()).  Am I missing
something here?

-- 
Brandon Williams

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

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-27  0:36 [RFC PATCH 0/3] Migrate the refs API to take the repository argument Stefan Beller
2018-07-27  0:36 ` [PATCH 1/3] refs.c: migrate internal ref iteration to pass thru " Stefan Beller
2018-07-27  0:36 ` [PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API Stefan Beller
2018-07-27 16:07   ` Duy Nguyen
2018-07-27 17:19     ` Brandon Williams [this message]
2018-07-27 17:30       ` Stefan Beller
2018-07-27 18:04         ` Duy Nguyen
2018-07-30 19:47           ` [PATCH 0/2] Cleanup refs API [WAS: Re: [PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API] Stefan Beller
2018-07-30 19:47             ` [PATCH 1/2] replace-objects: use arbitrary repositories Stefan Beller
2018-07-30 19:47             ` [PATCH 2/2] refs: switch for_each_replace_ref back to use a ref_store Stefan Beller
2018-07-31  0:18               ` Jonathan Tan
2018-07-31  0:41                 ` Stefan Beller
2018-07-31 16:17                   ` Duy Nguyen
2018-07-27  0:36 ` [PATCH 3/3] replace: migrate to for_each_replace_repo_ref 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

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

  git send-email \
    --in-reply-to=20180727171941.GA109508@google.com \
    --to=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=mhagger@alum.mit.edu \
    --cc=pclouds@gmail.com \
    --cc=sbeller@google.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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.