Ignore case only changes in export from git to tfs #1063

EdorianDark · 2017-04-13T11:16:34Z

Rewrite my last pull request.
Fixes #1060
Is this ok?

pmiossec

Thanks a lot for the work and the abnegation!!!

I have one big question (see comment on line 36) that I will be pleased if you could explain to me.

And some other small comments.

Thanks again!

pmiossec · 2017-04-13T20:55:40Z

GitTfs/Core/Changes/Git/RenameEdit.cs

-            workspace.Rename(Path, PathTo, Score);
-            var workspaceFile = workspace.GetLocalPath(PathTo);
-            _repository.CopyBlob(NewSha, workspaceFile);
+            if (Path.ToLower() != PathTo.ToLower())


I think I prefer using string.Equals(Path, PathTo, StringComparison.OrdinalIgnoreCase) here.

Ok, I will change this.

pmiossec · 2017-04-13T20:57:29Z

GitTfs/Core/Changes/Git/RenameEdit.cs

+        }
+
+        //From https://support.microsoft.com/en-us/help/320348/how-to-create-a-file-compare-function-in-visual-c
+        private bool FileCompare(string file1, string file2)


I think that this method could be extracted in a helper and unit tested (takes 2 streams following the previous comments).

pmiossec · 2017-04-13T20:58:29Z

GitTfs/Core/Changes/Git/RenameEdit.cs

+            }
+
+            // Open the two files.
+            fs1 = new FileStream(file1, FileMode.Open);


why not use using(var fs1 = new FileStream(file1, FileMode.Open))?

If you refactor to take 2 Streams, the using() will indeed be outside of the method...

pmiossec · 2017-04-13T21:01:33Z

GitTfs/Core/Changes/Git/RenameEdit.cs

+            // Return the success of the comparison. "file1byte" is 
+            // equal to "file2byte" at this point only if the files are 
+            // the same.
+            return ((file1byte - file2byte) == 0);


return (file1byte == file2byte); instead ?

pmiossec · 2017-04-13T21:12:46Z

GitTfs/Core/Changes/Git/RenameEdit.cs

+        //From https://support.microsoft.com/en-us/help/320348/how-to-create-a-file-compare-function-in-visual-c
+        private bool FileCompare(string file1, string file2)
+        {
+            int file1byte;


These definitions could be made closer to the use...

pmiossec · 2017-04-13T21:21:40Z

GitTfs/Core/Changes/Git/RenameEdit.cs

+                //This change can not be mapped to TFS, since TFS is case insensitive
+                var workspaceFile = workspace.GetLocalPath(Path);
+                var newTestFile = workspaceFile + NewSha;
+                _repository.CopyBlob(NewSha, newTestFile);


I think that we should not use CopyBlob() but write a new method that return a Stream that will be compared by the method bool FileCompare(Stream stream1, Stream stream2).

That way, we are not obliged to write the 2 contents in new files to read and delete them after. All will be made in memory.

Look at the code of CopyBlob(). There is the lines needed to return the streams
You could have a look

pmiossec · 2017-04-13T21:22:34Z

GitTfs/Core/Changes/Git/RenameEdit.cs

+            if (fs1.Length != fs2.Length)
+            {
+                // Close the file
+                fs1.Close();


Close() could be removed if using using(). That will make the code shorter...

pmiossec · 2017-04-13T21:37:16Z

GitTfs/Core/Changes/Git/RenameEdit.cs

+            else
+            {
+                //This change can not be mapped to TFS, since TFS is case insensitive
+                var workspaceFile = workspace.GetLocalPath(Path);


Sorry but I don't understand exactly what we try to achieve here (it should be my fault because I was not there at the beginning).

I understand that we compare the content of 2 blobs but what does the content of the 2 blobs represent exactly?
What is changeInfo ?

Because I see that you use changeInfo.newSha and changeInfo.oldSha to name the files and because that's sha, these 2 values will be the same if the content is the same and different if the content is different!

So, it seems that we don't need to compare the content, no? Does I miss something?

I was under the impression, that the file name is used for calculating the hash.
Since this is not the case, the file comparison is unnecessary.

EdorianDark · 2017-04-19T09:10:33Z

I noticed, that sometimes a rename is split into an add and a delete.
This means there is filtering needed in the GitRepository.
But the change would be too complicated for me.

EdorianDark · 2017-04-20T07:46:44Z

@pmiossec I want to to filter needed the changes in GitRepository. For that I would need to collect the changes first and then look at for case-only renames in the add, delete and rename changes.

Would this change to the collection removing the yield be too big?

EdorianDark · 2017-04-20T10:20:45Z

I tried the the other approach. I hope this is ok now.

spraints · 2017-04-21T11:54:16Z

@EdorianDark I like this implementation. Can you extract the new code to a separate method or class and add some unit tests for it?

pmiossec

Thanks again for the work.
You could find here some small things to change.
We are near the end ;)

And at the end, feel free to squash your commits and force push.

pmiossec · 2017-04-24T11:30:56Z

GitTfs/Core/GitChangeInfo.cs

@@ -46,15 +47,72 @@ public struct ChangeType
        public static IEnumerable<GitChangeInfo> GetChangedFiles(TextReader reader)
        {
            string line;
+            List<GitChangeInfo> changes = new List<GitChangeInfo>(); ;


I prefer using var is this special case. What do you think?

Remove the leading ;

pmiossec · 2017-04-24T11:32:02Z

GitTfs/Core/GitChangeInfo.cs

+            return filterd;
+        }
+
+        private static IEnumerable<GitChangeInfo> FilterNotRepresentable(IEnumerable<GitChangeInfo> changed)


Could your rename changed to changes?

pmiossec · 2017-04-24T11:33:10Z

GitTfs/Core/GitChangeInfo.cs

            }
+            var filterd = FilterNotRepresentable(changes);


Could your rename to filtered? Or even better return directly.

pmiossec · 2017-04-24T11:38:09Z

GitTfs/Core/GitChangeInfo.cs

+                String.Compare(cha.path, cha.pathTo, System.StringComparison.OrdinalIgnoreCase) != 0 ||
+                cha.newSha != cha.oldSha).ToList();
+
+            foreach (var cha in remainingChanges)


