checkout: when examining index (instead of workdir), also examine mode#4535
Conversation
|
This is a small and hopefully uncontroversial slice of #4536. I'm opening this separately only because it seems like a simple and obvious fix that we may want to include before the next release. |
|
While this seems to work alright on Travis/AppVeyor, it causes "index::filemodes::frombuffer_requires_files" to fail for me: |
|
Ugh, no. This is obviously because of #4529. |
pks-t
left a comment
pks-t
left a comment
There was a problem hiding this comment.
I think the change itself makes sense, I just have a few comments about the test
| cl_must_pass(p_chmod("testrepo/new.txt", 0755)); | ||
| cl_git_pass(git_repository_index(&index, g_repo)); | ||
| cl_git_pass(git_index_add_bypath(index, "new.txt")); | ||
| git_index_write(index); |
There was a problem hiding this comment.
This line is missing an cl_git_pass.
| cl_git_pass(git_repository_index(&index, g_repo)); | ||
| cl_git_pass(git_index_add_bypath(index, "new.txt")); | ||
| git_index_write(index); | ||
| git_index_free(index); |
There was a problem hiding this comment.
I'd prefer to move that line to the end of the test
| cl_git_pass(git_checkout_head(g_repo, &opts)); | ||
|
|
||
| cl_git_pass(p_stat("testrepo/new.txt", &st)); | ||
| cl_assert(!GIT_PERMS_IS_EXEC(st.st_mode)); |
There was a problem hiding this comment.
As the second test is concerned about a different thing (index vs workdir), I'd move it into a separate test.
When checking out a file, we determine whether the baseline (what we expect to be in the working directory) actually matches the contents of the working directory. This is safe behavior to prevent us from overwriting changes in the working directory. We look at the index to optimize this test: if we know that the index matches the working directory, then we can simply look at the index data compared to the baseline. We have historically compared the baseline to the index entry by oid. However, we must also compare the mode of the two items to ensure that they are identical. Otherwise, we will refuse to update the working directory for a mode change.
ethomson
force-pushed
the
ethomson/checkout_typechange_with_index_and_wd
branch
from
3cf2996 to
755557e
Compare
|
I've fixed up the test per your requests. |
pks-t
left a comment
pks-t
left a comment
There was a problem hiding this comment.
Perfect, thanks a lot! One variable is leaking now, though. Do you want this change to be backported to v0.26.1?
| void test_checkout_head__typechange_workdir(void) | ||
| { | ||
| git_checkout_options opts = GIT_CHECKOUT_OPTIONS_INIT; | ||
| git_object *target; |
When the working directory has changed permissions on a file - but only the permissions, such that the contents of the file are identical - ensure that `git_checkout` updates the permissions to match the checkout target.
When both the index _and_ the working directory has changed permissions on a file permissions on a file - but only the permissions, such that the contents of the file are identical - ensure that `git_checkout` updates the permissions to match the checkout target.
ethomson
force-pushed
the
ethomson/checkout_typechange_with_index_and_wd
branch
from
755557e to
4e4771d
Compare
|
Agreed, I added the backport label. |
2 participants
When checking out a file, we determine whether the baseline (what we expect to be in the working directory) actually matches the contents of the working directory. This is safe behavior to prevent us from overwriting changes in the working directory.
We look at the index to optimize this test: if we know that the index matches the working directory, then we can simply look at the index data compared to the baseline.
We have historically compared the baseline to the index entry by oid. However, we must also compare the mode of the two items to ensure that they are identical. Otherwise, we will refuse to update the working directory for a mode change.
Update this behavior and include a test to validate the behavior.