Skip to content

Conversation

@chrisd8088
Copy link
Member

@chrisd8088 chrisd8088 commented Apr 25, 2022

In preparing a PR to address #4945 and adding tests to ensure that issue does not recur, a number of problems with our tests, test helpers, and the implementation of the git lfs prune command were exposed by the new tests and checks. These will be resolved in a series of forthcoming PRs.

As preparation, we first make a number of small adjustments to our test suite, adding additional checks to the "fsck detects invalid objects" test in t/t-fsck.sh, removing whitespace and revising comments for greater clarity, and changing the assert_pointer() and revise_pointer() test helper functions to be consistent with each other again and to apply a stricter match for file paths. We also remove one unused function from the Go codebase.

This PR may be most easily reviewed commit-by-commit, mostly because the individual commit descriptions provide detailed context on each change. The key changes are:

  1. The assert_pointer() and refute_pointer() test helper functions use grep to match specific lines of output from git ls-tree. However, while assert_pointer() matches with fixed patterns by using grep's -F option, refute_pointer() does not. This difference was introduced in commit fc421da of PR Provide an option to track to handle paths literally #3756, so we update refute_pointer() to match.

    In a subsequent PR we will also need to ensure that the file paths we provide as arguments to these functions (specifically, to assert_pointer()) do not match just any part of the paths output by git ls-tree, but only at the start of the path. That is, a.bin should match a.bin but not foo/a.bin. Because the output from git ls-tree is space-separated, we prepend a space to our fixed patterns so we match against the start of the file path.

    This has the limitation that if a test has a file named foo a.bin, and calls one of these functions with just the filename a.bin, it will incorrectly match the filename with a space. However, at present only one of our tests that calls either of these functions, specifically the "track: escaped glob pattern with spaces in .gitattributes" test in t/t-track.sh, has a filename with a space in it, and does not create any additional files which might be confused by that filename.

  2. In our "fsck detects invalid objects" test of the git lfs fsck command we currently only use files in the top-level directory for which we then corrupt or remove the corresponding Git LFS object files. In a subsequent PR we will add a parallel test which checks that the lfs.fetchexclude option is supported for this command, specifically by excluding files in a subdirectory. For the primary test, then, we want to demonstrate normal behaviour without path-based exclusions, so we add files in a subdirectory to our existing test and ensure their corruption is detected.

  3. The internal parseLogOutputToPointers() function was used only by the logPreviousSHAs() function and only until commit d8ca610 of PR Gitscanner callbacks master #1743 in 2016, so we remove it now.

The internal parseLogOutputToPointers() function was used only
by logPreviousSHAs() and only until commit
d8ca610 of PR git-lfs#1743 in 2016,
so we can remove it now.
Prior to making changes in the "git lfs prune" command's
implementation in subsequent PRs, we first remove a bit of
spurious whitespace, just to improve consistency between
similar functions.
@chrisd8088 chrisd8088 requested a review from a team as a code owner April 25, 2022 09:22
The assert_pointer() and refute_pointer() test helper functions
use grep to match specific lines of output from "git ls-tree".
However, while assert_pointer() matches with fixed patterns by using
grep's -F option, refute_pointer() does not.  This difference was
introduced in commit fc421da of
PR git-lfs#3756, so we update refute_pointer() here to match.

In a subsequent PR we will also need to ensure that the file paths
we provide as arguments to these functions (specifically, to
assert_pointer()) do not match just any part of the paths output
by "git ls-tree", but only at the start of the path.  That is,
"a.bin" should match "a.bin" but not "foo/a.bin".

Because the output from "git ls-tree" is space-separated, we
prepend a space to our fixed patterns so we match against the
start of the file path.  This has the limitation that if a test
has a file named "foo a.bin", and calls one of these functions with
just the filename "a.bin", it will incorrectly match the filename
with a space.

However, at present only one of our tests that calls either of these
functions, specifically the "track: escaped glob pattern with spaces
in .gitattributes" test in t/t-track.sh, has a filename with a
space in it, and does not create any additional files which might
be confused by that filename.
In our "fsck detects invalid objects" test of the "git lfs fsck"
command we currently only use files in the top-level directory
for which we then corrupt or remove the corresponding Git LFS
object files.

In a subsequent PR we will add a parallel test which checks that
the "lfs.fetchexclude" option is supported for this command,
specifically by excluding files in a subdirectory.  For the
primary test, then, we want to demonstrate normal behaviour
without path-based exclusions, so we add files in a subdirectory
to our existing test and ensure their corruption is detected.
In subsequent PRs we expect to expand and revise our tests of
the "git lfs prune" command, so we first remove some spurious
whitespace, just to improve consistency between the tests.
We revise some of the comments in the "prune keep unpushed"
test to clarify their meaning before we add additional checks
to this test.

We also drop an unused "git lfs prune --dry-run" invocation
since we are not testing its output and therefore it serves no
purpose in the test.
@chrisd8088 chrisd8088 force-pushed the tidy-and-expand-tests branch from 9814d7e to d85d28d Compare April 25, 2022 09:38
@chrisd8088 chrisd8088 merged commit efc4bcb into git-lfs:main Apr 25, 2022
@chrisd8088 chrisd8088 deleted the tidy-and-expand-tests branch April 25, 2022 15:31
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