Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

WIP - Unsymlinked #878

Open
wants to merge 1 commit into from

4 participants

@nulltoken
Collaborator

This PR is something between a WIP and some R&D. It originates back to libgit2/libgit2sharp#196 and libgit2/libgit2sharp#201.

Curiously, it looks like the .Net tests pass on Mono, Windows x86 but fail on Windows amd64

This PR brings the C version of one of the .Net test in order to try to troubleshoot this at the libgit2 level.

@travisbot

This pull request fails (merged 04c9b9c4 into fc1826d).

@travisbot

This pull request fails (merged fa967951 into fc1826d).

@travisbot

This pull request fails (merged 0e7635d7 into fc1826d).

@travisbot

This pull request passes (merged cc94c113 into fc1826d).

@nulltoken
Collaborator

@arrbee @aroben Would you be so kind as to run this test on a 64bits Windows platform?

/cc @carlosmn @dahlbyk

@nulltoken
Collaborator

@arrbee Could you please peek at libgit2/libgit2@ed04f77? test_diff_tree__options() refuses to pass if it isn't running in sandboxed mode. And I can't figure out why....

@carlosmn
Owner

On my amd64 VM, index_0 and index_range both fail with 1 != 0, and then after tree_1 I get a debug assertion with "Expression: (unsigned)(c+1) <= 256", which presumably happens inside tree_2.

@carlosmn
Owner

Igore that, I ran the wrong test (ctest seems to choose the wrong binary).

Now properly: the test fails with 2 != 3 at line 295.

@nulltoken
Collaborator

Is there a branch / commit I should clone to get the failing test into my tree so I can experiment a little bit?

@arrbee Thanks for this. It's part of the the WIP #878.

tests-clar/diff/diff_helpers.c
@@ -88,7 +88,10 @@ int diff_line_fn(
e->line_adds++;
break;
case GIT_DIFF_LINE_ADD_EOFNL:
- assert(0);
+ /*
+ * FIXME: What should we do with the following assertion
@arrbee Owner
arrbee added a note

This constant is deprecated, so it would be an error to return it. I thought assert(0) wasn't a horrible solution, but it does probably need a comment explaining why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arrbee
Owner

Okay, I figured out the bug that is being shown the unit test...

On platforms that don't have symlinks, if a tree or index is being diffed against the working directory and we see a symlink that has been converted into a regular file, then we ignore the fact that it was "converted" and keep it as a symlink. Of course, we should really only do this if the new file info is coming from the filesystem. Line 471-474 of diff.c:maybe_modified() needs to be changed to:

    /* on platforms with no symlinks, preserve mode of existing symlinks */
    if (S_ISLNK(omode) && S_ISREG(nmode) &&
        !(diff->diffcaps & GIT_DIFFCAPS_HAS_SYMLINKS) &&
        new_iter->type == GIT_ITERATOR_WORKDIR)
        nmode = omode;

With that change, I think we should get consistent behavior across platforms.

By the way, I think we only have to do this for comparison with the WORKDIR because of the index_merge_mode function in index.c that preserves the symlink-ness of a file in the index when a git_index_add is being done. However, there are probably situations where this could get us in trouble. If so, then instead of doing new_iter->type == GIT_ITERATOR_WORKDIR we would need to do new_iter->type != GIT_ITERATOR_TREE. I think that is not the right thing, however.

@arrbee arrbee referenced this pull request from a commit
@arrbee arrbee Minor bug fixes in diff code
In looking at PR #878, I found a few small bugs in the diff code,
mostly related to work that can be avoided when processing tree-
to-tree diffs that was always being carried out.  This commit has
some small fixes in it.
5fdc41e
@arrbee
Owner

Okay, so @nulltoken I think I know what's going here except for the amd64 problem.

When you don't run sandboxed, the "gitattributes" file is not renamed to ".gitattributes" and as a result there is no .gitattributes file and the file that should be treated as a binary file is not, hence the number of hunks and the number of additions don't match up. That is why the test values don't match expected.

In going through this code, however, I found several small bugs in the diff code. One is the problem mentioned above that the platform-specific symlink capabilities are effecting tree-to-tree diffs which they should not do. Another was an unnecessary recomputation of SHAs for tree-to-tree diffs because of a bug that was clearing the "OID is valid" flag in the diff.

Since those bug fixes are independent of this PR, I just put them all into 5fdc41e and pushed them to development. If you rebase this PR, then you'll get those fixes. Of course, they won't actually matter for the different values between sandboxed / not sandboxed because that depends on the worktree based renaming of "gitattributes".

@travisbot

This pull request passes (merged 3b0b32f7 into 5fdc41e).

@travisbot

This pull request passes (merged c0a6b0a2 into 5fdc41e).

@nulltoken
Collaborator

@arrbee I've rebased the PR and fixed the tests.

Below, some things I'd like yo interest you in ;-)

  • The assert cannot be currently uncommented. Doing so makes one of the tests fail (cf the comment in the code)
  • The tree-to-tree test no longer differentiates the platform it's being run on. This passes on Windows and Linux
  • I've created a new workdir-to-tree test which compares the tree containing the symlinked blob against a plain file in the workdir. This test currently fails on Windows (cf the comment in the code)
  • Those haven't been run on a Windows amd64 platform yet.
@arrbee
Owner

@nulltoken Thanks! I will look into what's happening with the assert. I think I know what's happening, but I think we should probably be returning GIT_DIFF_LINE_ADDITION in the particular case and this is another bug. I'll go into the debugger and see what it looks like.

The Windows failure in that case is a little odd to me, but I suspect it is due to the path munging that our Windows implementation of p_readlink is doing. I realize now that assuming that the in-memory representation of a the link is the same number of bytes as the on-disk file is probably not safe. The logic for buffer allocation in diff_output.c probably needs to be tweaked...

@arrbee
Owner

Looking over the use of p_readlink throughout the code base, I'm surprised that any use of symlinks on Windows works at all. Everywhere that calls that function assumes that a buffer equal to the file size will be sufficient to hold the data, and that is not guaranteed with the win32 implementation.

I think this is a) not tested much in the library, b) not activated in most cases because we try to skip this code on Windows usually. I think that's the issue in this case with diff output - because we copy the mode bits from the index, this is one of the few places where we ever try to invoke the Windows p_readlink implementation. I actually think that may be the real bug here. Again, need to see how msysgit handles it.

