On 2/28/2022 9:27 AM, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Feb 28 2022, Derrick Stolee wrote: > >> On 2/25/2022 5:31 PM, Ævar Arnfjörð Bjarmason wrote: >>> Or maybe they won't. I just found it surprising when reviewing this to >>> not find an answer to why that approach wasn't >>> considered. >> >> The point is to create a new format that can be chosen when deployed >> in an environment where older Git versions will not exist (such as >> a Git server). The new version is not chosen by default and instead >> is opt-in through the commitGraph.generationVersion config option. >> >> Perhaps in a year or two we would consider making this the new >> default, but there is no rush to do so. > > Looking into this a bit more I think that in either case this is less of > a big deal after my 43d35618055 (commit-graph write: don't die if the > existing graph is corrupt, 2019-03-25), which came out of some of those > discussions at the time of [1]. > > I.e. now a client that only understands version N-1 will warn when > loading it, wheras it's only if a pre-v2.22.0 client (which has that > commit) reads the repository that we'd hard die on it, correct? > > But speaking of hyper-focus. I think that arguably applies to you in > this case when considering the trade-offs of these sorts of format > changes :) > > I.e. you're primarily considering cases of say a git server (presumably > running on GitHub) or another such deployment where it's easy to have > full control over all of your versions "in the wild". I'm thinking of servers, yes, but also 99% of clients who only upgrade (or _maybe_ downgrade to a recent, previous version occasionally). > And thus a three-phase rollout of something like a format change can be > done in a timely and predictable manner. > > But git is used by *a lot* of people in a bunch of different > scenarios. E.g.: > > * A shared (hopefully read-only) NFS mounted by remote "unmanaged" clients. > * A tarred-up directory including a .git, which may be transferred to > a machine with a pre-v2.22.0 version. > > Or even softer cases of failure, such as: > > * A cronjob causes an alert/incident somewhere because the server > operator started writing a new version, but forgot about a set > of machines that are still on the old version. It is important to continue supporting these cases, and this change does not cause any issues for them. However, this handful of corner cases should not block progress in the main cases. > I think that even if it's less conceptually clean it's worth considering > being over backwards to be kinder to such use-cases, unless it's really > required for other reasons to break such in-the-wild use-cases. > > Or in this case, if it's thought to be worth it to help reviewers decide > by separating the performance improvement aspect from the changed > interaction between new graphs and older clients. > > As a further nit on the proposed end-state here: Do I understand it > correctly that commitGraph.generationVersion=[1|2] (i.e. on current > "master") will always result in a file that's compatible with older > versions, since the only thing "v2" there controls now is to write the > optional GDAT and GDOV chunks? > > Whereas going from commitGraph.generationVersion=2 to > commitGraph.generationVersion=3 in this series will impact older clients > as noted above, since we're bumping the version (of the file, to 2 if > the config is 3, which as Junio noted is a bit confusing). > > I think if you're set on going down the path of bumping the top-level > version that deserves to be made much clearer in the added > documentation. Right now the only hint to that is a passing mention that > for v3: > > [it] will be incompatible with some old versions of Git > > Which if we're opting for breaking format changes really should note > some of the caveats above, that pre-v2.22.0 hard-dies, and probably > describe "some old versions of Git" a bit more clearly. > > It actually means once this gets released "the git version that was the > latest one you could download yesterday". Which a reader of the docs > probably won't expect when starting to play with this in mixed-version > environment. > > 1. https://lore.kernel.org/git/87h8acivkh.fsf@xxxxxxxxxxxxxxxxxxx/ This documentation could be altered to be specific about versions, but such a specific change makes assumptions of the version that will include it. As of now, the generation number v2 fixes will _probably_ get in for 2.36 and the format change would have enough time to cook for 2.37, so I'll update the docs to refer to that version explicitly. The pre-2.22.0 change might be helpful to mention, but it could also be noise to the reader. We can revisit this when these patches are submitted again in another thread. There's also concern about third- party tools like libgit2. I'd rather draw the line as "tread carefully here" than "here is so much information that a reader might think it is all they need to know". Thanks, -Stolee
