Handle macro attribute references with unspecified flag #5168
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrisd8088
force-pushed
the
unspecified-macro-attrs
branch
from
3bcdc2e
to
111b426
Compare
In commit f4b8938 of PR git-lfs#3391 the t/t-attributes.sh file was added with an initial "macros" test, and part of that test confirms that macro attribute definitions are only processed when they appear in the top-level .giattributes file in a repository. The test does confirm this in that it creates both an "lfs2" macro attribute definition and assignment of that attribute name to a file pattern in a .gitattributes file in a subdirectory, and then validates that a file matching that pattern in the subdirectory is not converted into an LFS object. The test also includes a second check of this logic in which it confirms that a "git lfs track" command for the file pattern in the subdirectory succeeds, i.e., that it does not fail because the file pattern was already assigned the normal "filter=lfs" attribute by the "lfs2" macro attribute. However, this particular check will always succeed, even if macro attribute definitions like the "lfs2" one are incorrectly accepted from .gitattributes files other than the top-level one. This is because the "git lfs track" command is run in the top-level directory and only sets a pattern that includes the subdirectory in its path (i.e., "dir/*.bin"). This will succeed regardless of whether the "*.bin" pattern is assigned to LFS attributes in the dir/.gitattributes file. We therefore make this second check more sensitive to potential future regressions by running the "git lfs track" command in the subdirectory. Now if the macro definition in the .gitattributes file in that directory is (incorrectly) read as a valid definition, and the check which tests that the file in the subdirectory has not been converted into an LFS object is skipped, then this second check fails as expected.
chrisd8088
force-pushed
the
unspecified-macro-attrs
branch
from
111b426
to
1263ab7
Compare
chrisd8088
changed the title
In commit f4b8938 of PR git-lfs#3391 the t/t-attributes.sh test script was introduced with its initial "macros" test, which validates that the "git lfs track" command is able to parse macro attribute definitions in the top-level .gitattributes file and resolve references to those macros in the same file. It also confirms that the command does not accept macro definitions in .gitattributes files in subdirectories, as Git does not accept these either. However, Git does resolve macro attribute references from .gitattributes files in subdirectories, so long as they refer to macro attributes defined in the top-level .gitattributes (or one of the other files where definitions are accepted, such as the .git/info/attributes file). But the "git lfs track" command at present does not resolve such references consistently because it sorts the attributes files by path length and then processes them strictly in that order, from longest to shortest. Thus references to macro attributes defined in the top-level .gitattributes file from other attributes files never succeed because the top-level file is always parsed last (except for the global and system attributes files). We therefore add a note to this effect in the "macros" test to explain why we do not test valid macro attribute references in a .gitattributes file in a subdirectory. (There is also an inconsistency in how "git lfs track" handles references to macro attributes defined in the .git/info/attributes file, because if the references appear in .gitattributes files whose full file path in the repository is longer than ".git/info/attributes", then the references are not resolved as these files are parsed before the .git/info/attributes one, whereas references from other .gitattributes files are resolved.) Separately, in commit 608bc8d of PR git-lfs#4525 support for scanning the repository contents using the output of the "git ls-tree" command was added to help enable the "git lfs fsck" to search for invalid Git LFS pointer files. The GitScanner.ScanRefByTree() method invokes a chain of functions, of which catFileBatchTreeForPointers() reads Git blob metadata and examines each blob in turn to see if it is a Git LFS pointer or a .gitattributes file, and if it is the latter it reads and parses its contents, including macro attribute definitions if the file is the top-level .gitattributes file. We therefore add a "fsck detects invalid pointers with macro patterns" test to the t/t-fsck.sh test script which validates the ability of the "git lfs fsck" command to report as invalid pointers any files matching patterns with a "filter=lfs" attribute defined by reference to a macro attribute defined in the top-level .gitattributes file. To do this we refactor the setup_invalid_pointers() helper function so that we can reuse some of its code in a new, smaller function that just creates invalid pointers. However, we also add a note explaining that we can not yet test this behaviour with a .gitattributes file whose parent directory sorts before the top-level .gitattributes one in the output from "git ls-tree". Because that command outputs its results sorted by filepath, a file such as .dir/.gitattributes will be listed before the top-level .gitattributes file, and so any macro attribute references from the .dir/.gitattributes file to macro attributes defined in the top-level .gitattributes file will not be resolved in the way that Git resolves them. For now we defer resolution of this issue and the ones described regarding the "git lfs track" command to the future.
In commit 1ff5254 of PR git-lfs#3391 we introduced the MacroProcessor type and methods to support the use of macro attributes in .gitattributes files. However, we do not currently support the case where a macro attributes is specified with a "!" prefix, which Git handles by setting all attributes defined by the macro attribute back to the unspecified state. (Note that the "-" prefix is not supported by Git for macro attributes, only the "!" one.) To mimic the same behaviour in Git LFS we add a check for a macro attribute with its Unspecified bool set to "true", and when this is detected we iterate through the set of attributes defined by the macro attribute and set them all to the same unspecified state. We also add tests to confirm this new handling works as expected, both a new Go test and a new "fsck does not detect invalid pointers with negated macro patterns" test in t/t-fsck.sh that will not succeed without the changes to the MacroProcessor in this commit. Without these changes, any patterns that reference a macro attribute with the "!" prefix are not processed as making the macro's attributes all unspecified again, and so non-pointer files matching those patterns are reported as invalid Git LFS pointers. In the new test in t/t-fsck.sh we include comments describing how the "git lfs fsck" command currently processes .gitattributes files in the order returned by "git ls-tree", and so a .gitattributes file in a subdirectory such as .dir/ will be parsed before the top-level .gitattributes one because it appears first in the "git ls-tree" output. The result is that any macro attribute references in the .dir/.gitattributes file will not be resolved properly, and so our test succeeds but not quite for the right reasons. We also add a new "macros with unspecified flag" test in the t/t-attributes.sh test script, but this test ultimately is only a placeholder as it can not actually test that the "git lfs track" command will not overwrite a pattern in a .gitattributes file in a subdirectory if it references a macro attribute defined in the top-level .gitattributes file and the reference has the "!" prefix. This is due to the fact that the "git lfs track" command parses .gitattributes files in the order of the length of their full paths, from longest to shortest, and so macro attribute references can not be resolved except within the top-level .gitattributes file (with some caveats regarding the .git/info/attributes file and the global and system attributes files). For now we defer resolution of both this issue and the one described regarding the "git lfs fsck" command to the future.
chrisd8088
force-pushed
the
unspecified-macro-attrs
branch
from
1263ab7
to
b8d97e3
Compare
bk2204
approved these changes
Nov 8, 2022
This was referenced Nov 11, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In commit 1ff5254 of PR #3391 we introduced the
MacroProcessortype and methods to support the use of macro attributes in.gitattributesfiles.However, we do not currently support the case where a macro attributes is specified with a
!prefix, which Git handles by setting all attributes defined by the macro attribute back to the unspecified state. (Note that the-prefix is not supported by Git for macro attributes, only the!one.)To mimic the same behaviour in Git LFS we add a check for a macro attribute with its
Unspecifiedboolset totrue, and when this is detected we iterate through the set of attributes defined by the macro attribute and set them all to the same unspecified state.We also add tests to confirm this new handling works as expected, both a new Go test and two new tests in the
t/t-fsck.shtest scrript. In the latter file we refactor thesetup_invalid_pointers()helper function so that we can reuse some of its code in a new, smaller function that just creates invalid pointers.The new
"fsck does not detect invalid pointers with negated macro patterns"test int/t-fsck.shwill not succeed without the changes to theMacroProcessorin this PR, because without those changes any patterns that reference a macro attribute with the!prefix are not processed as making the macro's attributes unspecified again, and so non-pointer files matching those patterns are reported as invalid Git LFS pointers. The"fsck detects invalid pointers with macro patterns"test, on the other hand, simply validates existing behaviour.In both of the new tests in
t/t-fsck.shwe include comments describing how thegit lfs fsckcommand currently processes.gitattributesfiles in the order returned bygit ls-tree, and so a.gitattributesfile in a directory such as.dir/will be parsed before the top-level.gitattributesone because it sorts first. The result is that any macro attribute references in the.dir/.gitattributesfile will not be resolved properly, and so our tests variously either skip testing this situation or succeed but not quite for the right reasons.We also update the
"macros"test int/t-attributes.shso that one of its checks that validates the Git LFS is not processing macro attribute definitions in.gitattributesfiles other than the top-level one would actually fail if this were not to be the case due to some regression of the existing logic.Finally, we add a new
"macros with unspecified flag"test in thet/t-attributes.shtest script, but this test ultimately is only a placeholder as it can not actually test that thegit lfs trackcommand will not overwrite a pattern in a.gitattributesfile in a subdirectory if it references a macro attribute defined in the top-level.gitattributesfile and the reference has the!prefix. This is due to the fact that thegit lfs trackcommand parses.gitattributesfiles in the order of the length of their full paths, from longest to shortest, and so macro attribute references can not be resolved except within the top-level.gitattributesfile (with some caveats regarding the.git/info/attributesfile and the global and system attributes files). We also add comments to the"macros"test relating to this issue. For now we defer resolution of both this issue and the one described regarding the order in which thegit lfs fsckcommand processes.gitattributesfiles to the future.It may be easiest to review this PR commit-by-commit, as each one has a detailed description and should be logically independent and bisectable, but it should also be possible to review this PR as a single diff.
/cc @baffles as reporter.
Fixes #5145.