Update TfsHelper.Common.cs' detection of the branch point changeset f… #973

jeremy-sylvis-tmg · 2016-06-28T21:06:34Z

I ran into an issue nearly identical to #955 - someone earlier in a project's history created a "branch" folder, deleted the folder, and then properly created the new branch by branching off our root branch.
As a result, git-tfs was unable to determine the root changeset because it assumed the first changeset in the child branch was always the actual branch point.

Assume we have this:
$/MyProject/Trunk - C100

We now create $/MyProject/Branch as a folder - C101.
We do some more work on Trunk - C102-105.
We realize the error of our ways and delete the $/MyProject/Branch folder - C106.
We recreate $/MyProject/Branch by branching Trunk C105 -> C107.
We further branch $/MyProject/Branch to $/MyProject/ChildBranch (still at C105 from its parent) -> C108.
We do some work on $/MyProject/Branch - C109-C115.
We merge that work to $/MyProject/ChildBranch - C116.

During the clone, $/MyProject/Trunk would fetch correctly.
Git-tfs would fail to determine the root changeset for $/MyProject/Branch. Even if I specify a changeset, it gets confused because it only reads from the folder creation up through the deletion - C106 and C107.
Git-tfs would detect $/MyProject/ChildBranch correctly but can't find changesets corresponding to the merge of C109-C115 into it, because git-tfs only read pre-branch history of $/MyProject/Branch.

The changes in this pull request fix these scenarios by:

Updating detection of the root changeset to read the child branch's change history to find the branch point instead of assuming it's the first changeset in the child branch
Updating branch initialization to detect when a child branch's branch point is a point other than its first changeset and to only read changes from that point forward

One unit test was added to illustrate this problem but the test itself is limited as most of the affected code is faked for tests. It does, at least, serve as an illustration of the problem.

…or a given path to support scenarios where the branch point isn't the first changeset; update GitTfsRemote to support setting InitialChangeset externally; update InitBranch to handle setting tfsRemote.InitialChangeset when a given branch's actual branch point is specified instead of assumed. This fixes issue git-tfs#955 for sure - I had the same issue.

jeremy-sylvis-tmg · 2016-06-28T21:11:41Z

Oh, I should add -
I tested with GitTfs.Vs2010 and .Vs2015.

jeremy-sylvis-tmg · 2016-06-28T21:13:44Z

AppVeyor automated build failed on chocolately's install of tfs2010objectmodel. I'm not sure how to help with that.

pmiossec · 2016-06-28T22:48:36Z

Oh ! shiny! That's the contribution I have waited too long....

I have relaunched the appveyor build and it is now failing on a real problem :-(

We are using paket on this project. Perhaps, instead of using nuget, perhaps just update the xunit runner version in 'paket.references' file and do a paket.exe install. And commit the file and the lock file regenerated. Should fix at least this build error...

jeremy-sylvis-tmg · 2016-06-29T15:12:39Z

Ah! I've never before used paket; sure - I can update that. Thanks for pointing me in the right direction.

…gain) covered by paket

jeremy-sylvis-tmg · 2016-06-29T16:10:19Z

I think I've got the NuGet -> paket fixes in place but AppVeyor is timing out again.

pmiossec · 2016-06-29T16:23:00Z

I have relaunched the build but it fails again :-( I don't know the exact problem, surely your fix that is not complete but I think I can have a deeper look and fix that... (soon). I let you know...

jeremy-sylvis-tmg · 2016-06-29T16:33:38Z

Oh, it's more than possible I've missed something with this build process - I'm not at all familiar with many parts of this solution's build process :)

pmiossec · 2016-06-29T16:51:44Z

I think you did well, once you know we use paket. That's just that you have also to revert the changes made by nuget to the GitTfsTest.csproj file

…ket install

pmiossec · 2016-06-29T17:12:01Z

Since the build with AppVeyor was introduced, we never had the problem with restauring the chocolatey packages ( here the tfs2010objectmodel).
That's the 3rd times in 2 days that it failed and I find it very boring!
I don't know where the problem come from! AppVeor or chocolatey!?!
Because chocolatey had some problems the last days and AppVeyor became very since quite some times (build use to start immediatly and now we have to wait at least 3minutes each times!)

We will see how it behave in the futur but I really think to remove build for TFS2010 (or make it in another independant build....)

cc @spraints

pmiossec · 2016-06-29T17:28:43Z

