Refactor macro attribute handling to prevent crashes with --fixup migration option
#5382
Conversation
This file contains hidden or 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
The "migrate info (--fixup, no .gitattributes)" test was added to the t/t-migrate-info.sh test suite in commit 4800c5e of PR git-lfs#4501, when we added --fixup option support to the "git lfs migrate info" command. As we expect to introduce additional, similar tests in subsequent commits in this PR, we rename the test to clarify that no fixup action is expected in this case, and refactor the test's invocation of the "git lfs migrate info --fixup" command so that it does not run inside a pipeline. This latter change ensures that if the command fails, the test also fails due to the "set -e" shell option. While this change to avoid the use of a pipeline is not critical for this test, it will be important for the similar tests we add in subsequent commits, because they will need to validate that the "git lfs migrate info --fixup" command does not simply crash and produce no output on stdout, which, if it occured in a pipeline, would not be detected. This is particularly true in these tests because we expect the command to produce no output on stdout, which it would not do both on success and if it panics, so we can not distinguish these cases by examining the number of lines of output. We then make a copy of this test in the t/t-migrate-fixup.sh test suite as well, except using the "git lfs migrate import --fixup" command, as we also expect to add similar tests to this suite in subsequent commits. Note that for this test it is critical that we supply the --yes option to the command, to avoid it waiting forever for input because it has detected the log file as a new file in the Git working tree and therefore has prompted for confirmation to proceed. Finally, we remove one unneeded "git add" command from the setup_single_local_branch_tracked_corrupt() test helper function, which we refactor further in subsequent commits.
Support for Git macro attributes was added in a series of commits in PR git-lfs#3391, including commit 9d3e52d, where the Line structure of the "git/gitattr" package was updated to include a Macro element which would be set non-nil for macro definition lines in a .gitattributes file file, while the existing Pattern element would be set non-nil for all other lines. The "git lfs track" command, among others, was then adjusted to create a MacroProcessor structure (from the same "git/gitattr" package) and call its ProcessLines() method to resolve any macro references and thus convert the "raw" parsed Line structures into a set for which the Pattern element was always non-nil, and no Macro elements appeared. Later, the "git lfs fsck" command gained the ability to process macro definitions in .gitattributes files, in PR git-lfs#4525. However, the "git lfs migrate import" command was not adjusted, specifically in the implementation of its "--fixup" option, which initializes a Tree structure (also of the "git/gitattr" package) for the root tree of each commit in a repository's history using the package's New() function. This function traverses all the trees in the hierarchy and finds and parses all the .gitattributes files in them. Then, when the command visits each file within the commit's tree using the Rewrite() method of the Rewriter structure in the "git/githistory" package, it calls the (*Tree).Applied() method to match the file's path against any applicable Git attributes, to see if the file should be treated as a Git LFS object. This lack of support for macro attributes in the "git lfs migrate import --fixup" command was then propagated to the "git lfs migrate info --fixup" command in commit 4800c5e of PR git-lfs#4501, when the "git lfs migrate info" command was updated to respect the --fixup option. As a result, both of these commands (when used with the --fixup option) panic if they encounter a .gitattributes file with any macro definitions, as they call the (*Tree).Applied() method and it attempts to access the nil Pattern element of the lines with non-nil Macro elements. (Prior to the changes in commit c374d1f of PR git-lfs#5375 the "git lfs migrate import --fixup" command would then stall indefinitely, but it now also exits after the panic condition.) These problems were reported in issue git-lfs#5332. To resolve this problem and avoid similar ones in the future, we refactor the Line structure into a Line interface, which only provides an Attrs() method to retrieve a slice of Attr attributes, and no other methods. We then also define two additional interfaces, each of which embeds the Line interface, PatternLine and MacroLine, with corresponding getter methods for their respective elements. The ParseLine() function of the "git/gitattr" package now returns a slice of generic Line types, each of which is either a PatternLine or a MacroLine, but never both. Callers like the Applied() method of the Tree structure therefore need to perform type assertions or switches to determine which type of Line they are handling, which ensures they always access the line's data through safe methods. We then update the Go tests for the "git/gitattr" package as appropriate, and also add two tests each to the t/t-migrate-fixup.sh and t/t-migrate-import.sh test suites. All four of these new shell tests fail without the changes in this commit. In particular, several of these tests make sure to run the "git lfs migrate" commands outside of any shell pipeline so the test will fail if the command panics and produces no output, even if no output is the expected condition for a successful execution of the command.
In a previous commit in this PR we have refactored our handling of parsed lines from .gitattributes files, and in doing so we have updated the Applied() method of the Tree structure in the "git/gitattr" package so that it has to perform a type check on each Line to confirm that it implements the PatternLine interface, before applying that Git attribute's match pattern to the method's file path argument. This resolves the problem reported in git-lfs#5332 where the "git lfs migrate info --fixup" and "git lfs migrate import --fixup" commands would panic when they encountered a .gitattributes file with a non-pattern macro definition line. However, these commands do not yet fully support macro attributes, as they never create a MacroProcessor structure and thus never call its ProcessLines() method on the "raw" lines parsed from each .gitattributes file they encounter while traversing a repository's Git commits and trees. As a result, if a valid .gitattributes file exists in a repository and defines a macro attribute for the "lfs" filter, and assigns this macro attribute to a pattern which matches a file that is not a proper Git LFS object, the two commands ignore this and do not either report or convert the object into a Git LFS object as they should. While we expect to resolve this problem in a subsequent commit, for the present time we simply add two tests which illustrate the issue, but which we comment out as they would otherwise cause their test suites to fail.
chrisd8088
force-pushed
the
fix-migrate-info-panic
branch
from
ba69fc4 to
07b1adf
Compare
bk2204
approved these changes
Jun 1, 2023
chrisd8088
added a commit
to Diamondemon/git-lfs
that referenced
this pull request
Nov 2, 2025
In PR git-lfs#5382 we resolved the problem reported in git-lfs#5332 where the "git lfs migrate info --fixup" and "git lfs migrate import --fixup" commands would panic when they encountered a .gitattributes file with a non-pattern macro definition line. However, at the time we did not update these commands to fully support macro attributes, so that if a valid .gitattributes file existed in a repository and defined a macro attribute for the "lfs" filter, and assigned this macro attribute to a pattern which matched a file that was not a proper Git LFS object, the two commands would ignore this and not convert the object into a Git LFS object. In commit 07b1adf of PR git-lfs#5382 we did, though, add two tests to our t/t-migrate-info.sh and t/t-migrate-fixup.sh test scripts which were designed to validate that the "git lfs migrate" command, when used with its --fixup option, correctly and fully supported the use of macro attributes. We left these tests commented out, with the expectation that we could enable them once we resolved the remaining problems with the command's support for macro attributes. In prior commits in this PR we have now refactored and improved the processing of Git attributes by the "git lfs migrate" command and so the problems described in commit 07b1adf have been resolved. We can therefore now enable the two tests that commit introduced. Note that we make two revisions to the "migrate import (--fixup, .gitattributes with LFS macro)" test as it was originally written. First, we fix a typo in name of the test, as it originally included the subcommand name "info" instead of "import". Second, we replace the final check in the test with two different checks. In the original version, the final grep(2) command checked for the presence of a non-macro Git LFS attribute definition; this was inappropriately copied from the last check of the preceding "migrate import (--fixup, .gitattributes with macro)" test. Instead, we need to check for two lines in the ".gitattributes" file, one of which defines an "lfs" macro and the other of which applies that macro definition to a specific file path pattern.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Support for Git macro attributes was added in a series of commits in PR #3391, including commit 9d3e52d, where the
Linestructure of thegit/gitattrpackage was updated to include aMacroelement which would be set non-nilfor macro definition lines in a.gitattributesfile file, while the existingPatternelement would be set non-nilfor all other lines.Multiple commands were then revised to make use of this functionality, including
git lfs trackand, in PR #4525,git lfs fsck. However, thegit lfs migrate importcommand was not adjusted, specifically in the implementation of its--fixupoption, which initializes aTreestructure (also of thegit/gitattrpackage) for the root tree of each commit in a repository's history using the package'sNew()function. This function traverses all the trees in the hierarchy and finds and parses all the.gitattributesfiles in them. Then, when the command visits each file within the commit's tree using theRewrite()method of theRewriterstructure in thegit/githistorypackage, it calls the(*Tree).Applied()method to match the file's path against any applicable Git attributes, to see if the file should be treated as a Git LFS object.This lack of support for macro attributes in the
git lfs migrate import --fixupcommand was then propagated to thegit lfs migrate info --fixupcommand in commit 4800c5e of PR #4501, when thegit lfs migrate infocommand was updated to respect the--fixupoption.As a result, both of these commands (when used with the
--fixupoption) panic if they encounter a.gitattributesfile with any macro definitions, as they call the(*Tree).Applied()method and it attempts to access thenilPatternelement of the lines with non-nilMacroelements. (Prior to the changes in commit c374d1f of PR #5375 thegit lfs migrate import --fixupcommand would then stall indefinitely, but it now also exits after the panic condition.) These problems were reported in issue #5332.To resolve this problem and avoid similar ones in the future, we refactor the
Linestructure into aLineinterface, which only provides anAttrs()method to retrieve a slice ofAttrattributes, and no other methods. We then also define two additional interfaces, each of which embeds theLineinterface,PatternLineandMacroLine, with corresponding getter methods for their respective elements.The
ParseLine()function of thegit/gitattrpackage now returns a slice of genericLinetypes, each of which is either aPatternLineor aMacroLine, but never both. Callers like theApplied()method of theTreestructure therefore need to perform type assertions or switches to determine which type ofLinethey are handling, which ensures they always access the line's data through safe methods.We then update the Go tests for the
git/gitattrpackage as appropriate, and also add two tests each to thet/t-migrate-fixup.shandt/t-migrate-import.shtest suites. All four of these new shell tests fail without the changes in this commit.In particular, several of these tests make sure to run the
git lfs migratecommands outside of any shell pipeline so the test will fail if the command panics and produces no output, even if no output is the expected condition for a successful execution of the command.Despite these changes, however, the
git lfs migrate info --fixupandgit lfs migrate import --fixupcommands do not yet fully support macro attributes, as they never create aMacroProcessorstructure and thus never call itsProcessLines()method on the "raw" lines parsed from each.gitattributesfile they encounter while traversing a repository's Git commits and trees.As a consequence, if a valid
.gitattributesfile exists in a repository and defines a macro attribute for thelfsfilter, and assigns this macro attribute to a pattern which matches a file that is not a proper Git LFS object, the two commands ignore this and do not either report or convert the object into a Git LFS object as they should.While we expect to resolve this problem in a subsequent commit, for the present time we simply add two tests which illustrate the issue, but which we comment out as they would otherwise cause their test suites to fail.
It may be easiest to review this PR commit-by-commit, as each commit has a detailed description and should be logically independent and bisectable.
Fixes #5332, in that the
git lfs migratecommands no longer panic, but note that macro attributes are still not supported correctly by the--fixupoptions./cc @recih as reporter.