Fixes `RebaseFixture` tests by adding .gitattributes file to repo #1137

whoisj · 2015-07-02T20:10:09Z

Several of the tests in the RebaseFixture assume that core.autocrlf = true was set. On systems where this wasn't set to true, the tests failed. The correct solution is to not rely on core.autocrlf and include a .gitattributes file in the test repo.

Fixes #1111

Supersedes #1136

nulltoken · 2015-07-03T11:11:38Z

@whoisj Thanks a lot for having made the diff easier to review ❤️

nulltoken · 2015-07-03T11:14:31Z

LibGit2Sharp.Tests/RebaseFixture.cs

@@ -700,6 +700,10 @@ private void ConstructRebaseTestRepository(Repository repo)
            string workdir = repo.Info.WorkingDirectory;
            Commit commit = null;

+            CreateAttributesFile(repo, "* eol=lf");


What led you to select this rather than * text=auto for instance?

Auto means Git can choose based on the OS. I want determinism, that means the test chooses.

But that wouldn't change the blobs in the odb, only the files in the workdir. Do any tests fail when using text=auto?

Yes, because my local system has core.autocrlf = false both CanRebaseHandlePatchAlreadyApplied and VerifyRebaseDetailed fail.

<hyperbole>
The fact is core.autocrlf and text=auto are tools of the devil and should be avoided like the line-ending plague they represent. 😷
</hyperbole>

What about setting these files as binary files, instead of setting the eol attribute?

This tests lays down files with Environment.NewLine endings - setting the eol=lf line endings attribute doesn't really match this. Probably the safest way to get consistent ODB data is to hard code the line ending the files are written with (lf's), and then check them in with the binary attribute set?

Using * binary is acceptable, maybe even preferable.

Done and pushed.

Do we also need to update the line endings used when writing the files out to disk? It currently uses Environment.NewLine, which means you will get different content depending on the platform (and it will not be normalized when being written to the ODB).

Do we also need to update the line endings used when writing the files out to disk? It currently uses Environment.NewLine, which means you will get different content depending on the platform (and it will not be normalized when being written to the ODB).

Good point. @#$%^! 😠 should have let me just keep the eol=lf - now we're back in line ending hell - oh well, at least we're deterministic now. ☺️

Done, pushed.

whoisj · 2015-07-08T15:42:46Z

@nulltoken @jamill are we good with this PR? I'd love to stop seeing false failures locally. 😄

nulltoken · 2015-07-08T18:28:01Z

@whoisj As far as I know, committing a .gitattributes file in the sandboxed repo should fix the issue of having an unknown auto.crlf value on the test platform, as .gitattributes should override a global auto.crlf config setting.

I understand what you're after. However most of the LibGit2Sharp tests are designed to actually leverage, when that's possible, features from different underlying platforms in the broadest way.

During the last years, those broad integration tests helped us catch random regressions or xplat issues with libgit2.

As such, I'd prefer you to revert the last change with the newlines and switch to * text=auto.

Unless something is very wrong, the tests should work with those settings on the CI servers.

Would something weird happen on your computer with those settings, I'd rather have it put under the light rather (and eventually fixed) rather than worked around by constraining the execution of the tests.

jamill · 2015-07-08T18:31:10Z

As such, I'd prefer you to revert the last change with the newlines and switch to * text=auto .

I think that approach would also work for these test (and exercise normal usage of filters).

whoisj · 2015-07-08T18:48:18Z

As such, I'd prefer you to revert the last change with the newlines and switch to * text=auto .

I think that approach would also work for these test (and exercise normal usage of filters).

Done. Now two of the rebase tests fail locally.

Really, you guys should disable core.autocrlf and watch the tests fail for yourself. The unnecessary noise is unnecessary.

Also note that while I care very much about the results of testing on the CI servers, I also care about the results of testing locally.

whoisj · 2015-07-08T19:08:36Z

@nulltoken @jamill I've squashed and applied the changed as requested.