@nulltoken
Collaborator

I've rebased this on top of development.

@nulltoken
Collaborator

Rebased on top of development.

  • The tree-to-tree test now passes
  • The tree-to-workdir still fails on Windows with the following message: "Failed to read symlink 'include/Nu/Nu.h': Not enough storage is available to process this command."
@carlosmn
Owner

Does this still happen? The diff code has probably gone though a couple of overhauls by now.

@nulltoken
Collaborator

@carlosmn: Rebased on latest tip. Still failing, but the error now originates back from git_fidff_foreach() and no longer from git_diff_tree_to_workdir().

diff::workdir::symlink_blob_mode_changed_to_regular_file [D:\Dropbox\Dropbox\lib
git2\libgit2\tests\diff\workdir.c:715]
  Function call failed: git_diff_foreach( diff, diff_file_cb, diff_hunk_cb, diff
_line_cb, &exp)
  error -1 - Failed to read symlink 'include/Nu/Nu.h': Not enough storage is ava
ilable to process this command.
@phatblat phatblat referenced this pull request from a commit in phatblat/libgit2
@arrbee arrbee Minor bug fixes in diff code
In looking at PR #878, I found a few small bugs in the diff code,
mostly related to work that can be avoided when processing tree-
to-tree diffs that was always being carried out.  This commit has
some small fixes in it.
69eb605
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 21, 2013
  1. @nulltoken
This page is out of date. Refresh to see the latest.
Showing with 35 additions and 0 deletions.
  1. +35 −0 tests/diff/workdir.c
View
35 tests/diff/workdir.c
@@ -689,6 +689,41 @@ void test_diff_workdir__eof_newline_changes(void)
git_diff_free(diff);
}
+void test_diff_workdir__symlink_blob_mode_changed_to_regular_file(void)
+{
+ const char *commit = "7fccd7";
+
+ git_tree *tree;
+ git_diff *diff;
+ diff_expects exp;
+
+ g_repo = cl_git_sandbox_init("unsymlinked.git");
+ cl_git_pass(git_repository_set_workdir(g_repo, ".", false));
+
+ cl_assert((tree = resolve_commit_oid_to_tree(g_repo, commit)) != NULL);
+
+ cl_git_pass(git_futils_mkpath2file("./include/Nu/Nu.h", 0755));
+ cl_git_mkfile("./include/Nu/Nu.h", "awesome content\n");
+
+ cl_git_pass(git_diff_tree_to_workdir(&diff, g_repo, tree, NULL));
+
+ git_tree_free(tree);
+
+ memset(&exp, 0, sizeof(exp));
+
+ /* FIXME: The line below crashes fails on Windows with the
+ * following error message: "Failed to read symlink 'include/Nu/Nu.h': Not
+ * enough storage is available to process this command."
+ */
+ cl_git_pass(git_diff_foreach(
+ diff, diff_file_cb, diff_hunk_cb, diff_line_cb, &exp));
+
+ // Here should live the assertions...
+ // However I *really* wonder what they should be... :-)
+
+ git_diff_free(diff);
+}
+
/* PREPARATION OF TEST DATA
*
* Since there is no command line equivalent of git_diff_tree_to_workdir,
Something went wrong with that request. Please try again.