History rewriting and documentaton

[Vogel612]

Implemented the breaking change for #621 and adjusted unit-tests to mimic behaviour before breakage.
Also adjusted the documentation for Commit#this[relativePath] as discussed in #966

Since I don't yet fully understand the history-rewriting unit-tests I have not added new Unit-Tests.

If I missed something, please do drop a note, so I can fix it 👍

Note: Running all tests gives me following results with Mono on a Ubunutu:
1989 okay, 0 failed, 0 errors, 0 inconclusive, 0 invalid, 17 ignored, 0 skipped

[dahlbyk]

Do you find the existing tests fail if you don't specify PrettifyMessage = true? I would generally think the tests should pass with the default behavior.

As for testing, I would suggest a Theory that adds a commit with CommitOptions.PrettifyMessage = false and observes the rewritten commit message with either value of RewriteHistoryOptions.PrettifyMessage.

[Vogel612]

@dahlbyk As of now the option does not have the default value true. Accordingly unit-tests relying on certain commit messages have been breaking. Since I didn't modify any existing functionality in that area, I added the explicit PrettifyMessage = true

[dahlbyk]

Ah, I see - I missed that only tests that rewrite commits had been updated.

Others may have a different opinion, but IMO if we're not specifically exercising the PrettifyMessage option I'd rather update the tests to assert assuming non-prettification (e.g. first Fact would drop the \n from the expected messages, or better yet use the same string for setup and assert).

[nulltoken]

Could we be a little more explicit about what information could be lost (and maybe a little less scary)?

Right now, this may be understood as "Use false, otherwise this will eat your hard drive for breakfast and nuke all your data in the Cloud". 😉

[nulltoken]

if we're not specifically exercising the PrettifyMessage option I'd rather update the tests to assert assuming non-prettification (e.g. first Fact would drop the \n from the expected messages, or better yet use the same string for setup and assert).

👍

[Vogel612]

HEAD for this PR after update has following test results when run on my machine:

LibGit2Sharp.Tests  Total: 1310, Errors: 0, Failed: 2, Skipped: 9, Time: 557,554s

I can't fully decipher the stacktrace, but the error seems to come from LibGit2Sharp.Tests.CloneFixture.CanCloneFromBBWithCredentials with the root cause being:

LibGit2Sharp.LibGit2SharpException : SSL error: error:140E0114:SSL routines:SSL_shutdown:uninitialized

Since this didn't show up in the travis-build I assume that's a local issue

[carlosmn]

That error message is a bug in libgit2 where we call OpenSSL's context free function with an NULL context (fixed in master). What that usually means is that we failed to connect to the remote host.

[dahlbyk]

[Vogel612]

darn monodevelop 😠

If that's the only thing I'd go ahead, fix this, squash the whole thing into two commits and update the PR. Everybody fine?

[Therzok]

Try out our roslyn branch. Be aware that memory consumption can skyrocket, but the editor is way more reliable.

[Vogel612]

Fixed the misindentation and adjusted the xml-doc and squashed the PR into two commits while rebasing onto vNext

[nulltoken]

@Vogel612 Thanks for this very thorough work!

One last nitpick, setting PrettifyMessages = true on some of the tests do not change their outcome (the commit messages being already "clean"). As such, I'd prefer to the option to not be set.

The proposal for changes below expresses this and also results in a passing test suite.

Note: I've reverted the first line for the following reasons:

• We tend to prefer beautification changes to appear in separate commits
• Keeping a trailing comma in a list (array, enums, ...) is supported by the compiler and reduce the diff noise when future changes lead to the addition of a new line.

diff --git a/LibGit2Sharp.Tests/FilterBranchFixture.cs b/LibGit2Sharp.Tests/FilterBranchFixture.cs
index 32f6f69..d71cb22 100644
--- a/LibGit2Sharp.Tests/FilterBranchFixture.cs
+++ b/LibGit2Sharp.Tests/FilterBranchFixture.cs
@@ -185,7 +185,7 @@ public void CanRewriteAuthorOfCommitsOnlyBeingPointedAtByTags()
                 OnError = OnError,
                 OnSucceeding = OnSucceeding,
                 CommitHeaderRewriter =
-                    c => CommitRewriteInfo.From(c, message: "Bam!")
+                    c => CommitRewriteInfo.From(c, message: "Bam!"),
             }, commit);

             AssertSucceedingButNotError();
@@ -591,7 +591,6 @@ public void CanRewriteParents()
             {
                 OnError = OnError,
                 OnSucceeding = OnSucceeding,
-                PrettifyMessages = true, // hashes are calculated with prettified messages
                 CommitParentsRewriter =
                     c =>
                     {
@@ -640,7 +639,6 @@ public void CanRewriteParentWithRewrittenCommit()
             {
                 OnError = OnError,
                 OnSucceeding = OnSucceeding,
-                PrettifyMessages = true, // hashes are calculated with prettified messages
                 CommitParentsRewriter =
                     c =>
                     c.Id != commitToRewrite.Id
@@ -699,7 +697,6 @@ public void CanProvideNewNamesForTags()
             {
                 OnError = OnError,
                 OnSucceeding = OnSucceeding,
-                PrettifyMessages = true, // hashes are calculated using prettified messages
                 CommitHeaderRewriter =
                     c => CommitRewriteInfo.From(c, message: ""),
                 TagNameRewriter = TagNameRewriter,

[Vogel612]

@nulltoken You're welcome. Changes applied and amended to previous commit.

[nulltoken]

🚀

Thanks!