Looks like I had some other setting borked - the * text=auto works regardless of core.autocrlf settings now.

whoisj · 2015-07-08T19:34:00Z

There - made the tests into [Theory] tests so that we can test all the various attribute values (except binary because that is a nightmare when dealing with Environment.NewLine vs "\r\n").

nulltoken · 2015-07-08T19:47:00Z

Looks like I had some other setting borked

That's great this has been solved! Out of curiosity, what was the borked setting? How was it interfering?

whoisj · 2015-07-08T22:59:56Z

That's great this has been solved! Out of curiosity, what was the borked setting? How was it interfering?

I'd changed the Environment.NewLine to literal new lines. That was not making things happy.

Looks like autocrlf=true is affecting * eol=lf and * eol-crlf. Needed to comment them out.

Is that supposed to be happening?

ethomson · 2015-07-08T23:26:02Z

Looks like autocrlf=true is affecting * eol=lf and * eol-crlf

No. That test is wrong. It's specifying text=lf, which is not a thing.

ethomson · 2015-07-08T23:39:38Z

LibGit2Sharp.Tests/RebaseFixture.cs

-        public void VerifyRebaseDetailed()
+        [Theory]
+        [InlineData("* text=auto", new[] { "2cad6e96a0028f1764dcbde6292a9a1471acb114", "18fd3deebe6124b5dacc8426d589d617a968e8d1", "048977d8cb90d530e83cc615a17a49f3068f68c1" })]
+        //[InlineData("* text=lf", new[] { "4f813f70525a6ba4ea414a54ad89412b8d9f25aa", "6a2261c0739058ac987f1fa0014946753161b167", "6aa53c88fc1e739678749dff5acf4b00799b5c4d" })]


Don't comment this out. This should be * text eol=lf or something like that.

Don't comment this out. This should be * text eol=lf or something like that.

Thanks, I'll dabble. Mind you, these InlineData are new to the test - I've added them because I like the abuse... I guess. 😖

Per the Git Attributes Documentation:

Unspecified
If the text attribute is unspecified, Git uses the core.autocrlf configuration variable to determine if the file should be converted.

Set to string value "auto"
When text is set to "auto", the path is marked for automatic end-of-line normalization. If Git decides that the content is text, its line endings are normalized to LF on checkin.

Set to string value "crlf"
This setting forces Git to normalize line endings for this file on checkin and convert them to CRLF when the file is checked out.

Set to string value "lf"
This setting forces Git to normalize line endings to LF on checkin and prevents conversion to CRLF when the file is checked out.

I'm reading this as we should be testing "* text=auto", "* text=auto\n_.txt eol=crlf", and "_ text=auto\n*.txt eol=lf". Is that correct?

@ethomson does that make sense to you?

No, there's no text=crlf, you snipped some important lines in that quote, which makes it look like crlf and lf are properties on text. They're not, those are attributes on the eol attribute, and setting text=crlf would fall into the "Any other value causes Git to act as if text has been left unspecified." bucket

text
This attribute enables and controls end-of-line normalization. When a text file is normalized, its line endings are converted to LF in the repository. To control what line ending style is used in the working directory, use the eol attribute for a single file and the core.eol configuration variable for all text files.

Set
Setting the text attribute on a path enables end-of-line normalization and marks the path as a text file. End-of-line conversion takes place without guessing the content type.

Unset
Unsetting the text attribute on a path tells Git not to attempt any end-of-line conversion upon checkin or checkout.

Set to string value "auto"
When text is set to "auto", the path is marked for automatic end-of-line normalization. If Git decides that the content is text, its line endings are normalized to LF on checkin.

Unspecified
If the text attribute is unspecified, Git uses the core.autocrlf configuration variable to determine if the file should be converted.

Any other value causes Git to act as if text has been left unspecified.

eol
This attribute sets a specific line-ending style to be used in the working directory. It enables end-of-line normalization without any content checks, effectively setting the text attribute.

