Skip to content

Conversation

@chrisd8088
Copy link
Member

Following from the work in PR #5882 to fully enable all the tests in our t/t-clone.sh test script, this PR further refines the clone ClientCert and clone ClientCert with homedir certs tests so they both perform the same sets of checks, including a reworked version of the loop in the clone ClientCert test which clarifies when the encrypted version of the TLS/SSL certificate's private key is in use.

In particular, the clone ClientCert with homedir certs test does not, at the moment, actually test the git lfs clone command, unlike all the other tests in t/t-clone.sh test script, and it also does not use the encrypted private key file at all, although we create some record files for our git-credential-lfstest helper program as if this were the case.

We also make minor formatting adjustments and simplifications to our t/t-clone.sh test script, and update an incorrect comment in our t/testenv.sh shell 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 -z test operator (when appropriate) or a simple grep(1) command in an AND list with an exit 1 statement, 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 LocalMediaDir line from git lfs env command's output in various functions in our t/testhelpers.sh shell library.

Lastly, we make sure our lfstest-gitserver program closes all of its instances of the Server structure from the net/http/httptest Go 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.

chrisd8088 and others added 11 commits November 13, 2024 20:27
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".
@chrisd8088 chrisd8088 requested a review from a team as a code owner November 18, 2024 06:11
Copy link
Member

@larsxschneider larsxschneider left a 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
Copy link
Member

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 (!)?

Copy link
Member Author

@chrisd8088 chrisd8088 Nov 18, 2024

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.

Copy link
Member Author

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

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?

Copy link
Member Author

@chrisd8088 chrisd8088 Nov 18, 2024

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.
@chrisd8088 chrisd8088 merged commit 2ef4108 into git-lfs:main Nov 19, 2024
10 checks passed
@chrisd8088 chrisd8088 deleted the test-grep-clone-fixups branch November 19, 2024 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants