Proposal: new hook to detect LFS attribute inconsistencies

[klimkin]

There are two common errors when working with LFS:

• Keeping files as general objects after setting LFS attributes (LFS produces a warnings like "Encountered 1 file(s) that should have been pointers, but weren't")
• Keeping LFS pointers after removing the attributes.

Proposed implementation of a check-lfs-attributes hook: https://github.com/klimkin/pre-commit-hooks/tree/feature/check-lfs-attributes

[asottile]

doesn't lfs already have things in place to prevent this?

[klimkin]

Below, after tracking the file you must add it again to create proper commit. I don't think LFS can prevent this kind of escape unless we use some kind of Git hook.

@xfailif_no_gitlfs
def test_regular_object_but_tracked_by_lfs(temp_git_dir_as_cwd, capsys):  # pragma: no cover
    temp_git_dir_as_cwd.join('a.bin').write('a')
    cmd_output('git', 'lfs', 'install', '--local')
    cmd_output('git', 'add', 'a.bin')
    cmd_output('git', 'lfs', 'track', '*.bin')
    assert main(('a.bin',)) == 1
    out, _ = capsys.readouterr()
    assert 'a.bin is tracked by LFS but added as a regular object' in out

[asottile]

right but when a push is executed lfs checks this right?

[klimkin]

It doesn't seem LFS is checking for inconsistencies. You can push .gitattributes without converting to pointers.

Looking at the LFS pre-push hook https://github.com/git-lfs/git-lfs/blob/b96d77b9563a8fd10c4e03f8f8898a0777ead9a6/commands/command_pre_push.go#L19, it only handles the logic of pushing the content for the pointers.

[asottile]

that really feels like something lfs should just fix -- I thought their pre-push did more validation than that?

[asottile]

I also think that check-added-large-files is sufficient to catch this case already

[klimkin]

There are two cases to catch:

1. Files with lfs attribute, but committed as general objects
2. Files without lfs attribute, but committed as LFS pointers

@asottile Are you suggesting adding the checks to check-added-large-files?

[klimkin]

that really feels like something lfs should just fix -- I thought their pre-push did more validation than that?

Did it?