Skip to content

Conversation

@bk2204
Copy link
Member

@bk2204 bk2204 commented Jan 23, 2024

If the user is having a problem checking out a file, it would be helpful to know why. Let's include the reason that the error is occurring so they can learn what the cause is and report it helpfully if there's a problem.

@chrisd8088
Copy link
Member

FWIW, I think this is still a good idea. Let me know if you'd like me to try updating this PR against the current main and adding a validation check of its changes somewhere in the test suite, and I'll put it on the to-do list!

@bk2204
Copy link
Member Author

bk2204 commented Mar 3, 2025

That would be fantastic if you want to do that.

@chrisd8088 chrisd8088 force-pushed the debug-failure-to-check-out branch 2 times, most recently from d11c001 to a3b4900 Compare April 21, 2025 01:47
@chrisd8088 chrisd8088 marked this pull request as ready for review April 21, 2025 03:06
@chrisd8088 chrisd8088 requested a review from a team as a code owner April 21, 2025 03:06
bk2204 and others added 2 commits April 21, 2025 12:44
If the user is having a problem checking out a file, it would be helpful
to know why.  Let's include the reason that the error is occurring so
they can learn what the cause is and report it helpfully if there's a
problem.
In a previous commit in this PR we adjusted the "git lfs checkout"
and "git lfs pull" commands so that when a Git LFS object's contents
can not be written to a file in the working tree, the commands output
a detailed set of error messages rather than just the message "could
not check out <filepath>".

We now add tests to our t-checkout.sh and t-pull.sh test scripts which
validate this change.  Specifically, the new "checkout: read-only
directory" and "pull: read-only directory" tests remove write permissions
on a directory in the working tree after removing a file which is
tracked as a Git LFS object from the directory.  The tests then check
that the "git lfs checkout" and "git lfs pull" commands output a full
set of error messages when they are unable to re-create the file.

Note that for historical reasons we expect that under these conditions
the commands will return a zero exit code, indicating success, rather
than a non-zero exit code, which would indicate failure.  We may adjust
this behaviour in the future, however, so that the commands also return
a non-zero exit code in these cases.

Note also that we use the "icacls" command on Windows to change the
directory write permissions, as the "chmod(1)" command, as emulated by
the Git Bash/MSYS2 environment, does not suffice.  See, for reference:

  https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/icacls

As well, we skip our new tests when running as the "root" user on Unix
systems or with Administrator privileges on Windows systems, since under
these conditions the Git LFS command will still be able to create a
file in the read-only directory.  To ensure we skip the tests when
the user has elevated privileges, we add a skip_if_root_or_admin()
helper function to our t/testhelpers.sh test library and call it at
the start of both of our new tests.

On Unix systems the skip_if_root_or_admin() function exits the test
if the user has an effective uid of zero, as reported in the protected
EUID environment variable in Bash.

On Windows, the function exits after checking for Administrator privileges
by running the "sfc.exe" (System File Checker) command and determining if
it output help text containing the string "SCANNOW", something it should
only do in Administrator mode.  See the following references for further
documentation on this technique:

  https://stackoverflow.com/a/58846650
  https://stackoverflow.com/a/21295806
  https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/sfc
@chrisd8088 chrisd8088 force-pushed the debug-failure-to-check-out branch from a3b4900 to 3d8f497 Compare April 21, 2025 19:52
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.

I love that!

@chrisd8088 chrisd8088 merged commit 9b9bccb into git-lfs:main Apr 24, 2025
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants