commands: show root help for -h#6172
Conversation
chrisd8088
left a comment
There was a problem hiding this comment.
Hey, thanks very much for this PR, and welcome to the Git LFS project!
I also really appreciate that you added a test for these changes! After a bit of reflection, though, I wonder if we would benefit even more from a new shell test rather than a Go test.
With a script like the one below, we could quickly validate a whole range of possible invocations, while also demonstrating that our mangen program has ingested our manual page source files.
May I therefore suggest that instead of a new commands/run_test.go file, we add a t/t-usage.sh file with the following contents?
(Note that we typically set these files to have their execute permissions enabled, e.g., by running chmod a+x t/t-usage.sh before git add t/t-usage.sh.)
#!/usr/bin/env bash
. "$(dirname "$0")/testlib.sh"
begin_test "usage: no command specified"
(
set -e
git lfs | grep 'git lfs <command> \[<args>\]'
git lfs -h | grep 'git lfs <command> \[<args>\]'
# Note that "git lfs --help" is handled by Git, not Git LFS.
git lfs --foo -h | grep 'git lfs <command> \[<args>\]'
git lfs --foo --help | grep 'git lfs <command> \[<args>\]'
git-lfs | grep 'git lfs <command> \[<args>\]'
git-lfs -h | grep 'git lfs <command> \[<args>\]'
git-lfs --help | grep 'git lfs <command> \[<args>\]'
git-lfs --foo -h | grep 'git lfs <command> \[<args>\]'
git-lfs --foo --help | grep 'git lfs <command> \[<args>\]'
)
end_testAs it happens, a new shell test file like this would be very convenient for my own next PR, so if you'd prefer to wait for me to finish that work first and get it merged, that's also completely fine.
Thank you again for fixing this issue! We really appreciate contributions like this to our project.
|
Thanks again very much for this PR! I've added the |
|
Alright. Lemme know how it goes.
…On Tue, Jan 13, 2026 at 9:05 AM Chris Darroch ***@***.***> wrote:
*chrisd8088* left a comment (git-lfs/git-lfs#6172)
<#6172 (comment)>
Thanks again very much for this PR!
I've added the t/t-usage.sh test script I proposed above to my own PR
#6188 <#6188>, and so once that's
merged, I think we can drop the commands/run_test.go file from this PR
and just add two lines to the new shell test, specifically these two lines:
git lfs -h | grep 'git lfs <command> \[<args>\]'
git-lfs -h | grep 'git lfs <command> \[<args>\]'
—
Reply to this email directly, view it on GitHub
<#6172 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AL3LX6ZVKQQILOJU6G7NPVT4GSRNFAVCNFSM6AAAAACPVJAHQGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTONBSGY3DSOBZHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Thanks, @osasisorae! I expect the issue in upstream Git which is causing parts of our CI suite to fail at the moment will be resolved next week, at which point I can merge my PR #6188 and then we can adjust this PR to make use of the new If you'd prefer me to make those adjustments, I'd be happy to do so; you'd just need to grant me permission to push to the branch in your repository. |
In a prior commit in this PR we updated the printHelp() function in our "commands" package to recognize the -h option when it is provided without any Git LFS command name, which should resolve the issue reported in git-lfs#6168. (Note that we already recognized the --help option when no Git LFS command name is provided.) In the same commit we also introduced a new "commands/run_test.go" source file with a single Go test function that verified the revised behaviour of the printHelp() function. Subsequently, in commit 926dc36 of PR git-lfs#6188, we added a new "t/t-usage.sh" shell test script with a single test. This test verifies that the "git lfs" and "git-lfs" commands respond to various conditions by returning the contents of our git-lfs(1) manual page. These conditions include the use of the --help option without any Git LFS command name, e.g., when a "git lfs --help" command is run. We can therefore now expand the "usage: no command specified" shell test so that it runs both the "git lfs -h" and "git-lfs -h" commands and confirms that the git-lfs(1) manual page is returned, as should be the case due to the changes from our prior commit in this PR. The shell test is able to exercise the "git-lfs" binary after it has been compiled with a complete set of manual pages in its ManPages map, since our "Makefile" ensures that our "mangen" program is compiled and executed to generate the ManPages map and store it into a "commands/mancontent_gen.go" file. The "git-lfs" binary is then compiled from sources which include this generated file. The Go language test we introduced earlier in this PR, on the other hand, has to handle the case that the ManPages map does not contain an appropriate entry for the git-lfs(1) manual page, as well as the case where an entry already exists. As a result, the Go language test is more complicated than the two lines we can now just add to our "usage: no command specified" shell test, while also not fully testing the "git-lfs" binary with a -h command-line option and instead just testing the printHelp() function directly. We therefore defer to our shell test to validate our changes to the printHelp() function, and remove the Go test and new source file we initially added.
|
Thanks! I was able to push one extra commit to this PR which just makes use of the new shell test we added in PR #6188 and drops the additional Go test file, since the shell test should be sufficient and effective. I'll let the CI suite run even though I know the Windows job will fail due to a regression in upstream Git which made its way into the latest Git for Windows pre-release candidate. We'll unfortunately have to wait for the next Git for Windows candidate to be tagged before that job will pass again, but once that's the case (and I've dealt with what are likely some test flakes as well), I can merge this PR. Thanks again for all your help, and welcome to the Git LFS project! |
Summary
-hthe same as--helpsogit lfs -hprints the root man page instead of “Sorry, no usage text…”printHelp("-h")to ensure the short flag keeps workingTesting
Fixes #6168