Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Small improvements #530

Merged
merged 16 commits into from Mar 14, 2014

Conversation

Projects
None yet
4 participants
Contributor

KindDragon commented Jan 18, 2014

Small code improvements, fixed some moments that are not allowed to clone my repository.

@sc68cal sc68cal commented on an outdated diff Jan 19, 2014

GitTfs.VsCommon/Wrappers.cs
@@ -455,7 +455,18 @@ public void ForceGetFile(string path, int changeset)
public void GetSpecificVersion(int changeset)
{
- _workspace.Get(new ChangesetVersionSpec(changeset), GetOptions.Overwrite | GetOptions.GetAll);
+ try
+ {
+ _workspace.Get(new ChangesetVersionSpec(changeset), GetOptions.Overwrite | GetOptions.GetAll);
+ }
+ catch (Microsoft.TeamFoundation.TeamFoundationServiceUnavailableException ex)
+ {
+ throw new TfsTimeoutException(ex.Message, ex);
+ }
+ catch (Microsoft.TeamFoundation.Framework.Client.DatabaseOperationTimeoutException ex)
+ {
+ throw new TfsTimeoutException(ex.Message, ex);
+ }
@sc68cal

sc68cal Jan 19, 2014

Contributor

👍

@spraints spraints and 1 other commented on an outdated diff Jan 20, 2014

GitTfs.VsCommon/TfsHelper.Common.cs
@@ -177,7 +177,7 @@ public virtual int GetRootChangesetForBranch(string tfsPathBranchToCreate, strin
{
Trace.WriteLine("Looking for the changeset between changeset id " + lowerBound + " and " + upperBound);
var firstBranchChangesetIds = VersionControl.QueryHistory(tfsPathParentBranch, VersionSpec.Latest, 0, RecursionType.Full,
- null, new ChangesetVersionSpec(lowerBound), new ChangesetVersionSpec(upperBound), int.MaxValue, true,
+ null, new ChangesetVersionSpec(lowerBound), new ChangesetVersionSpec(upperBound), int.MaxValue, false,
@spraints

spraints Jan 20, 2014

Owner

This seems like it should break git-tfs. I guess it doesn't?

includeChanges A flag that describes whether the individual item changes will be included with the changesets. Otherwise, only changeset metadata is included.
-- http://msdn.microsoft.com/en-us/library/bb138961.aspx

git-tfs uses the Item information...

foreach(var change in changeset.Changes)
{
    var tfsPath = change.Item.ServerItem; // <-- doesn't this need `includeChanges` to be true?
    ...
@KindDragon

KindDragon Jan 20, 2014

Contributor

Where it is used in this method?

@spraints

spraints Jan 22, 2014

Owner

Oops. I missed the context. Nevermind.

@spraints spraints commented on an outdated diff Jan 20, 2014

GitTfs/Commands/InitBranch.cs
@@ -151,7 +152,14 @@ public int Run()
foreach (var tfsBranchPath in childBranchPaths)
{
_stdout.WriteLine("- " + tfsBranchPath.TfsRepositoryPath);
- tfsBranchPath.RootChangesetId = defaultRemote.Tfs.GetRootChangesetForBranch(tfsBranchPath.TfsRepositoryPath);
+ try
+ {
+ tfsBranchPath.RootChangesetId = defaultRemote.Tfs.GetRootChangesetForBranch(tfsBranchPath.TfsRepositoryPath);
+ }
+ catch (GitTfsException ex)
+ {
+ _stdout.WriteLine("Exception occured: {0}", ex.Message);
@spraints

spraints Jan 20, 2014

Owner

This may be a bit mysterious to users. Maybe replace the message with "Unable to find root changeset for {0}, treating it as a baseless branch: {1}", tfsBranchPath.TfsRepositoryPath, ex.Message. Also, add a Trace.WriteLine(ex) so that we can get the full stack trace if we need to.

@spraints spraints commented on an outdated diff Jan 20, 2014

GitTfs/Commands/InitBranch.cs
@@ -248,6 +256,12 @@ private IGitTfsRemote CreateBranch(IGitTfsRemote defaultRemote, string tfsReposi
throw new GitTfsException("error: The Git branch name '" + gitBranchName + "' is not valid...\n");
Trace.WriteLine("Git local branch will be :" + gitBranchName);
+ if (_globals.Repository.HasRemote(gitBranchName))
+ {
+ Trace.WriteLine("Remote already exist");
+ return _globals.Repository.ReadTfsRemote(gitBranchName);
@spraints

spraints Jan 20, 2014

Owner

Should this make sure that it's the same remote? e.g. if the tfs path is different, we should probably at least warn the user.

@spraints spraints commented on an outdated diff Jan 20, 2014

GitTfs/Core/GitTfsRemote.cs
- if (ExportMetadatas)
+ private bool ProcessMergeChangeset(ITfsChangeset changeset, bool stopOnFailMergeCommit, LogEntry log)
@spraints

spraints Jan 20, 2014

Owner

Can you add a note here that ProcessMergeChangeset returns true on error? I default to assuming that true == success.

@spraints spraints commented on an outdated diff Jan 20, 2014

GitTfs/Core/GitTfsRemote.cs
@@ -317,30 +316,13 @@ public IFetchResult FetchWithMerge(long mergeChangesetId, bool stopOnFailMergeCo
fetchResult.NewChangesetCount = fetchedChangesets.Count;
foreach (var changeset in fetchedChangesets)
{
@spraints

spraints Jan 20, 2014

Owner

❤️ Thanks for breaking up this method.

@spraints spraints commented on an outdated diff Jan 20, 2014

GitTfs/Util/Retry.cs
@@ -0,0 +1,39 @@
+using System;
+using System.Collections.Generic;
+using System.Threading;
+using Sep.Git.Tfs.Core;
+
+namespace Sep.Git.Tfs.Util
+{
+ public static class Retry
@spraints

spraints Jan 20, 2014

Owner

❤️ I really like this. This will also be useful for #494.

Can you add some unit tests for it?

Contributor

KindDragon commented Mar 11, 2014

What can I do to speed up merge this PR? Maybe move some commits into a separate PR?

Contributor

sc68cal commented Mar 11, 2014

👍 for merge.

Owner

spraints commented Mar 12, 2014

What can I do to speed up merge this PR? Maybe move some commits into a separate PR?

Right now, just resolve the merge conflicts with master.

In general, smaller changes are easier to review and discuss. In the future, breaking things like this up into a few different branches would be 💎.

Owner

pmiossec commented Mar 12, 2014

In general, smaller changes are easier to review and discuss. In the future, breaking things like this up into a few different branches would be 💎.

@KindDragon My point of view is that perhaps you could cut these changes in 2 PR. One with all that is linked to Retry() and one with the other changes

PS :

  • commit "nofetch parameter in documentation fixed" is already corrected in master so you could remove it.
  • not sure commit "InitBranch crash on GetRootChangesetForBranch exception fixed" should end up in master because it was solved in #480 and will be boring to merge. But no sure... What is your opinion on that?
Contributor

KindDragon commented Mar 12, 2014

In general, smaller changes are easier to review and discuss. In the future, breaking things like this up into a few different branches would be 💎.

It was problematic in that situation. I had a lot dependent changes.

Contributor

KindDragon commented Mar 12, 2014

PS :

I removed both commit. I'll wait for PR #480

Owner

spraints commented Mar 13, 2014

🤘 Just to double-check, this branch is OK to merge as-is?

In general, smaller changes are easier to review and discuss. In the future, breaking things like this up into a few different branches would be 💎.

It was problematic in that situation. I had a lot dependent changes.

Making PRs that depend on other ones is totally fine. Even branching off of another PR works. The diff view on the pull request will shrink once the base branch is merged.

Contributor

KindDragon commented Mar 13, 2014

🤘 Just to double-check, this branch is OK to merge as-is?

Yes, I finished

spraints added a commit that referenced this pull request Mar 14, 2014

@spraints spraints merged commit 4ec2097 into git-tfs:master Mar 14, 2014

1 check passed

default The Travis CI build passed
Details

spraints added a commit that referenced this pull request Mar 14, 2014

@KindDragon KindDragon deleted the KindDragon:small-improvments branch Mar 14, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment