On Mon, May 27, 2024 at 10:13:55PM -0400, Joey Hess wrote:
> Johannes Schindelin wrote:
> > > More than one major project; they also broke git-annex in the case where
> > > a git-annex repository, which contains symlinks into
> > > .git/annex/objects/, is pushed to a bare repository with
> > > receive.fsckObjects set. (Gitlab is currently affected[1].)
> >
> > This added fsck functionality was specifically marked as `WARN` instead of
> > `ERROR`, though. So it should not have failed.
>
> A git push into a bare repository with receive.fsckobjects = true fails:
>
> joey@darkstar:~/tmp/bench/bar.git>git config --list |grep fsck
> receive.fsckobjects=true
> joey@darkstar:~/tmp/bench/bar.git>cd ..
> joey@darkstar:~/tmp/bench>cd foo
> joey@darkstar:~/tmp/bench/foo>git push ../bar.git master
> Enumerating objects: 4, done.
> Counting objects: 100% (4/4), done.
> Delta compression using up to 12 threads
> Compressing objects: 100% (2/2), done.
> Writing objects: 100% (3/3), 324 bytes | 324.00 KiB/s, done.
> Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
> remote: error: object ea461949b973a70f2163bb501b9d74652bde9e30: symlinkPointsToGitDir: symlink target points to git dir
> remote: fatal: fsck error in pack objects
> error: remote unpack failed: unpack-objects abnormal exit
> To ../bar.git
> ! [remote rejected] master -> master (unpacker error)
> error: failed to push some refs to '../bar.git'
>
> So I guess that the WARN doesn't work like you expected it to in this case of
> receive.fsckobjects checking.
This is a long-standing weirdness with the fsck severities. The
fsck_options struct used for fetches/pushes has the "strict" flag set,
which upgrades warnings to errors. But if you manually configure a
severity to "warn", then we respect that.
For example, try:
git init
git commit --allow-empty -m 'message with NUL'
commit=$(git cat-file commit HEAD |
perl -pe 's/NUL/\0/' |
git hash-object -w --stdin -t commit --literally)
git update-ref HEAD $commit
which is defined as WARN in fsck.h. And hence:
$ git fsck; echo $?
warning in commit 09b2d5bda87ffda7a0f36ea80c4b542edf9b9374: nulInCommit: NUL byte in the commit object body
Checking object directories: 100% (256/256), done.
0
But that's upgraded to ERROR for transfers:
$ git init --bare dst.git
$ git -C dst.git config transfer.fsckObjects true
$ git push dst.git
...
remote: error: object e6db180f21250e03b633a3684f593ceb7b9cd844: nulInCommit: NUL byte in the commit object body
remote: fatal: fsck error in packed object
error: remote unpack failed: unpack-objects abnormal exit
To dst.git
! [remote rejected] main -> main (unpacker error)
error: failed to push some refs to 'dst.git'
Unless we override it:
$ git -C dst.git config receive.fsck.nulInCommit warn
$ git push dst.git
remote: warning: object 09b2d5bda87ffda7a0f36ea80c4b542edf9b9374: nulInCommit: NUL byte in the commit object body
To dst.git
* [new branch] main -> main
But of course most sites just use the defaults, so all warnings are
effectively errors.
I think it's been this way at least since c99ba492f1 (fsck: introduce
identifiers for fsck messages, 2015-06-22). We've discussed it once or
twice on the list. It mostly seemed like a cosmetic issue to me, but in
this case it looks like it caused functional confusion.
I don't think just turning off the "strict" flag is a good idea, though.
The current severities are all over the place. A missing space in an
ident line is an error, but a tree with a ".git" directory is just a
warning!
So I think we'd first want to straighten out the severities, and then
think about letting warnings bypass transfer fscks. Though it's not
clear to me what hosters would want; pushing to a public site is a great
time to let people know their objects are broken _before_ everyone else
sees them, even if it's "just" a warning. But when you do have old
history with broken objects, the control and incentives are in the wrong
place; every person who wants to interact with the repo has to loosen
their fsck config. So it's not clear to me how aggressive transfer-level
fsck-ing should be.
In the meantime, we also have an "INFO" severity which gets reported but
not upgraded via strict. It sounds like that's what was intended here.
It should be available in all backport versions if we want it; it was
introduced in f27d05b170 (fsck: allow upgrading fsck warnings to errors,
2015-06-22).
-Peff
