avoid panic on checkout with --to but no path, and update checkout manual#4766
Merged
chrisd8088 merged 3 commits intogit-lfs:mainfrom Dec 7, 2021
Merged
avoid panic on checkout with --to but no path, and update checkout manual#4766chrisd8088 merged 3 commits intogit-lfs:mainfrom
--to but no path, and update checkout manual#4766chrisd8088 merged 3 commits intogit-lfs:mainfrom
Conversation
At present if the "git lfs checkout" command is invoked with the --to option, a file path, and a "stage" option (--base, --ours, or --theirs) , but without any object path, a Go panic is reported, because there is an attempt to reference the non-existent first element of the "args" slice. For example, "git lfs checkout --to foo --base" reports a panic. We address this issue by exiting with an error message if the number of additional arguments is not exactly one when --to is supplied along with accompanying file path and one of the "stage" options. This also covers the situation where the user supplies these options plus spurious additional arguments beyond a single Git LFS object path, since only a single object can be checked out at a given stage into the file specified with --to. (Note that this object path is not treated as a filepath pattern, unlike the normal "git lfs checkout" mode of operation.)
In a previous commit we added an error message to the "git lfs checkout" command which will be output in certain cases when the --to option is supplied and either zero or multiple paths are provided to be checked out. To validate the previous commit's change we expand our "checkout: conficts" test to check the relevant cases, and we also add some checks for other invalid options and arguments, such as when no "stage" option is given (--base, --ours, or --theirs), when too many "stage" options are set, or when paths to conflicting files are provided which do not correspond to Git LFS objects. Note that while our new checks will not catch a Go panic in particular, they would still cause the test to fail if our previous commit's change was not in place, because the expected new error message would not appear.
In a previous commit we revised the "git lfs checkout" command to handle certain improper argument error conditions more gracefully, including when too many paths to conficting files are provided along with the --to option (plus the necessary file for the --to option and one of the "stage" options: --base, --ours, or --theirs). We therefore also update the git-lfs-checkout(1) manual page to clarify that only a single path should be provided in this case, and that it must reference a conflicting Git LFS object file, not just any file with a merge conflict. We also add some missing formatting in the Synposis section to ensure the two alternative invocation formats are clearly separated, and we adjust the terminology of some of the user-provided arguments to align better with Git's manual and with other Git LFS manual pages. Specifically, we use <file> to indicate any file in the filesystem, <glob-pattern> to indicate a Git LFS glob pathspec pattern matched according to gitignore(5) rules, and <path> (in this case, <conflict-obj-path>) to indicate a path within the repository. And we expand on the details of each of these terms in the text of the manual page as well. Finally, we refine and expand the Examples section by adding an example use case for the --to option, and making all the examples follow the same formatting style.
--to but no path, and update checkout manual
bk2204
approved these changes
Dec 7, 2021
Member
bk2204
left a comment
There was a problem hiding this comment.
This looks great. Thanks for fixing the bug, improving the tests, and making a nice update to the documentation.
| Try to ensure that the working copy contains file content for Git LFS objects | ||
| for the current ref, if the object data is available. Does not download any | ||
| content, see git-lfs-fetch(1) for that. | ||
| content; see git-lfs-fetch(1) for that. |
Member
There was a problem hiding this comment.
I appreciate your changing this to use a semicolon.
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
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.
At present if the
git lfs checkoutcommand is invoked with the--tooption, a file path, and a "stage" option (one of--base,--ours, or--theirs), but without any object path, a Go panic is reported, because there is an attempt to reference the non-existent first element of theargsslice. For example,git lfs checkout --to foo --basereports a panic.We address this issue by exiting with an error message if the number of additional arguments is not exactly one when
--tois supplied along with accompanying file path and one of the "stage" options.This also covers the situation where the user supplies these options plus spurious additional arguments beyond a single Git LFS object path, since only a single object can be checked out at a given stage into the file specified with
--to. (Note thatthis object path is not treated as a filepath pattern, unlike the normal
git lfs checkoutmode of operation.)To validate this change we expand our
"checkout: conficts"test to check the relevant cases, and we also add some checks for other invalid options and arguments, such as when no "stage" option is given, when too many "stage" options are set, or when paths to conflicting files are provided which do not correspond to Git LFS objects.Note that while our new checks will not catch a Go panic in particular, they would still cause the test to fail if our previous commit's change was not in place, because the expected new error message would not appear.
To accord with our change we also update the
git-lfs-checkout(1)manual page to clarify that only a single path should be provided in this case, and that it must reference a conflicting Git LFS object file, not just any file with a merge conflict.We also add some missing formatting in the Synposis section to ensure the two alternative invocation formats are clearly separated, and we adjust the terminology of some of the user-provided arguments to align better with Git's manual and with other Git LFS manual pages.
Specifically, we use
<file>to indicate any file in the filesystem (e.g., as seen ingit log --output=<file>),<glob-pattern>to indicate a Git LFS glob pathspec pattern matched according togitignore(5)rules (similar to the usage ingit log --glob=<glob-pattern>, although we match files in the index rather than under.git/refs), and<path>(in this case,<conflict-obj-path>) to indicate a path within the repository. And we expand on the details of each of these terms in the text of the manual page as well.Finally, we refine and expand the Examples section by adding an example use case for the
--tooption, and making all the examples follow the same formatting style.