Skip to content

avoid panic on checkout with --to but no path, and update checkout manual#4766

Merged
chrisd8088 merged 3 commits intogit-lfs:mainfrom
chrisd8088:fixup-checkout-to-args
Dec 7, 2021
Merged

avoid panic on checkout with --to but no path, and update checkout manual#4766
chrisd8088 merged 3 commits intogit-lfs:mainfrom
chrisd8088:fixup-checkout-to-args

Conversation

@chrisd8088
Copy link
Member

@chrisd8088 chrisd8088 commented Dec 7, 2021

At present if the git lfs checkout command is invoked with the --to option, 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 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.)

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 in git log --output=<file>), <glob-pattern> to indicate a Git LFS glob pathspec pattern matched according to gitignore(5) rules (similar to the usage in git 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 --to option, and making all the examples follow the same formatting style.

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.
@chrisd8088 chrisd8088 requested a review from a team December 7, 2021 08:36
@chrisd8088 chrisd8088 changed the title avoid panic on checkout with --to but no path and update checkout manual avoid panic on checkout with --to but no path, and update checkout manual Dec 7, 2021
Copy link
Member

@bk2204 bk2204 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate your changing this to use a semicolon.

@chrisd8088 chrisd8088 merged commit b197e98 into git-lfs:main Dec 7, 2021
@chrisd8088 chrisd8088 deleted the fixup-checkout-to-args branch December 7, 2021 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants