Skip to content

Conversation

fourpastmidnight
Copy link
Contributor

This PR contains a single commit which re-implements the changes made in PR #973. The original author of #973 declined to make the requested changes due to time constraints.

Since the contribution of those changes could have a significant, positive impact on enabling Git-TFS to better clone TFS repositories into Git repositories, I have made the requested changes to PR #973 and am submitting this PR in its place.

@fourpastmidnight
Copy link
Contributor Author

Not sure why the tests failed...as I didn't author the original solution, and PR #973 was passing all of its checks, I'm unsure why these checks should be failing now.

@pmiossec
Copy link
Member

pmiossec commented Nov 11, 2016 via email

@fourpastmidnight
Copy link
Contributor Author

I can't get the tests to run locally; I'm going to try and figure that out and also see if I can reproduce the issue as well.

@pmiossec
Copy link
Member

pmiossec commented Nov 11, 2016 via email

@fourpastmidnight
Copy link
Contributor Author

Ah, I was talking unit and integration tests; but you are right, I want to look into the functional tests. Thanks.

@fourpastmidnight
Copy link
Contributor Author

OK, good news, I think: the functional tests passed when I ran them locally. Could this be something temporal on the AppVeyor CI box?? Is there a way to force this to run again up on AppVeyor?

@fourpastmidnight
Copy link
Contributor Author

Also, should I have updated NEXT.md?

@pmiossec
Copy link
Member

pmiossec commented Nov 12, 2016

OK, good news, I think: the functional tests passed when I ran them locally. Could this be something temporal on the AppVeyor CI box?? Is there a way to force this to run again up on AppVeyor?

No. In fact the tests are not passing. I reproduce the problem. (Perhaps you forgot to pass the directory path toward the new build exe --or update the script if you found it easier-- and it used the one in your path?)
I successfully found the problem:

In file TfsHelper.Common.cs, there is 2 call to AddNewRootBranch in GetRootChangesetForBranch()
Suppress the 2nd. After that, the tests pass...

Also, should I have to updated NEXT.md?

yes, do it. Update the line to point to the 2 pull request (the yours first) and the 2 user ids.

rootChangesetMergeInfo.TargetChangeset : rootChangesetMergeInfo.SourceChangeset;

var rootBranch = new RootBranch(rootChangesetInParentBranch, rootChangesetInChildBranch, tfsPathBranchToCreate);
AddNewRootBranch(rootBranches, rootBranch);

AddNewRootBranch(rootBranches, new RootBranch(rootChangesetInParentBranch, tfsPathBranchToCreate));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be removed...

@fourpastmidnight
Copy link
Contributor Author

fourpastmidnight commented Nov 12, 2016

Hmm, wonder why the tests passed locally for me? In any event, I'll update the PR as you indicated a bit later this evening and hopefully we'll be good to go.

@pmiossec
Copy link
Member

Hmm, wonder why the tests passed loudly for me?

I have updated my message to imagine a reason but perhaps you didn't see
the update...

In any event, I'll update the PR as you indicated a bit later this evening
and hopefully we'll be good to go.

It's late for me, too... ;-) I will wait. No problem.

@fourpastmidnight
Copy link
Contributor Author

@pmiossec Yes, I saw your revisions, and thank you for spotting that!! I totally missed it. What I was trying to communicate is that I ran the functional tests with the exact same code I pushed up and they ran to completion without any errors. So that's what's weird. But, I do see that the line you mentioned should in fact be removed.

I'll be pushing up the changes right after I run the functional tests.

Thanks again for the once-over!! 😄

@fourpastmidnight
Copy link
Contributor Author

HAHA, I figured out why my functional tests passed! I wasn't using the newly built git-tfs, but my locally installed copy. Oops!! That'll do it!

@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 fourpastmidnight force-pushed the feature/improve-detection-of-branch-point-changeset branch from e2549c2 to 334a040 Compare November 13, 2016 01:47
@pmiossec pmiossec merged commit a8f685d into git-tfs:master Nov 13, 2016
@pmiossec
Copy link
Member

thanks a lot @fourpastmidnight and @jeremy-sylvis-tmg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants