On Fri, Mar 17, 2023 at 01:24:49PM -0700, Jonathan Tan wrote: > Patrick Steinhardt <ps@xxxxxx> 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] > > 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. > > 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
Attachment:
signature.asc
Description: PGP signature
