-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix improper negated test expressions and refine TLS client certificate tests #5914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
In commit 5e654f2 in PR git-lfs#565 a pair of test assertion functions were added to the forerunner of our current t/testhelpers.sh shell library. These assert_local_object() and refute_local_object() functions check for the presence or absence of a file in the object cache maintained by the Git LFS client in a local repository. To perform these checks, the functions capture the output of the "git lfs env" command and parse the contents of the LocalMediaDir line, which reports the full path to the Git LFS object cache location. To retrieve the path, the functions ignore the first 14 characters of the line, as that corresponds to the length of the LocalMediaDir field name (13 characters) plus one character in order to account for the equals sign which follows the field name. Later PRs have added three other assertion functions that follow the same design. The delete_local_object() function was added in commit 97434fe of PR git-lfs#742 to help test the "git lfs fetch" command's --prune option, the corrupt_local_object() function was added in commit 4b0f50e of PR git-lfs#2082 to help test the detection of corrupted local objects during push operations, and most recently, the assert_remote_object() function was added in commit 9bae8eb of PR git-lfs#5905 to improve our tests of the SSH object transfer protocol for Git LFS. All of these functions retrieve the object cache location by ignoring the first 14 characters from the LocalMediaDir line in the output of the "git lfs env" command. However, the refute_local_object() function contains a hint of an alternative approach to parsing this line's data. A local "regex" variable is defined in the refute_local_object() function, which matches the LocalMediaDir field name and equals sign and captures the subsequent object cache path value. Although this "regex" variable was included when the function was first introduced, it has never been used, and does not appear in any of the other similar functions. While reviewing PR git-lfs#5905, larsxschneider suggested an even simpler option than using a regular expression to extract the object cache path from the LocalMediaDir line. Rather than asking the Bash shell to start its parameter expansion at a fixed offset of 14 characters into the string, we can define a pattern which matches the leading LocalMediaDir field name and equals sign and specify that the shell should remove that portion of the string during parameter expansion. See also the discussion in this review comment from PR git-lfs#5905: git-lfs#5905 (comment) In addition to these changes, we can remove the definition of the "regex" variable from the refute_local_object() function, as it remains unused. Co-authored-by: Lars Schneider <[email protected]>
In commit a5d20de of PR git-lfs#5282 we revised a number of our test scripts so as to avoid the use of the "!" shell word in cases where we want to confirm that a command fails and returns a non-zero exit code. We made this change because we had a variety of instances where we had checks of this form which would in practice always pass, regardless of the exit code of their commands. (This behaviour is a consequence of our use of "set -e" in all our test scripts. The Git project's shell test scripts do not use this option, but ours do; it means that the shell will exit immediately if a normal command returns a non-zero exit code. However, according to the POSIX shell specification, when the "set -e" option is enabled and a command's exit code is inverted with "!", the shell will not exit immediately even if the inverted value is non-zero.) Commit a5d20de addressed all of the instances in our test scripts where we relied on an incorrect usage of the "!" shell word. However, we have another similar form of incorrect usage in a number of our test scripts where we rely on the "!" test operator to confirm that a command's exit code is non-zero (i.e., in conditions where we expect the command to fail). In particular, we often use tests of the following form, with the expectation that "foo" should not appear in the log file, and if it does the test should fail: [ ! $(grep "foo" bar.log) ] These checks always succeed, but for the wrong reason. When the grep(1) command does not find the given string in its input stream or the specified file, it prints nothing to its standard output and returns a non-zero exit code. Because we are evaluating a command substitution inside the test expression, and the output of that substitution is the standard output of the command, which is empty, the test devolves to an expression of the form: [ ! ] The Bash shell treats the absence of an expression as a false value, which is then inverted by the "!" operator, so the test passes, but not because has confirmed that the command's exit code was non-zero. Some of these improper test expressions could still catch regressions in the Git LFS client's behaviour, though. For instance, in the example above, if "foo" was found in the log file, the test expression that would be evaluated would have the following form, where all the log lines which matched the string "foo" would appear after the "!" operator: [ ! ... foo ... ] This might form an invalid shell expression, or one with too many arguments (thus causing a "[: too many arguments" error), in which case the test would fail. We can replace all the instances where we rely on these improper test expressions using the same approach we used in commit a5d20de. We simply perform the commands from what was enclosed in a command substitution as the first command pipeline in an AND list, where "exit 1" is the second and final command in the list. If the first command pipeline returns a non-zero exit code, as we expect it to, the "exit 1" is not executed. However, if the pipeline should catch a regression and unexpectedly succeed, "exit 1" will be executed, causing our test to fail. Note that we do not rely on the "set -e" option here, because it ignores the return values of command lists entirely. In cases where our revised check is the last line of one of our tests, we add a "true" statement, because otherwise the test will fail since the last evaluated command would be the first one in the list, i.e., the grep(1) command that we expect will find no matches in its input. In our t/t-migrate-import.sh test script we also have some improper test expresssions of the following form, where we expect that the output of the "git cat-file" command should be empty because the ".gitattributes" file at the given Git ref is blank: [ ! $(git cat-file -p "ref:.gitattributes") ] As with the improper tests of the output of grep(1) commands, these tests succeed but for the wrong reason, because they also devolve to expressions of the form: [ ! ] To repair these checks, we simply replace the "!" test operator with the "-z" one, as we just want to confirm that the output of the command substitution is a zero-length string.
In commit 365d615 of PR git-lfs#1478 a pair of tests in what is now our t/t-clone.sh test script were enhanced to check that the "git lfs clone" command did not create a directory named "lfs" within the top level of a cloned repository's working tree, as the "lfs" directory should instead be created within the ".git" directory. This check has since been replicated in a number of other tests in the t/t-clone.sh script, but not all of them, so we add this check in the relevant sections of the tests where it is not yet performed. We also adjust one check which dates from commit 365d615 to use the same -e shell test as all the other checks, rather than the -f test. These changes are not especially important, but in conjunction with the revisions we will make in other commits in this PR, along with those we made recently in PRs git-lfs#5882 and git-lfs#5906, they will help synchronize the tests in the t/t-clone.sh script so they are more consistent with each other and complete.
As we are making a number of changes to the tests in our t/t-clone.sh test script in other commits in this PR, with the goal of increasing the level of commonality between our tests, we take the opportunity to indent all the code contained between "pushd" and "popd" commands. This style of formatting is already used in a number of the tests, but not all of them, so we make them all consistent now, and we also remove one line of excess whitepace.
In commit c54c9da of PR git-lfs#5657 we added support for the use of client TLS/SSL certificate and key files in a user's home directory, specifically, when their paths are prefixed with a tilde character. This change aligned Git LFS's support of the "http.<url>.sslCert" and "http.<url>.sslKey" Git configuration values with that of Git itself. As part of this change, the "clone ClientCert with homedir certs" test was added to our t/t-clone.sh test script. This test sets the "http.<url>.sslCert" and "http.<url>.sslKey" Git configuration options to paths beginning with a tilde, in order to validate our support for such paths. As well, the following test, the "clone with flags" test, was updated in the same commit to include a pair of commands which reset the "http.<url>.sslCert" and "http.<url>.sslKey" Git configuration options to their usual values; that is, to the values set by the setup() shell function run at the start of each test file. This makes sense, because the goal was to ensure that in all cases, the tests following the new "clone ClientCert with homedir certs" test had the expected values for these configuration options. However, resetting these values is not actually required, because every test starts with the begin_test() shell function, which already copies a fresh version of the global Git configuration file into the directory used as the value of the HOME environment variable in each test. So we can simply remove the two "git config" calls at the start of the "clone with flags" test, as the values they set are always the same as the ones provided by our default test environment.
In commit b9602f3 of PR git-lfs#5616 we resolved a problem which caused flaky test results in our CI jobs by ensuring that the t/t-credential.sh test script used a unique directory for the credential record files we provide to our git-credential-lfstest helper program. This change ensured that when the "clone (HTTP server/proxy require cookies)" test in our t/t-clone.sh script creates a record file named "localhost" in the common directory specified by the CREDSDIR environment variable, specifically a copy of the "127.0.0.1" file created by the setup_creds() shell library function, this does not conflict with the "credentials from netrc" test and other related tests in the t/t-credential.sh test script, which depend on there being no credential records associated with the "localhost" hostname. However, we already made a previous attempt to resolve this problem in commits bb87944 and bcb9eb3 of PR git-lfs#3825, but that change assumes that our test scripts always run sequentially rather than in parallel. In PR git-lfs#3825 the "clone (HTTP server/proxy require cookies)" test was added, and at the end of the test, it removes the "localhost" credential record file it creates at the beginning of the test. Of course, if the test ever fails, then it leaves the "localhost" record file in place, so even if our test scripts always ran sequentially there would still be a potential conflict with the tests in the t/t-credential.sh script. We can more comprehensively ensure that these two test scripts do not conflict again in the future by using the same technique applied in commit b9602f3 to the t/t-credential.sh script in the t/t-clone.sh script as well. First, we add a call to setup_creds() at the start of the t/t-clone.sh script after setting the CREDSDIR environment variable to a path unique to the t/t-clone.sh script. This ensures that a separate copy of the default credential record file for the 127.0.0.1 hostname is available for the exclusive use of the tests in the script. Next, we move the creation of the credential record files associated with the TLS/SSL client certificate used in the "clone ClientCert" test from the generic setup_creds() function into the test itself. We would otherwise need to define the "certpath" and "keypath" variables, as their values are interpolated by the function into the names of these credential record files. The TLS/SSL client certificate and key files are generated by our lfstest-gitserver utility when it is first executed. Their locations are defined by the LFSTEST_CLIENT_* environment variables passed by the setup() function in our t/testhelpers.sh shell library to the lfstest-count-tests utility, which then runs the lfstest-gitserver program. The values of these environment variables are set from the LFS_CLIENT_CERT_FILE and LFS_CLIENT_KEY_FILE* variables defined by our t/testenv.sh script. We now use these variables in the "clone ClientCert" test when we call the write_creds_file() function directly, instead of allowing the setup_creds() function to perform those calls. We could define the "certpath" and "keypath" variables (using the LFS_CLIENT_CERT_FILE and LFS_CLIENT_KEY_FILE_ENCRYPTED variables) prior to calling setup_creds() at the start of the t/t-clone.sh script. However, the "clone ClientCert" test is the only one which actually makes use of these credential record files, as this is the only test which actively checks the use of an encrypted TLS/SSL client certificate, and so is the only place where we need to create these record files. Further, we can also move into the test the "git config" commands that set the "http.<url>.sslCert" and "http.<url>.sslKey" Git configuration options with the locations of the TLS/SSL client certificate files, and we do the same for the "create lock with server using client cert" test in the t/t-lock.sh test script. Previously, we set these configuration options in the setup() function in our t/testhelpers.sh shell library, so they were configured for all tests in all test scripts, although only the "clone ClientCert" test and the "create lock with server using client cert" test make use of them. Finally, the "clone (HTTP server/proxy require cookies)" test no longer needs to attempt to delete the credential record file for the "localhost" hostname after the test is complete, so we can simply remove that code. Note that the "clone ClientCert" test was first introduced in commit daba49a of PR git-lfs#1893, at which time the "git config" commands to set the "http.<url>.sslCert" and "http.<url>.sslKey" options were added to the setup() function, and the LFS_CLIENT_CERT_FILE and LFS_CLIENT_KEY_FILE variables were defined in what was then the test/testenv.sh script. Later, in commit 52f94c2 of PR git-lfs#3270, the LFS_CLIENT_KEY_FILE_ENCRYPTED variable was added along with support in the lfstest-gitserver program to generate an encrypted certificate key file. In another commit in PR git-lfs#3270, commit 706beca, the "clone ClientCert" test was then expanded to validate use of the encrypted key file with the "git lfs clone" command. (Note too that the following test, the "clone ClientCert with homedir certs" test, appears to also depend on credential record files for the TLS/SSL client certificate files it creates in the dedicated home directory used by our tests. However, because this test does not set the "http.sslCertPasswordProtected" Git configuration option or the GIT_SSL_CERT_PASSWORD_PROTECTED environment variable, Git does not attempt to retrieve a passphrase for the certificate files, and so the associated credential record files are never actually used. We will address this issue in a subsequent commit in this PR.)
The "cloneSSL" test in what is now our t/t-clone.sh test script was added in commits 4c64e82 and 8f91a1b of PR git-lfs#1067, and since then has performed the same basic set of checks. A bare test repository is first cloned using a TLS/SSL connection. Example Git LFS objects are then committed to the repository, and pushed, after which a "git lfs clone" command is run with a specified, non-default directory name argument so the repository is cloned into a new working tree under that local directory. The test then checks that the Git LFS objects have also been populated into the new working tree. Finally, a regular "git clone" command is run (without a directory name argument), again with a TLS/SSL connection, which should run the Git LFS "smudge" filter and fail if that does not also succeed. This test design was later used as a template for the "clone ClientCert" test when it was added in commit daba49a of PR git-lfs#1893; like the "cloneSSL" test, it performed the same series of checks, just using a client TLS/SSL certificate and key when cloning from the remote. We then added support for encrypted TLS/SSL certificate keys in commit 706beca of PR git-lfs#3270, and at this time, the "clone ClientCert" was updated to repeat the "git lfs clone" step and checks using first an unencrypted key and then an encrypted key. However, the "git clone" step was not moved into the same loop with the "git lfs clone" command, so it is only run using the unencrypted key. For this reason, the "http.<url>.sslKey" Git configuration option is reset after the end of the loop to contain the unencrypted key file's path. As we expect to make several other adjustments to this test in subsequent commits in this PR, we first bring the "git clone" command into the same loop with the "git lfs clone" command, which means we no longer need to reset the "http.<url>.sslKey" configuration option, as we want to use the same value for both cloning commands. This change also ensures we perform all the same checks with both types of TLS/SSL certificate keys.
We added support for encrypted TLS/SSL certificate keys in PR git-lfs#3270, and as part of that change, in commit 706beca we updated the "clone ClientCert" test in our t/t-clone.sh test script to perform "git lfs clone" operations twice, once with an unencrypted TLS/SSL certificate key and once with an encrypted one. Our test needs to ensure that Git and Git LFS do not generate user prompts to retrieve the passphrase to the encrypted key, as this would make impossible to run the test in our CI jobs, and so we set the GIT_SSL_CERT_PASSWORD_PROTECTED environment variable to "1" at the start of the main loop in our test. However, we do this for both passes through the loop, which means we set the GIT_SSL_CERT_PASSWORD_PROTECTED environment variable when testing with the unencrypted TLS/SSL certificate key as well as with the encrypted one. We only need to set it in the latter case, though. To clarify the intent of the test's loops, we refactor the test to now iterate through a list of two values, "false" and "true". When the value is "true", we set the "http.sslCertPasswordProtected" Git configuration option to "true", and reset the "http.<url>.sslKey" option with the path from the LFS_CLIENT_KEY_FILE_ENCRYPTED variable. By refactoring the test's main loop in this way we can avoid setting Git environment variables or configuration options which do not apply when neither the certificate file nor the private key file are encrypted. In a prior commit this PR we moved the "git config" commands which set the "http.<url>.sslCert" and "http.<url>.sslKey" configuration options specific to the "clone ClientCert" test out of the common setup() function in our t/testhelpers.sh test library and into the test itself. To align with those initial "git config" commands and with the command we use on the second pass through the test's loop to reset the value of the "http.<url>.sslKey" option, we prefer to set the "http.sslCertPasswordProtected" Git configuration option instead of the GIT_SSL_CERT_PASSWORD_PROTECTED environment variable. This option has the same effect on Git's behaviour as the environment variable, so there is no change to the test conditions. We also take the opportunity to fix a small typo in the directory name argument passed to the "git lfs clone" command, and we avoid the need to delete the working tree created by the previous iteration of the loop by including our loop index value in the directory name. Finally, we add a detailed set of code comments to explain exactly why we have to create two credential record files for use by our git-credential-lfstest helper program, associated with the paths of both the TLS/SSL certificate file and the private key file, when only the latter is encrypted. This is due to the fact that Git and Git LFS each make calls to the credential helper to retrieve the private key's passphrase, but do so for different reasons, and use different "path" values in the input they send to the "git credential fill" command.
In commit c54c9da of PR git-lfs#5657 we added support for the use of client TLS/SSL certificate and key files in a user's home directory, specifically, when their paths are prefixed with a tilde character. This change aligned the Git LFS client's support of the "http.<url>.sslCert" and "http.<url>.sslKey" Git configuration options with that of Git itself. As part of this change, the "clone ClientCert with homedir certs" test was added to our t/t-clone.sh test script. This test sets the "http.<url>.sslCert" and "http.<url>.sslKey" configuration options to paths beginning with a tilde, in order to validate our support for such paths. As well, the setup() and setup_creds() functions in our t/testhelpers.sh shell library were revised to create credential record files for our git-credential-lfstest helper program, corresponding to the paths of the certificate file and private key file which the "clone ClientCert with homedir certs" test copies into the location specified by the HOME environment variable. That location is set by our t/testlib.sh script to a directory named "home" under the temporary directory path stored in our TRASHDIR variable, where all our tests generate their various artifacts. In practice, however, because the "clone ClientCert with homedir certs" test never sets the "http.sslCertPasswordProtected" Git configuration option or the GIT_SSL_CERT_PASSWORD_PROTECTED environment variable, and because it never sets the "http.<url>.sslKey" option to point to an encrypted version of the private key file, neither Git nor Git LFS ever invoke the "git credential fill" command. As a result, our credential helper program does not run, and so the record files created by the setup_creds() function for use by this test are never opened. Moreover, the "clone ClientCert with homedir certs" test never actually runs the "git lfs clone" command, unlike all the other tests in the t/t-clone.sh test script. In previous commits in this PR we have refactored the "clone ClientCert" test, which precedes the "clone ClientCert with homedir certs" test, so that it more clearly distinguishes between the use of encrypted and unencrypted private key files, and explains the conditions under which Git and Git LFS will retrieve a passphrase for an encrypted certificate or private key file. We therefore copy the main loop from the "clone ClientCert" test into the "clone ClientCert with homedir certs" test so that the latter now runs the "git lfs clone" command twice, once using the unencrypted version of the private key file, and once using the encrypted version. Like the "clone ClientCert" test, the "clone ClientCert with homedir certs" test validates the clone create by the "git lfs clone" command on each pass through its loop, and also runs a regular "git clone" command afterward to ensure it succeeds as well. The test now needs to copy the encrypted private key file specified by the LFS_CLIENT_KEY_FILE_ENCRYPTED variable into the test home directory, since that location is then used as the value of the "http.<url>.sslKey" configuration option during the second iteration of the test loop. We also slightly refactor the sequence of other initial steps performed by the "clone ClientCert with homedir certs" test to more closely align with the "clone ClientCert" test, such as by moving the definition of the "reponame" variable later in the test. We reorder the sequence in which we copy the certificate and key files and set the "http.<url>.sslCert" and "http.<url>.sslKey" configuration options so they follow the same sequence used in other tests and scripts. As well, we use quotation marks around the configuration option key names, which again aligns with our general (but not consistent) practice. In a prior commit in this PR we added a detailed set of code comments to the "clone ClientCert" test, which we replicate in the "clone ClientCert with homedir certs" test. These comments help explain the conditions under which Git and Git LFS query or do not query the credential helper via a "git credential fill" command, and why we need two record files for our credential helper, both with the same passphrase but each associated with a different file path. Finally, because the test now establishes the record files used by the git-credential-lfstest helper, akin to how the "clone ClientCert" test does also, there is no need to create these record files in the common setup_creds() function in the t/testhelpers.sh library. Thus we can eliminate the calls to the write_creds_file() function for these files from the setup_creds() function, and also remove the definition of the "homecertpath" and "homekeypath" variables from the library as well. Note that when we call the write_creds_file() function to create the credential record files we need to use paths which align with what our git-credential-lfstest helper will construct from the values it receives in the "path" fields of its input. On Windows these "path" fields will contain Windows-style paths with a leading volume identifier like "D:". As discussed in commit bc11a31 of PR git-lfs#5882, our git-credential-lfstest helper converts such paths so they begin with a lowercase letter preceded by a slash character, like "/d", in order to match the paths found in the variables used to create the filenames of the credential record files in our "clone ClientCert" test. In our CI jobs, which run in the Git Bash environment, which uses the MSYS2 environment, these LFS_CLIENT_CERT_FILE and LFS_CLIENT_KEY_FILE_ENCRYPTED variables contain paths of the "mixed" Cygwin form, so they have Unix-style slash characters but start with a leading volume identifier segment such as "/d". In the "clone ClientCert with homedir certs" test, because we do not use these variables to construct the filenames of the credential record files, but the HOME environment variable instead, we have to perform the same path translation using the "cygwin" program, and then converting its output so volume identifiers like "D:" are converted into the "/d" form. This ensures the record files we create have the same names as the git-credential-lfstest helper will construct. Note, too, that because the colon character is disallowed in Windows filenames (other than in a volume identifier), we can not simply use the "D:" form of the paths when constructing the record file names, so this technique resolves that problem as well.
We added support for encrypted TLS/SSL certificate keys in PR git-lfs#3270, and as part of that change, in commit 52f94c2, we revised our lfstest-gitserver test utility program to create an encrypted private key file of the TLS/SSL client certificate it generates. We advise the lfstest-gitserver program where to write the encrypted key file using an LFSTEST_CLIENT_KEY_ENCRYPTED environment variable, which we populate in turn from an LFS_CLIENT_KEY_FILE_ENCRYPTED variable defined in our t/testenv.sh shell script, both of which were added in the same commit. However, the comment which describes the LFS_CLIENT_KEY_FILE_ENCRYPTED variable is identical to the comment for the LFS_CLIENT_KEY_FILE variable, so we update the one for the LFS_CLIENT_KEY_FILE_ENCRYPTED variable to clarify its intent.
The lfstest-gitserver utility program used by our test suite to simulate a full Git LFS server starts a multiple HTTP server instances using the implementation provided by the Go language's "net/http/httptest" package. However, these server instances are never cleanly shut down, and while this makes no difference to the success of our test suite, the Go documentation advises that we should call the Close() method for each Server structure, so we do that now after the primary server receives a request to its "/shutdown" endpoint. We also adjust the message logged by the program upon shutdown so it is no longer prefixed with "init" but rather "close".
larsxschneider
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR. I commented with a few minor questions/suggestions but the PR would still be ready to go.
|
|
||
| [ ! $(echo "$feature_attrs" | grep -q "*.md !text !filter !merge !diff") ] | ||
| [ ! $(echo "$feature_attrs" | grep -q "*.txt !text !filter !merge !diff") ] | ||
| echo "$feature_attrs" | grep -q "*.md !text !filter !merge !diff" && exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we single quote the grep argument to avoid possible wildcard expansion (*) or history expansion (!)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're OK here, although I agree that it would be nice to be more thorough in our use of single quotes in our shell tests.
Bash (which is the shell we require for our test scripts) shouldn't perform wildcard expansion inside double quotes, I believe, according to the manual page's section on quoting:
Enclosing characters in double quotes preserves the literal value of all characters within the quotes, with the exception of
$,`,\, and, when history expansion is enabled,!.
That aligns with my own results from a quick test:
$ touch foo.jpg
$ echo '*.jpg' >bar.txt
$ echo 'foo.jpg' >>bar.txt
$ cat bar.txt
*.jpg
foo.jpg
$ grep *.jpg bar.txt
foo.jpg
$ grep "*.jpg" bar.txt
*.jpg
As a separate issue, we rely in these grep(1) checks for patterns like *.md on the fact that * loses its meaning as a regular expression wildcard because it happens to be the first character in the pattern. That's a little obscure, but correct.
However, the . in *.md will match any character, not just a literal ., so that's something which should eventually be fixed. There are a lot of places where we fail to escape * and . and so forth, though, such as this example in t/track.sh, which would match if the file contained the string XYZjpg" already supported or Zjpg" already supported but not jpg" already supported, because the "* matches zero or more " characters, and the . matches any character, so we just need anything in front of the jpg part to match. Fixing all of those and ensuring they are correct is a big job, and not one I'm ready to tackle in this PR, though.
As for history wildcard expansion, I think we're OK there too because our test scripts start their own shells, and so are not interactive, and thus history expansion is disabled by default, according to the documentation.
Non-interactive shells do not perform history expansion by default.
Running echo $- in one of our test scripts confirms that it outputs ehxB, with no H option for history expansion. We set the x option in our begin_test() function, and most tests start by setting the e option, and B (for brace expansion) and h (to remember the location of commands after they are executed) are enabled by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened #5915 to track the problems in our tests' use of regular expression meta-characters.
| # from the "http.<url>.sslCert" option. Note that this path refers | ||
| # to our unencrypted certificate file; Git does not use the path to | ||
| # the encrypted private key file from the "http.<url>.sslKey" option | ||
| # in its query to the credential helper. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that a bug in Git? Why does Git query for the cert file and not the key file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's behaviour that stems from this line where the certificate file's path, not the key file's path, is sent to the credential helper, and it's clearly documented, because the certificate file may be encrypted, since those PEM files often contain both the certificate and the private key. From git/git@30dd916 there's this explanation, as well:
The user is always prompted, even if the certificate is not encrypted. This should be fine; unencrypted certificates are rare and a security risk anyway.
In our lfstest-gitserver test utility, we only encrypt the private key, and the client certificate file we create does not include it, so there's no particular need to encrypt that file.
To your point, the Git LFS client should arguably behave more like Git, and pass credential helper the path to the certificate file, not the key file. At the moment, we simply expect to be able to read the client certificate file, but check if the private key file was encrypted, and if it was, call a function and pass the key file's path to it, so it uses that path in the call to the credential helper. Changing that is outside the scope of this PR, but it's worth noting in case we want to adjust it in the future.
I think having the extra comments in these tests is a good step in that direction, since it makes it clear what's going on and the differences between Git and Git LFS in this regard.
Because full support of the "filter" attribute and protocol were only introduced with Git version 2.11.0, we require at least that version of Git for our t/t-filter-process.sh test script. This attribute and the associated protocol were developed in part so as to make the Git LFS client's integration with Git more efficient. The original version of the t/t-filter-process.sh test script predated the release of Git v2.11.0, so when it was introduced in commit d1dca3e of PR git-lfs#1617, it included a workaround technique for detecting whether the version of Git in use did or did not support the "filter" attribute yet. This workaround was preceded by a comment describing how it worked. In commit 54ddc13 of PR git-lfs#1699 the workaround was removed and replaced with a call to the ensure_git_version_isnt() function from our t/testhelpers.sh shell library, since Git version 2.11.0 had been released and so we could perform a simpler version check to confirm whether Git supported the "filter" attribute. However, the comment describing the original workaround technique for detecting support for the "filter" attribute was left in place, so we update it now.
In previous commits in this PR we have refactored a number of the initial tests in our t/t-clone.sh script so they are more consistent with each other and perform a more complete set of checks of the "git lfs clone" command when it is used with HTTP and HTTPS remote URLs and with TLS/SSL client certificates. All four of these tests run one or more "git lfs clone" commands. After each, they check the logs from the command, and then verify that a local Git repository has been created with the expected Git LFS hooks installed and with files in the working tree populated from the contents of Git LFS objects that have been downloaded. Three of these four tests, other than the very first "clone" test, also follow a pattern added in commits 4c64e82 and 8f91a1b of PR git-lfs#1067, in which they run a "git clone" command after the "git lfs clone" command. This test design was introduced first for the "cloneSSL" test, and then replicated in the "clone ClientCert" test when that was added in commit daba49a of PR git-lfs#1893. In prior commits in this PR we have now replicated it again into the "clone ClientCert with homedir certs" test, and have also ensured that it runs twice in both that test and the "clone ClientCert" test, once with an unencrypted TLS/SSL private key file and once with an encrypted one. However, although these three tests run a "git clone" command, they simply check that it does not exit with a non-zero error code (because we use the "set -e" option in our tests, so if the command failed it would cause the tests to fail). This is less than ideal, given that the "git lfs clone" command is now deprecated, and almost all Git LFS users will use the regular "git clone" command instead. As suggested by larsxschneider during the review of this PR, we can improve all four of these tests by adding checks following each "git clone" command similar to those we perform after the "git lfs clone" commands. We can also add a "git clone" command to the end of the initial "clone" test so that it now follows the same pattern as the other three tests. In each of these tests, we now confirm that the "git clone" command exits with a non-zero value, but also that it outputs a "Cloning into" message. We then check that Git LFS hooks were installed in the cloned local repository, that the expected number of Git LFS objects were downloaded, and that files in the working tree have been populated with the contents of those objects.
Following from the work in PR #5882 to fully enable all the tests in our
t/t-clone.shtest script, this PR further refines theclone ClientCertandclone ClientCert with homedir certstests so they both perform the same sets of checks, including a reworked version of the loop in theclone ClientCerttest which clarifies when the encrypted version of the TLS/SSL certificate's private key is in use.In particular, the
clone ClientCert with homedir certstest does not, at the moment, actually test thegit lfs clonecommand, unlike all the other tests int/t-clone.shtest script, and it also does not use the encrypted private key file at all, although we create some record files for ourgit-credential-lfstesthelper program as if this were the case.We also make minor formatting adjustments and simplifications to our
t/t-clone.shtest script, and update an incorrect comment in ourt/testenv.shshell library.In addition to these changes, we fix a number of instances throughout our test suite where we use incorrectly negated test expressions of the form
[ ! $(grep "foo" bar.log) ]to try to check that a given log file does not contain a specific string. These checks succeed at present, but for the wrong reason. When the given string is not found in the log file, the command substitution returns an empty value, so the test devolves to just[ ! ], and the shell treats the lack of a test expression as a false value, which it then inverts due to the!operator, and so the test always passes. We replace these incorrectly formed checks using either a-ztest operator (when appropriate) or a simplegrep(1)command in anANDlist with anexit 1statement, as we already did in PR #5282 to deal with incorrect uses of the!shell word.Prior to these corrections, we adopt a suggestion made during review of PR #5905 to improve the readability of the commands used to parse the
LocalMediaDirline fromgit lfs envcommand's output in various functions in ourt/testhelpers.shshell library.Lastly, we make sure our
lfstest-gitserverprogram closes all of its instances of theServerstructure from thenet/http/httptestGo package before it exits. This has no effect on our tests, but is idiomatic and recommended by the Go documentation.This PR will be most easily reviewed on a commit-by-commit basis, with whitespace changes ignored.