potentially invoke lfs for 'git update-index' #2647
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Oct 4, 2017
test/test-clone.sh
Outdated
| @@ -119,6 +123,7 @@ begin_test "cloneSSL" | |||
|
|
|||
| newclonedir="testcloneSSL1" | |||
| git lfs clone "$SSLGITSERVER/$reponame" "$newclonedir" 2>&1 | tee lfsclone.log | |||
| #assert_clean_status | |||
There was a problem hiding this comment.
Is this commented intentionally?
| @@ -459,7 +459,7 @@ func DefaultRemote() (string, error) { | |||
| } | |||
|
|
|||
| func UpdateIndexFromStdin() *subprocess.Cmd { | |||
| return gitNoLFS("update-index", "-q", "--refresh", "--stdin") | |||
| return git("update-index", "-q", "--refresh", "--stdin") | |||
There was a problem hiding this comment.
OK, here is my understanding:
We hit this code path only if we manually resolve the Git LFS pointers. Manually here means: we don't use the Git clean/smudge mechanism. This happens in the git lfs clone ... and git lfs pull calls. In these calls the following happens:
- Git checks out all files without running the Git LFS filters. We have the Git LFS pointer files checked out as a result.
- Git LFS iterates over all pointer files and replaces them with the actual content
- Git LFS asks Git to re-index the changed content (this happens with the
update-indexcall above). During the re-index Git needs tocleanthe real content to calculate the pointer file and compare that against the version stored in the repository. This waygit statuscan tell us if the file was changed or not.
--> I think it makes sense to run Git with the clean filter here.
larsxschneider
approved these changes
Oct 5, 2017
|
Thanks for the review 🤘 |
|
Your welcome! Do you know when 2.3.3 will be released with the fix? |
|
I have some stuff this morning, but I could release it this afternoon. Though, I don't think a friday afternoon release is a good idea, so I'm leaning towards Monday. What do you think? |
|
I trust your judgement here. I agree with the Friday argument. However, this is a pretty nasty bug, too. |
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 10, 2024
In commit caaa0f9 of PR git-lfs#2647 and number of tests of the "git lfs clone" and "git lfs pull" commands were enhanced so as check that after those commands are invoked, a "git status" command returns a "working tree clean" message. To perform these checks, a call to our assert_clean_status() shell function was added to the tests. In the case of the "cloneSSL" test, an assert_clean_status() call was added, but left commented out; it was then uncommented in commit e0eede1 of the same PR. Unfortunately, the call is made when the current working directory has not yet been changed to that of the newly cloned repository's working tree, and so will fail as it stands now. However, as described in git-lfs#5658, the "cloneSSL" test and the "clone ClientCert" tests in our t/t-clone.sh test script do not actually run to completion, as a consequence of an improper check of the TRAVIS variable (which is no longer used since we migrated our test suite to GitHub Actions in PR git-lfs#3808). This bug was already in place at the time of PR git-lfs#2647, and so the assertions are never actually performed in these tests. We expect to address this problem in a subsequent commit in this PR, and when we do so, the test "cloneSSL" test will fail because the assert_clean_status() call is made without having changed the current working directory to that of the cloned repository's working tree. Therefore we move the assertion to the end of the block of checks performed after the "pushd" shell command is used to change the directory to that of the new clone's working tree. We also take the opportunity to add the assert_clean_status() calls to a number of other tests in the t/t-clone.sh script, so they are all performing similar sets of checks. This will help keep our tests in closer aligment with each other.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 15, 2024
In commit caaa0f9 of PR git-lfs#2647 and number of tests of the "git lfs clone" and "git lfs pull" commands were enhanced so as check that after those commands are invoked, a "git status" command returns a "working tree clean" message. To perform these checks, a call to our assert_clean_status() shell function was added to the tests. In the case of the "cloneSSL" test, an assert_clean_status() call was added, but left commented out; it was then uncommented in commit e0eede1 of the same PR. Unfortunately, the call is made when the current working directory has not yet been changed to that of the newly cloned repository's working tree, and so will fail as it stands now. However, as described in git-lfs#5658, the "cloneSSL" test and the "clone ClientCert" tests in our t/t-clone.sh test script do not actually run to completion, as a consequence of an improper check of the TRAVIS variable (which is no longer used since we migrated our test suite to GitHub Actions in PR git-lfs#3808). This bug was already in place at the time of PR git-lfs#2647, and so the assertions are never actually performed in these tests. We expect to address this problem in a subsequent commit in this PR, and when we do so, the test "cloneSSL" test will fail because the assert_clean_status() call is made without having changed the current working directory to that of the cloned repository's working tree. Therefore we move the assertion to the end of the block of checks performed after the "pushd" shell command is used to change the directory to that of the new clone's working tree. We also take the opportunity to add the assert_clean_status() calls to a number of other tests in the t/t-clone.sh script, so they are all performing similar sets of checks. This will help keep our tests in closer aligment with each other.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Brings LFS back for
git update-indexcalls. This fixes issues wheregit statusis unclean after agit lfs pullorgit lfs clone./cc #2646 (comment)