@jeremy-sylvis-tmg Build failed. I didn't understand what you tried to do :(
You just had to revert the csproj file with git :(
I tried it and it solved the problem (my personal build passed!).
You could find it here : https://github.com/pmiossec/git-tfs/tree/jeremy/fix_branch-rename

and reset to this commit if you want ;)

jeremy-sylvis-tmg · 2016-06-29T17:35:01Z

Well, I'd reverted the changes from NuGet and performed the .paket/paket.exe install - it looks like the build's still failing with paket's changes to the .csproj. More specifically, it looks like paket is just modifying the .csproj to reference the nuget packages through paket.

It looks like I'm going to need to leave the .csproj untouched entirely. Any idea why paket might be trying to reference the NuGet package that I'm trying to not depend on?

… build is failing to compile though local compilation succeeded and ReSharper is suggesting removing the redundant private setter

jeremy-sylvis-tmg · 2016-06-29T18:36:08Z

Well, AppVeyor succeeded but TeamCity is running into what appears to be an issue stemming from a lack of private setters on a few properties. I've corrected that. It's odd that local builds succeed, appveyor's build/tests succeed, but TeamCity's build fails.

jeremy-sylvis-tmg · 2016-06-29T20:59:49Z

Excellent... looks like that did it. All yours, @pmiossec

fourpastmidnight

I've reviewed this code. In short, I can't wait to see these changes make it into the Git-TFS code base.

I have however, made, a lot of my comments around unnecessary reformatting and whitespace changes that made it take longer for me to review this code.

I also found two significant concerns: A renamed public interface method, and the addition of an interface property that in one implementing class throws an exception for both the getter and setter.

Lastly, I personally would appreciate if you would rebase your commits and reword your commit messages such that they follow Git good commit message guidelines. This is just one article, but I have 5 more bookmarked that are very similar, if not identical in nature--so these are fairly strong guidelines that ought not to be ignored and makes browsing repository history much easier--especially when running a git log --one-line command.

Other than that, excellent work in helping resolve this long outstanding problem with Git-TFS. I myself wanted to look into this but didn't have the time to dedicate to it, as I've been working on a project for Git for Windows, so Thank You for this PR!!

fourpastmidnight · 2016-10-06T14:50:41Z

GitTfs.VsCommon/TfsHelper.Common.cs


                    string renameFromBranch;
-                    var rootChangesetInParentBranch =


Generally it's considered bad form to reformat code. For example, perhaps this project has some "line-length" policy with respect to their source code formatting? Also, it just adds extra noise to the Pull Request that reviewers have to look at that detracts from the main point of the Pull Request.

More likely, the reason why the code was formatted the way it was is because when reviewing Pull Requests, the lines of code fit nice and neatly in the amount of space provided by GitHub. Now I have to horizontally scroll over to the right to see what's hanging off the edge.

Please revert all code re-formatting so that it fits nicely in the GitHub source code review pane.

Most of the reformatting, renaming, and/or refactoring I'd performed was in order to add clarity where it was lacking and to make it easier to understand what's going on.

I've got exceptionally limited available time to work on this, but thank you for the guidance on Git commit messages.

I'll address what I can but my concern in fixing this issue was fixing broken functionality, as opposed to preserving whitespace & formatting.

I agree, refactoring in a pull request that add a feature or fix a bug is a bad idea (that could just achieve the goal to delay the PR merge :( )

But now that it's done, no need to undo. It's fine...

@pmiossec: You mentioned in your review below about the change in name for this variable and wanting the rename change reverted. Based on the nature of the changes being made, I think a name change may be warranted to clarify what exactly is now happening in this region of code. I opposed the reformatting of the line breaks that were made--but not necessarily the name change.

Of course, you're the maintainer and may have other reasons for not wanting to change this name, so I'll defer to your judgement. But I just wanted to be clear about what I opposed with respect to this particular diff and also be supportive of why a name change may not be such a bad thing.

I don't have a strong advice on that naming and don't really know which is the best. Keep it that way if you feel that's better...

fourpastmidnight · 2016-10-06T14:52:15Z

GitTfs.VsCommon/TfsHelper.Common.cs

                    Trace.WriteLineIf(renameFromBranch != null, "Found original branch '" + renameFromBranch + "' (renamed in branch '" + tfsPathBranchToCreate + "')");
+


Generally, it's considered bad form to add/remove existing whitespace (unless fixing an obvious whitespace issue, like a space hanging off the end of a line--but then, only sparingly) for some of the same reasons that it's bad form to reformat existing code. In this case, the primary reason being that it adds noise to the PR and requires extra effort for reviewers.

fourpastmidnight · 2016-10-06T14:55:33Z

GitTfs.VsCommon/TfsHelper.Common.cs

@@ -516,28 +565,30 @@ public override string ToString()
            }
        }