I prefer naming the variable change instead of only cha (and below too...)

pmiossec · 2017-04-24T11:39:44Z

GitTfs/Core/GitChangeInfo.cs

+
+            var deletes = remainingChanges.Where(cha => cha.Status == GitChangeInfo.ChangeType.DELETE).ToArray();
+            var adds = remainingChanges.Where(cha => cha.Status == GitChangeInfo.ChangeType.ADD).ToArray();
+            var removeElement = "RemoveElement";


Extract as a const...

pmiossec · 2017-04-24T11:49:58Z

GitTfs/Core/GitRepository.cs

@@ -484,6 +484,7 @@ public IEnumerable<IGitChangedFile> GetChangedFiles(string from, string to)
            using (var diffOutput = CommandOutputPipe("diff-tree", "-r", "-M", "-z", from, to))
            {
                var changes = GitChangeInfo.GetChangedFiles(diffOutput);
+


remove changes done on this file?

pmiossec · 2017-04-24T11:52:58Z

GitTfs/Core/GitChangeInfo.cs

+                }
+            }
+
+            remainingChanges.RemoveAll(cha => cha.Status == removeElement);


Why not just use a .Where() to not introduce sideeffects?

pmiossec · 2017-04-24T11:58:46Z

GitTfs/Core/GitChangeInfo.cs

