| Message ID | cover.1678878623.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | fetch: refactor code that prints reference updates | expand |
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 should be as few global state as possible. This is to reduce > confusion and having to repeat the same incantations in multiple > different locations. Makes sense. > - The logic should be as self-contained as possible. This is so that > it can easily be changed in a subsequent patch series. Also makes sense, but I think that some of your patches might be contrary to this goal (more details below). I've read all the patches, but will just summarize my thoughts here. > Patrick Steinhardt (8): > fetch: rename `display` buffer to avoid name conflict One other way, as others have discussed, is to just name the new variable display_state. (I would prefer that, at the very least so that in case someone else has a patch that contains the identifier "display", problems would be more easily noticed. This is very unlikely to happen but I think it's a good general direction for the Git project to follow.) > fetch: move reference width calculation into `display_state` > fetch: move output format into `display_state` > fetch: pass the full local reference name to `format_display` All these are good changes that I would be happy to see merged. > 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). > 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. > fetch: centralize printing of reference updates Same as "fetch: deduplicate handling of per-reference format".
On Fri, Mar 17, 2023 at 01:24:49PM -0700, Jonathan Tan wrote: > 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] > > 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
On Fri, Mar 17, 2023 at 01:24:49PM -0700, Jonathan Tan wrote: > Patrick Steinhardt <ps@pks.im> writes: [snip] > > 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). We likely could, but right now the benefit isn't all that high. If the URL was only used in `display_ref_update()` then this would be easy enough to do. But we also access the sanitized URL when the connectivity check fails or when printing to FETCH_HEAD. If we provided an accessor function thet returns the URL it would be trivial to do, but what do we really gain here? In the best case we save an allocation for the URL and two loops ranging over it. That doesn't quite feel worth it to me. Patrick
Hi, at GitLab, we want to gain more control over fetches to achieve two different things: 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. To do this, we aim to use git-fetch(1)'s `--dry-run` mode with a manually set up quarantine directory. One issue we currently face though is that git-fetch(1), to the best of my knowledge, has no mode in which it would print all reference updates in a machine-parseable format. I thus set out to implement a "porcelain"-style mode for git-fetch(1) that surfaces this information: - The reference that would be updated. - The remote reference this is coming from. - The old and new object IDs of the reference. - Whether there's any error, like a D/F conflict. I had a hard time understanding the current implementation of how ref updates are printed though. So as a first step towards such a porcelain mode this patch series refactors said code. It sets out to achieve two major goals: - There should be as few global state as possible. This is to reduce confusion and having to repeat the same incantations in multiple different locations. - The logic should be as self-contained as possible. This is so that it can easily be changed in a subsequent patch series. This patch series does exactly that, but does not yet introduce the new machine-parsebale porcelain mode. Patrick Patrick Steinhardt (8): fetch: rename `display` buffer to avoid name conflict fetch: move reference width calculation into `display_state` fetch: move output format into `display_state` fetch: pass the full local reference name to `format_display` fetch: deduplicate handling of per-reference format fetch: deduplicate logic to print remote URL fetch: fix inconsistent summary width for pruned and updated refs fetch: centralize printing of reference updates builtin/fetch.c | 270 ++++++++++++++++++++++++------------------------ 1 file changed, 134 insertions(+), 136 deletions(-)