Set to string value "crlf"
This setting forces Git to normalize line endings for this file on checkin and convert them to CRLF when the file is checked out.

Set to string value "lf"
This setting forces Git to normalize line endings to LF on checkin and prevents conversion to CRLF when the file is checked out.

Sure - I was attemping to be terse (with a link to actual for detail).

Still does this seem like the right set: testing "* text=auto", "* text=auto\n.txt eol=crlf", and " text=auto\n*.txt eol=lf"?

... how do I stop markdown from killing my asterisks?

whoisj · 2015-07-09T20:54:46Z

No. That test is wrong. It's specifying text=lf, which is not a thing.

You are correct sir. Looks like I really wanted * text=auto and *.ext eol=lf, which works now.

Glad this whole .gitattributes thing is simple and clear... </sarcasm>

nulltoken · 2015-07-11T12:14:21Z

@whoisj Could you please also exercise [InlineData("* text=auto", "\n", ...] and squash?

Otherwise , that looks very neat!

Changed two `RebaseFixture` tests from `Fact` to `Theory` to support testing multiple .gitattribute values. Everthing works on Windows with `autocrlf` true or false.

whoisj · 2015-07-13T19:07:03Z

Updated, squashed.

Fixes `RebaseFixture` tests by adding .gitattributes file to repo

nulltoken · 2015-07-13T19:22:08Z

Updated, squashed.

Merged! 😻

whoisj changed the title ~~Whoisj/fix rebase tests~~ Fixes RebaseFixture tests by adding .gitattributes file to repo Jul 2, 2015

nulltoken reviewed Jul 3, 2015
View reviewed changes

whoisj force-pushed the whoisj/fix-rebase-tests branch 2 times, most recently from d149bf0 to b73c9ef Compare July 8, 2015 19:07

ethomson reviewed Jul 8, 2015
View reviewed changes

Fixes RebaseFixture tests by adding .gitattributes file to repo.

84f9f65

Changed two `RebaseFixture` tests from `Fact` to `Theory` to support testing multiple .gitattribute values. Everthing works on Windows with `autocrlf` true or false.

whoisj force-pushed the whoisj/fix-rebase-tests branch from c7152a6 to 84f9f65 Compare July 13, 2015 19:06

nulltoken added a commit that referenced this pull request Jul 13, 2015

Merge pull request #1137 from libgit2/whoisj/fix-rebase-tests

b5c40e5

Fixes `RebaseFixture` tests by adding .gitattributes file to repo

nulltoken merged commit b5c40e5 into vNext Jul 13, 2015

nulltoken deleted the whoisj/fix-rebase-tests branch July 13, 2015 19:21

nulltoken added this to the v0.22 milestone Jul 13, 2015

Fixes RebaseFixture tests by adding .gitattributes file to repo #1137

Fixes RebaseFixture tests by adding .gitattributes file to repo #1137

Uh oh!

Conversation

whoisj commented Jul 2, 2015

Uh oh!

nulltoken commented Jul 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whoisj commented Jul 8, 2015

Uh oh!

nulltoken commented Jul 8, 2015

Uh oh!

jamill commented Jul 8, 2015

Uh oh!

whoisj commented Jul 8, 2015

Uh oh!

whoisj commented Jul 8, 2015

Uh oh!

whoisj commented Jul 8, 2015

Uh oh!

nulltoken commented Jul 8, 2015

Uh oh!

whoisj commented Jul 8, 2015

Uh oh!

ethomson commented Jul 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whoisj commented Jul 9, 2015

Uh oh!

nulltoken commented Jul 11, 2015

Uh oh!

whoisj commented Jul 13, 2015

Uh oh!

nulltoken commented Jul 13, 2015

Uh oh!

Uh oh!

Fixes `RebaseFixture` tests by adding .gitattributes file to repo #1137

Fixes `RebaseFixture` tests by adding .gitattributes file to repo #1137