+
+            foreach (var cha in remainingChanges)
+            {
+                if(cha.Status == GitChangeInfo.ChangeType.RENAMEEDIT)


I will be happy if we could extract a method and name it as mucha as possible clearly after what we do.
From what I understand, something like ReconsiderChangeWithCaseRenamingAsModification()...

Do you mean something like FilterAddDeleted and FilterRenameEdit?
Or which operations should I extract?

pmiossec · 2017-04-24T12:05:10Z

GitTfs/Core/GitChangeInfo.cs

+                if (cha.Status == GitChangeInfo.ChangeType.ADD)
+                {
+                    if (deletes.Any(del => String.Equals(cha.path, del.path, System.StringComparison.OrdinalIgnoreCase)))
+                    {


For efficiency and readability, isn't it possible to that at the same time we handle the DELETE change (line 90)?
We already search for the ADD change corresponding to the DELETE. We just have to update it at the same time.
This way we prevent 2 more searches (and remove most of this code)....

pmiossec · 2017-04-24T12:10:13Z

GitTfsTest/Core/GitChangeInfoTests.cs

@@ -245,5 +245,28 @@ public void MultipleChangesWithJapanese()
                Assert.Equal("TestFiles/試し4.txt", changes[4].path);
            }
        }
+
+        [Fact]
+        public void NotRepresentableChanges()


Could you cut this test in 2 (one for Add/Delete case and one for Renaming) with a naming explicit (I know that the other tests are not very well named :( )

EdorianDark · 2017-04-24T13:52:35Z

I did not understand which part to factor out of FilterNotRepresentable.
Can you explain it again?

pmiossec · 2017-04-24T23:54:53Z

I have made the changes in a branch that you could find here: https://github.com/pmiossec/git-tfs/tree/EdorianDark-master2

I have cut all the changes in small commits to make your review easier.
Feel free to fetch them and put them in your pull request if that please you!
Don't forget to squash all that if you do so...

EdorianDark · 2017-04-25T07:52:22Z

Thank you very much!! 👍

pmiossec

Ok to merge it. 👍

EdorianDark mentioned this pull request Apr 13, 2017

Ignore case only changes in export from git to tfs. #1024

Closed

pmiossec requested changes Apr 13, 2017

View reviewed changes

EdorianDark closed this Apr 19, 2017

EdorianDark reopened this Apr 20, 2017

EdorianDark force-pushed the master2 branch from 7d4392d to 1ce67c6 Compare April 20, 2017 10:16

pmiossec changed the title ~~Ignore case only changes in export from git to tfs 2~~ Ignore case only changes in export from git to tfs Apr 24, 2017

pmiossec reviewed Apr 24, 2017

View reviewed changes

EdorianDark force-pushed the master2 branch from 5f88ff4 to 9ee45f7 Compare April 24, 2017 13:44

Filter case-only file renames, as they cannot be applied in tfs.

6e33979

EdorianDark force-pushed the master2 branch from 9ee45f7 to 6e33979 Compare April 25, 2017 07:51

pmiossec approved these changes Apr 25, 2017

View reviewed changes

Merge branch 'master' into master2

9495629

pmiossec merged commit 899f97e into git-tfs:master Apr 25, 2017

pmiossec pushed a commit that referenced this pull request Apr 25, 2017

Filter case-only file renames, as they cannot be applied in tfs. (#1063)

4e9a5a0

spraints mentioned this pull request May 3, 2017

Small code refactoring in git to tfs filtering #1066

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore case only changes in export from git to tfs #1063

Ignore case only changes in export from git to tfs #1063

EdorianDark commented Apr 13, 2017 •

edited

pmiossec left a comment

pmiossec Apr 13, 2017

EdorianDark Apr 18, 2017

pmiossec Apr 13, 2017

pmiossec Apr 13, 2017

pmiossec Apr 13, 2017

pmiossec Apr 13, 2017

pmiossec Apr 13, 2017

pmiossec Apr 13, 2017

pmiossec Apr 13, 2017

EdorianDark Apr 18, 2017

EdorianDark commented Apr 19, 2017

EdorianDark commented Apr 20, 2017

EdorianDark commented Apr 20, 2017

spraints commented Apr 21, 2017

pmiossec left a comment

pmiossec Apr 24, 2017

pmiossec Apr 24, 2017

pmiossec Apr 24, 2017

pmiossec Apr 24, 2017

pmiossec Apr 24, 2017

pmiossec Apr 24, 2017

pmiossec Apr 24, 2017

pmiossec Apr 24, 2017

EdorianDark Apr 24, 2017

pmiossec Apr 24, 2017

pmiossec Apr 24, 2017

EdorianDark commented Apr 24, 2017 •

edited

pmiossec commented Apr 24, 2017

EdorianDark commented Apr 25, 2017

pmiossec left a comment

Ignore case only changes in export from git to tfs #1063

Ignore case only changes in export from git to tfs #1063

Conversation

EdorianDark commented Apr 13, 2017 • edited

pmiossec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdorianDark commented Apr 19, 2017

EdorianDark commented Apr 20, 2017

EdorianDark commented Apr 20, 2017

spraints commented Apr 21, 2017

pmiossec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdorianDark commented Apr 24, 2017 • edited

pmiossec commented Apr 24, 2017

EdorianDark commented Apr 25, 2017

pmiossec left a comment

Choose a reason for hiding this comment

EdorianDark commented Apr 13, 2017 •

edited

EdorianDark commented Apr 24, 2017 •

edited