Re: [PATCH 0/8] fetch: refactor code that prints reference updates

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 20, 2023, 06:57 UTC
Message-ID: <ZBgD3Ux34V/U/Lv5@ncase>
In-Reply-To: <20230317202449.1083635-1-jonathantanmy@google.com>

On Fri, Mar 17, 2023 at 01:24:49PM -0700, Jonathan Tan wrote:

Show 13 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> >     1. We want to take control of the reference updates so that we can
> >        atomically update all or a subset of references that git-fetch
> >        would have updated.
> > 
> >     2. We want to be able to quarantine objects in a fetch so that we
> >        can e.g. perform consistency checks for them before they land in
> >        the main repository.
> 
> If you want to do this, something that might be possible is to change
> the RHS of the refspecs to put the refs in a namespace of your choice
> (e.g. ...:refs/<UUID>/...) and then you can look at what's generated and
> process them as you wish.

There's two major problems with this, unfortunately:

- We want to use the machine-parseable format in our repository
  mirroring functionality, where you can easily end up fetching
  thousands or even hundreds of thousands of references. If you need to
  write all of them anew in a first step then you'll end up slower than
  before.

- Repository mirroring is comparatively flexible in what it allows. Most
  importantly, it gives you the opiton to say that divergent references
  should not be updated at all, which translates into an unforced fetch.
  It's even possible to have fetches with mixed forced and unforced
  reference updates. So if we fetched into a separate namespace first,
  we'd now have to reimplement checks for forced updates in Gitaly so
  that we correctly update only those refs that would have been updated
  by Git. We'd also need to manually figure out deleted references.

  This would be quite a risky change and would duplicate a lot of
  knowledge. Furthermore, merging the two sets of references would
  likely be quite expensive performance-wise.

Also, even if we did have a different RHS, it still wouldn't fix the issue that objects are written into the main object database directly. Ideally, we'd really only accept changes into the repository once we have fully verified all of them. Right now it can happen that we refuse a fetch, but the objects would continue to exist in the repository.

A second motivation for the quarantine directory is so that we can enumerate all objects that are indeed new. This will eventually be used to implement more efficient replication of the repository, where we can theoretically just take all of the fetched objects in the quarantine object directory and copy it to the replicas of that repository.

[snip]

Show 7 quoted lines

> >   fetch: deduplicate handling of per-reference format
> 
> I'm not so sure that this is the correct abstraction. I think that this
> and the last patch might be counterproductive to your stated goal of
> having one more mode of printing the refs, in fact, since when we have
> that new mode, the format would be different but the printing would
> remain (so we should split the format and printing).

I already have the full implementation of the new machine-parseable format available locally, but didn't want to send it as part of this patch series yet to avoid it becoming overly large. But I can say that this change really did make the end goal easier to achieve, due to two reasons:

- If we continued to handle the per-reference format at the different
  callsites, I'd have to also amend each of the callers when introducing
  the new format as we're going to use a different format there. But
  when doing this in `format_display()`, we really only need to have a
  single switch at the beginning to check whether to use the machine
  parseable format or the other one.

- Currently, all reference updates are printed to stderr. As stderr is
  also used to display errors and the progress bar, this really makes it
  not a good fit for the machine-parseable format. Instead, I decided
  that it would make more sense to print the new format to stdout. And
  by having the printing-logic self-contained we again only have a
  single location we need to change.

I realize though that all of this isn't as well-documented in the commit messages as it should be, which is also something that Junio complained about. I'll hopefully do a better job in v2 of this patch series.

Show 12 quoted lines

> >   fetch: deduplicate logic to print remote URL
> 
> Makes sense, although I would need to consider only storing the
> raw URL in the struct display_state and processing it when it needs
> to be emitted (haven't checked if this is feasible, though).
> 
> >   fetch: fix inconsistent summary width for pruned and updated refs
> 
> This changes the behavior in that the summary width, even when printing
> the summary of pruned refs, is computed based only on the updated refs.
> The summary width might need to remain out of the struct display_state
> for now.

Fair, that's a case I didn't yet consider. I'll have another look.

Patrick