-        private IEnumerable<MergeInfo> GetMergeInfo(string tfsPathBranchToCreate, string tfsPathParentBranch,


Please un-reformat.

fourpastmidnight · 2016-10-06T14:55:44Z

GitTfs.VsCommon/TfsHelper.Common.cs

        {
            var mergedItemsToFirstChangesetInBranchToCreate = new List<MergeInfo>();
-            var merges = VersionControl


Please un-reformat.

fourpastmidnight · 2016-10-06T14:56:27Z

GitTfs.VsCommon/TfsHelper.Common.cs

                    new ItemIdentifier(tfsPathBranchToCreate),
                    new ItemIdentifier[] { new ItemIdentifier(tfsPathParentBranch), },
                    null)
                .OrderByDescending(x => x.SourceChangeset.ChangesetId);
+


To be clear, I like these whitespace changes, personally. But it would've been better to have a commit soley dedicated to whitespace changes that I could ignore when reviewing since they don't affect the execution of the code.

That's a fair point - I'll keep it in mind in the future. Most of the whitespace changes were automatically performed by ReSharper as I went through; I'll have to keep an eye on it.

fourpastmidnight · 2016-10-06T15:06:35Z

GitTfs/Core/GitTfsRemote.cs

@@ -695,10 +703,17 @@ private void quickFetch(ITfsChangeset changeset)
        private IEnumerable<ITfsChangeset> FetchChangesets(bool byLots, int lastVersion = -1)
        {
            int lowerBoundChangesetId;
+
+            // If we're starting at the Root side of a branch commit (e.g. C1) but there are invalid commits between C1 and the actual branch-side of the commit operation


It would be helpful if each comment line fit within the line-length for a GitHub pull request code file review pane.

I'm sure it would... but that also means roughly 2/3 of a 1080p display is wasted when editing source through Visual Studio.
It feels like trying to fix a problem by treating a symptom (line width) rather than the problem (poor code review layout in GitHub).

It's actually to help others to be able to quickly review and understand the code without needing to scroll horizontally (which is a huge pain in the butt). While you may have a wide display, not everyone does. In addition, GitHub limits the length of lines shown in PR code reviews, so even if you and I have wide displays, they're virtually useless when reviewing a PR here on GitHub.

Wide monitors are great for watching movies and gaming--not so much for coding (in trems of long line lengths). They are also great for being able to view multiple source files side-by-side...but beyond that, not really useful in code files in and of themselves.

fourpastmidnight · 2016-10-06T15:07:26Z

GitTfs/Core/IGitRepository.cs

@@ -16,7 +16,7 @@ public interface IGitRepository : IGitHelpers
        IGitTfsRemote ReadTfsRemote(string remoteId);
        IGitTfsRemote CreateTfsRemote(RemoteInfo remoteInfo, string autocrlf = null, string ignorecase = null);
        void DeleteTfsRemote(IGitTfsRemote remoteId);
-        IEnumerable<string> GetGitRemoteBranches(string gitRemote);
+        IEnumerable<string> GetGitRepositoryBranchRemotes(string gitRemote);


Why the method name change? You're changing an interface contract that others may depend upon, potentially breaking 3rd party libraries that build on top of Git-TFS.

Clarity, essentially.

I wouldn't change this method name--it's on an interface. Do you know who's all implementing this interface? Is it only Git-TFS libraries that implement this interface? You can't be sure, because the interface is public.

I really think this needs to be reverted.

Yes, revert the rename, please...

fourpastmidnight · 2016-10-06T15:09:31Z

GitTfs/Core/IGitTfsRemote.cs

@@ -48,6 +48,7 @@ public interface IGitTfsRemote
        string Prefix { get; }
        bool ExportMetadatas { get; set; }
        Dictionary<string, string> ExportWorkitemsMapping { get; set; }
+        int? InitialChangeset { get; set; }


Based on comments I made above for this property, I think this is better off being a method on the interface. Please change this to a method pair: GetInitialChangeset() and SetInitialChangeset(int?). This way, you can document under what conditions these proposed methods will throw a DerivedRemoteException exception.

I agree - I think I'd gone with the property out of sheer frustration and desire to just expose the needed information. It should have been a method pair.

It should have been a method pair

Minor correction: It should be a method pair

Please change this and update the pull request.

fourpastmidnight · 2016-10-06T15:13:01Z

GitTfs/Core/TfsInterop/RootBranch.cs

-            get { return string.Format("{0} C{1}{2}", TfsBranchPath, RootChangeset, (IsRenamedBranch ? " renamed" : "")); }
+            get
+            {
+                if (TargetBranchChangesetId > -1)


Hmm, properties shouldn't do much computation or logic. They should simply return some value. Since you added some conditional logic here, it might be better to extract a private method from the getter here and return the result from the newly extracted method. What are the chances this logic may be needed elsewhere?

I think the chance of this logic being needed elsewhere is slim. On one hand, I agree that a property shouldn't do computation or logic. On the other hand, we'd then be adding a private method for the sole purpose of building a string exposed by a property. They feel equally wrong to me.

It's actually a good thing. I'm not going to say that this has to be done, but it's definitely something I would strongly advocate for.

fourpastmidnight · 2016-10-06T15:16:21Z

GitTfsTest/Integration/CloneTests.cs

+            //Ensure our Branch was migrated...
+            Assert.NotNull(branch);
+
+            // So, it turns out GetRootChangesetForBranch is really the unit under test here.


Hmm, maybe you could create a Test Spy or some other kind of "test double" to help you assert/verify the behavior you're expecting?

I had considered it.

The larger problem is that the unit under test isn't the appropriate one; we could test the integration of Clone and InitBranch easily enough using a spy/stub via Moq, but the logic we're actually concerned with is in InitBranch, TfsHelper, DerivedGitTfsRemote, GitTfsRemote, and RootBranch.
I would have loved to test-drive these changes but didn't (and don't) have the time necessary to build missing test coverage, especially given the necessary refactoring with buried dependencies, singletons, etc. This was my compromise - a test added to the existing layer illustrating what we're going for with a warning for future developers.

Yikes...hmm.. Yeah, seems like a concept is missing and is now looking to be extracted out and become its own thing--which is not a trivial thing to accomplish. Well, at least it's documented. 😄

We know that there a lot of things that could be improved in our tests.
We are fine with these type of integration tests because the cases that we want to tests are difficult to build with pure unit tests :(

You could keep the test like that.

fourpastmidnight · 2016-10-06T17:14:51Z

@pmiossec, @spraints I'm very interested in seeing this PR make it into the code base. I have left a review and was wondering if you had any further observations? I'd also be interested in overall feedback on my feedback (meta-feedback? 😉) to help me learn what you expect of code contributions (beyond what's documented in the CONTRIBUTING.md document).

pmiossec · 2016-10-06T22:59:56Z

I'm very interested in seeing this PR make it into the code base.

I am also very interested by that (even if I couldn't find the time to do a proper review until now!).
I thank you to did it and I will take advantage of that to do it now...

First of all, I mostly agree with what @fourpastmidnight wrote in the review...

@jeremy-sylvis-tmg I think @fourpastmidnight told you weel but I will say it myself. When we do a PR, like , that's better to do NO refactoring (moving code, renaming, fix white spaces,...) to easy the code review.
And keep all theses refactoring in a separate PR (even based on the first one if needed) doing just that. That way the 2 PRs are a lot easier to review!
Because your PR is already huge and difficult to review :(

Could you also take some time rebase all your work onto master?

No need to worrie too much for the formatting in this PR (even if in normal time we should do) because I plan to use CodeFormatter ( https://github.com/dotnet/codeformatter ) after that PR merged to clean a lot of things...

Let's go begin the review ;)

pmiossec

If you could rebase the PR and fix most of the things, I will be pleased to merge this pull request.
Thanks a lot again for the hard work! 👍

pmiossec · 2016-10-06T23:01:38Z

GitTfs.Setup/GitTfs.Setup.csproj

@@ -98,6 +98,13 @@
    <WCFMetadata Include="Service References\" />
  </ItemGroup>
  <Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
+  <Choose>


Please revert all the changes of this files. Blame paket but there is just noise here :(

pmiossec · 2016-10-06T23:03:59Z

GitTfs.Vs2015/GitTfs.Vs2015.csproj

@@ -40,9 +40,6 @@
    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
  </PropertyGroup>
  <ItemGroup>
-    <Content Include="app.config.install.xdt">


Please revert all the changes of this files. That's tricky but the explaination is here: https://github.com/git-tfs/git-tfs/blob/master/doc/paket.md#when-adding-a-new-package

pmiossec · 2016-10-06T23:04:38Z

GitTfs.Vs2015/app.config.install.xdt

@@ -1,33 +0,0 @@
-<?xml version="1.0" encoding="utf-8"?>
-<configuration xmlns:xdt="http://schemas.microsoft.com/XML-Document-Transform">


Again, revert this change...

pmiossec · 2016-10-06T23:08:10Z

GitTfs.VsCommon/TfsHelper.Common.cs


                    string renameFromBranch;
-                    var rootChangesetInParentBranch =


I agree, refactoring in a pull request that add a feature or fix a bug is a bad idea (that could just achieve the goal to delay the PR merge :( )

But now that it's done, no need to undo. It's fine...

pmiossec · 2016-10-06T23:09:45Z

GitTfs/Commands/InitBranch.cs

@@ -76,6 +76,8 @@ public int Run(string tfsBranchPath, string gitBranchNameExpected)

            if (CloneAllBranches)
            {
+                // TODO: Remove this!
+                //Debugger.Launch();
                return CloneAll(tfsBranchPath);


You're right!! Please remove this comment ;)

pmiossec · 2016-10-06T23:12:43Z

GitTfs/Core/GitRepository.cs

@@ -155,7 +155,7 @@ private IGitTfsRemote ReadTfsRemote(string tfsUrl, string tfsRepositoryPath)
            }
        }

-        public IEnumerable<string> GetGitRemoteBranches(string gitRemote)
+        public IEnumerable<string> GetGitRepositoryBranchRemotes(string gitRemote)


Not sure I like this rename, too... we are in the repository so no need to add it in the name.

pmiossec · 2016-10-06T23:14:16Z

GitTfs/Core/IGitRepository.cs

@@ -16,7 +16,7 @@ public interface IGitRepository : IGitHelpers
        IGitTfsRemote ReadTfsRemote(string remoteId);
        IGitTfsRemote CreateTfsRemote(RemoteInfo remoteInfo, string autocrlf = null, string ignorecase = null);
        void DeleteTfsRemote(IGitTfsRemote remoteId);
-        IEnumerable<string> GetGitRemoteBranches(string gitRemote);
+        IEnumerable<string> GetGitRepositoryBranchRemotes(string gitRemote);


Yes, revert the rename, please...

pmiossec · 2016-10-06T23:15:11Z

GitTfs/GitTfs.csproj

+    <When Condition="($(TargetFrameworkIdentifier) == '.NETStandard' And ($(TargetFrameworkVersion) == 'v1.0' Or $(TargetFrameworkVersion) == 'v1.1' Or $(TargetFrameworkVersion) == 'v1.2' Or $(TargetFrameworkVersion) == 'v1.3' Or $(TargetFrameworkVersion) == 'v1.4' Or $(TargetFrameworkVersion) == 'v1.5')) Or ($(TargetFrameworkIdentifier) == '.NETFramework' And ($(TargetFrameworkVersion) == 'v4.0' Or $(TargetFrameworkVersion) == 'v4.5' Or $(TargetFrameworkVersion) == 'v4.5.1' Or $(TargetFrameworkVersion) == 'v4.5.2' Or $(TargetFrameworkVersion) == 'v4.5.3' Or $(TargetFrameworkVersion) == 'v4.6' Or $(TargetFrameworkVersion) == 'v4.6.1' Or $(TargetFrameworkVersion) == 'v4.6.2'))">
+      <PropertyGroup>
+        <__paket__LibGit2Sharp_props>net40\LibGit2Sharp</__paket__LibGit2Sharp_props>
+      </PropertyGroup>


Paket noise again. PLease revert all the file...

pmiossec · 2016-10-06T23:16:12Z

GitTfsTest/GitTfsTest.csproj

@@ -183,6 +183,22 @@
    <None Include="paket.references" />
  </ItemGroup>
  <Import Project="$(MSBuildBinPath)\Microsoft.CSharp.targets" />
+  <Choose>
+    <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And ($(TargetFrameworkVersion) == 'v4.0' Or $(TargetFrameworkVersion) == 'v4.5' Or $(TargetFrameworkVersion) == 'v4.5.1' Or $(TargetFrameworkVersion) == 'v4.5.2' Or $(TargetFrameworkVersion) == 'v4.5.3' Or $(TargetFrameworkVersion) == 'v4.6' Or $(TargetFrameworkVersion) == 'v4.6.1' Or $(TargetFrameworkVersion) == 'v4.6.2')">


Paket noise again. Please revert the file...

pmiossec · 2016-10-06T23:18:51Z

GitTfsTest/Integration/CloneTests.cs

+            //Ensure our Branch was migrated...
+            Assert.NotNull(branch);
+
+            // So, it turns out GetRootChangesetForBranch is really the unit under test here.


We know that there a lot of things that could be improved in our tests.
We are fine with these type of integration tests because the cases that we want to tests are difficult to build with pure unit tests :(

You could keep the test like that.

pmiossec · 2016-10-21T23:39:26Z

Do you have problem updating you pull request.
I'm eager to merge it...

jeremy-sylvis-tmg · 2016-10-25T18:29:32Z

Yes, because I was working on this as part of a project at work and I’ll be moving on within the week ☺

From: Philippe Miossec [mailto:notifications@github.com]
Sent: Friday, October 21, 2016 6:39 PM
To: git-tfs/git-tfs git-tfs@noreply.github.com
Cc: Jeremy Sylvis Jeremy.Sylvis@tmg.global; Mention mention@noreply.github.com
Subject: Re: [git-tfs/git-tfs] Update TfsHelper.Common.cs' detection of the branch point changeset f… (#973)

Do you have problem updating you pull request.
I'm eager to merge it...

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com//pull/973#issuecomment-255489908, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ATQas15sgkGi-Zli5vdah-FE1koM-snZks5q2U0wgaJpZM4JAiYv.
This email and any files transmitted with it are confidential and intended solely for the designated recipient. Any other use of this email is prohibited. If you have received this email in error please notify the sender. Please note that any views or opinions presented in this email are solely those of the author and do not necessarily represent those of the company. Finally, the recipient should check this email and any attachments for the presence of viruses. The company accepts no liability for any damage caused by any virus transmitted by this email.

fourpastmidnight · 2016-10-25T22:01:42Z

@pmiossec I wouldn't mind putting this into a state suitable for merging. This will help a great many people with using git-tfs. If @jeremy-sylvis-tmg is unwilling to fix this up, I'll take a look into doing it.

pmiossec · 2016-10-25T22:05:01Z

@jeremy-sylvis-tmg 😢
@fourpastmidnight great! 👍

fourpastmidnight · 2016-10-31T14:25:53Z

@pmiossec: Something came up last week and I didn't get to this. I hope to get to it sometime this week, though. Just wanted to ping you and let you know I didn't forget!!

pmiossec · 2016-11-02T08:32:40Z

No problem since you are still on it ;-). I understand that it is not always easy to find some time to contribute.

fourpastmidnight · 2016-11-11T04:49:16Z

Sorry, things have been hectic. I started on this tonight. Hopefully I can finish this this weekend. I can't wait to see this merged.

BTW, do you know, is there a way to get this pull request directly into my clone of the git-tfs project? Optimally, I would love to pull this pull request into a feature branch off of master instead of working off master itself....

If this isn't easy to do, I'll probably end up creating a new pull request with the updated changes contained in this pull request.

spraints · 2016-11-11T13:35:54Z

BTW, do you know, is there a way to get this pull request directly into my clone of the git-tfs project? Optimally, I would love to pull this pull request into a feature branch off of master instead of working off master itself....

@fourpastmidnight This might work....

I assume you've already done git clone https://github.com/git-tfs/git-tfs.
In the git-tfs directory, do git remote add mine https://github.com/fourpastmidnight/git-tfs. You may have already done this, too.
To get this PR: git fetch origin refs/pull/973/head:refs/heads/whatever-you-want and then git checkout whatever-you-want.
Get the branch into your fork: git push --set-upstream mine whatever-you-want:whatever-you-want.

Unfortunately, there isn't a great way to have multiple collaborators work on a single PR. You can do it by adding each other as collaborators to your individual forks (https://github.com/fourpastmidnight/git-tfs/settings/collaboration), I think.

fourpastmidnight · 2016-11-11T14:24:04Z

Thanks @spraints.

pmiossec · 2016-11-11T18:31:11Z

@fourpastmidnight Thanks. Good work and good luck ;)

@jeremy-sylvis-tmg

@jeremy-sylvis-tmg is the original author of this commit. He originally issued Pull Request git-tfs#973. However, a code review was performed by myself and @pmiossec whereby changes were requested. Unfortunately, the author of the pull request declined to make the requested changes due to time constraints. Because the changes implemented in this commit could potentially solve a long outstanding problem in how git-tfs currently detects branch point changeset detection, I have recreated this commit with the requested changes. Basically, this commit allows gits-tfs to properly clone a TFS branch into a git repository when the branch was first created as a folder, deleted, and subsequently created as an actual TFS branch: *--C1--C2--C3--C4(X)--*--*--* \ C5(?)--*--* In the diagram above, a folder would be created at C3 called 'Branch'. At C4, it was determined that either C3 was a mistake, or simply that a true TFS branch was desired as opposed to a TFS folder. So at C4, the folder named 'Branch' is deleted. At C5, an actual TFS branch called 'Branch' is created, and C5 is the first changeset on TFS branch 'Branch'. Prior to this commit, Git-Tfs would get "confused" because branches and folder in TFS are similar, but not the same--and there's no analogue in Git whatsoever. The branch 'Branch' usually fails to clone properly, and IIRC, you'll get messages such as "Could not find parent changeset for branch 'Branch'". In my experience, providing a parent changeset ID does not always resolve the situation satisfactorily. The changes in this commit detect this situation. At C3, it detects that a folder was created, called 'Branch'. TFS folders are not branches, and so this commit causes git-tfs to keep searching through TFS history for the most recent branch point. So, when the folder named 'Branch' is deleted at C4, git-tfs will continue to search for merge history, which it finds at C5, where the TFS branch named 'Branch' is created. This is a true branch point off of the main line. Because C3 and C4 effectively "cancel" each other out, the resulting Git history after cloning this TFS repository history looks like the following (NOTE: this author is unsure how git-tfs represents commits C3 and C4 in the commit history of a Git repository since Git has no concept of "folders"): *--C1--C2--C3--C4(X)--*--* \ C5--*--* NOTE: This commit does add some methods to an interface. As such, the next version of Git-TFS should have it's minor version incremented; e.g. the next version of Git-Tfs released containing this commit should be in the 0.26.x series. Signed-off-by: Craig E. Shea <craig.e.shea@gmail.com>

fourpastmidnight · 2016-11-11T21:12:50Z

@spraints, @pmiossec I have just pushed up a branch to my clone of this project containing the core changes of this PR with the requested changes. Is it possible for me to "force push" to this pull request instead of creating a new one?

If not, I'll just create a new PR referencing this PR as the original source of the changes. Let me know how you wish me to proceed.

pmiossec · 2016-11-11T21:24:19Z

Do the easier. I think that's doing a new PR...

fourpastmidnight · 2016-11-11T22:20:31Z

@pmiossec Great, thanks! I'll be submitting the PR momentarily.

@jeremy-sylvis-tmg

@jeremy-sylvis-tmg is the original author of this commit. He originally issued Pull Request git-tfs#973. However, a code review was performed by myself and @pmiossec whereby changes were requested. Unfortunately, the author of the pull request declined to make the requested changes due to time constraints. Because the changes implemented in this commit could potentially solve a long outstanding problem in how git-tfs currently detects branch point changeset detection, I have recreated this commit with the requested changes. Basically, this commit allows gits-tfs to properly clone a TFS branch into a git repository when the branch was first created as a folder, deleted, and subsequently created as an actual TFS branch: *--C1--C2--C3--C4(X)--*--*--* \ C5(?)--*--* In the diagram above, a folder would be created at C3 called 'Branch'. At C4, it was determined that either C3 was a mistake, or simply that a true TFS branch was desired as opposed to a TFS folder. So at C4, the folder named 'Branch' is deleted. At C5, an actual TFS branch called 'Branch' is created, and C5 is the first changeset on TFS branch 'Branch'. Prior to this commit, Git-Tfs would get "confused" because branches and folder in TFS are similar, but not the same--and there's no analogue in Git whatsoever. The branch 'Branch' usually fails to clone properly, and IIRC, you'll get messages such as "Could not find parent changeset for branch 'Branch'". In my experience, providing a parent changeset ID does not always resolve the situation satisfactorily. The changes in this commit detect this situation. At C3, it detects that a folder was created, called 'Branch'. TFS folders are not branches, and so this commit causes git-tfs to keep searching through TFS history for the most recent branch point. So, when the folder named 'Branch' is deleted at C4, git-tfs will continue to search for merge history, which it finds at C5, where the TFS branch named 'Branch' is created. This is a true branch point off of the main line. Because C3 and C4 effectively "cancel" each other out, the resulting Git history after cloning this TFS repository history looks like the following (NOTE: this author is unsure how git-tfs represents commits C3 and C4 in the commit history of a Git repository since Git has no concept of "folders"): *--C1--C2--C3--C4(X)--*--* \ C5--*--* NOTE: This commit does add some methods to an interface. As such, the next version of Git-TFS should have it's minor version incremented; e.g. the next version of Git-Tfs released containing this commit should be in the 0.26.x series. Signed-off-by: Craig E. Shea <craig.e.shea@gmail.com>

fourpastmidnight · 2017-02-22T22:30:43Z

@spraints Since PR #1017 basically duplicated this PR in order to make the fixups that were requested by both you and I, and #1017 has been merged, perhaps we should close this PR.

spraints · 2017-02-23T21:42:04Z

Since PR #1017 basically duplicated this PR in order to make the fixups that were requested by both you and I, and #1017 has been merged, perhaps we should close this PR.

👍

jeremy-sylvis-tmg added 2 commits June 29, 2016 10:43

Update the version of xunit.runners.visualstudio referenced by paket

51cdedd

Remove GitTfsTest's unnecessary packages.config - this is now (once a…

1f63f75

…gain) covered by paket

Remove the packages.config reference from GitTfsTest.csproj; run a pa…

3747c73

…ket install

jeremy-sylvis-tmg added 2 commits June 29, 2016 12:42

Shotgun debugging the removal of NuGet wire-up in GitTfsTest.csproj

7880b2a

Implement private setters on RootBranch's autoproperties - TeamCity's…

c32e02f

… build is failing to compile though local compilation succeeded and ReSharper is suggesting removing the redundant private setter

pmiossec mentioned this pull request Aug 23, 2016

Git clone stops if changeset is rename #986

Open

fourpastmidnight suggested changes Oct 6, 2016

View reviewed changes

pmiossec requested changes Oct 6, 2016

View reviewed changes

fourpastmidnight mentioned this pull request Nov 11, 2016

Improve branch point changeset detection #1017

Merged

fourpastmidnight mentioned this pull request Nov 18, 2016

error initializing the branch #859

Closed

pmiossec mentioned this pull request Dec 13, 2016

The clone/fetch algorithm doesn't handle renaming of branch #1029

Closed

spraints closed this Feb 23, 2017

		Trace.WriteLineIf(renameFromBranch != null, "Found original branch '" + renameFromBranch + "' (renamed in branch '" + tfsPathBranchToCreate + "')");

		@@ -1,33 +0,0 @@
		<?xml version="1.0" encoding="utf-8"?>
		<configuration xmlns:xdt="http://schemas.microsoft.com/XML-Document-Transform">

Update TfsHelper.Common.cs' detection of the branch point changeset f… #973

Update TfsHelper.Common.cs' detection of the branch point changeset f… #973

Conversation

jeremy-sylvis-tmg commented Jun 28, 2016

jeremy-sylvis-tmg commented Jun 28, 2016

jeremy-sylvis-tmg commented Jun 28, 2016

pmiossec commented Jun 28, 2016

jeremy-sylvis-tmg commented Jun 29, 2016

jeremy-sylvis-tmg commented Jun 29, 2016

pmiossec commented Jun 29, 2016 via email

jeremy-sylvis-tmg commented Jun 29, 2016

pmiossec commented Jun 29, 2016

pmiossec commented Jun 29, 2016

pmiossec commented Jun 29, 2016

jeremy-sylvis-tmg commented Jun 29, 2016

jeremy-sylvis-tmg commented Jun 29, 2016

jeremy-sylvis-tmg commented Jun 29, 2016

fourpastmidnight left a comment

Choose a reason for hiding this comment

fourpastmidnight Oct 6, 2016 • edited

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

fourpastmidnight Oct 6, 2016 • edited

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

fourpastmidnight Oct 6, 2016 • edited

Choose a reason for hiding this comment

fourpastmidnight Oct 6, 2016 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fourpastmidnight commented Oct 6, 2016

pmiossec commented Oct 6, 2016

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

pmiossec commented Oct 21, 2016

jeremy-sylvis-tmg commented Oct 25, 2016

fourpastmidnight commented Oct 25, 2016

pmiossec commented Oct 25, 2016 • edited

fourpastmidnight commented Oct 31, 2016

pmiossec commented Nov 2, 2016 via email

fourpastmidnight commented Nov 11, 2016

spraints commented Nov 11, 2016

fourpastmidnight commented Nov 11, 2016

pmiossec commented Nov 11, 2016

fourpastmidnight commented Nov 11, 2016

pmiossec commented Nov 11, 2016 via email

fourpastmidnight commented Nov 11, 2016

fourpastmidnight commented Feb 22, 2017

spraints commented Feb 23, 2017

fourpastmidnight Oct 6, 2016 •

edited

fourpastmidnight Oct 6, 2016 •

edited

fourpastmidnight Oct 6, 2016 •

edited

fourpastmidnight Oct 6, 2016 •

edited

pmiossec commented Oct 25, 2016 •

edited