[postponed] Add an interactive mode when init branches #316

Closed
wants to merge 6 commits into
from

3 participants

@pmiossec
git-tfs member

The user can now choose its branches names interactively when using --interactive option...

@spraints
git-tfs member

a) nice!

b) It looks like it doesn't compile?

c) Please replace the use of Console.* with something that's injected. My preference would be to use _stdout for things that are not related to the interaction, and build a small dialog shell thing for doing the prompt/response (i.e. out/in) loop. It doesn't need to be fancy, just something that you'd use like:

var name = _dialog.Ask("What should it be called?");
_dialog.Say("You picked the name'" + name "'.");
var response = _dialog.Ask("Are name ok?(Yes/No/Quit)");
if(response == "Yes") { /* ... */ } /* ... */

I would keep the implementation of the dialog thing really simple. The main point of it is to isolate the dependency on Console. Also, if it's not too much of a PITA, you probably want to check that stdin is actually an interactive user session. I imagine it's pretty unlikely that git-tfs would be run like this, but it does happen that someone will write a script that kicks off an interactive session on a server with no UI.

@pmiossec
git-tfs member
@sc68cal sc68cal commented on an outdated diff Mar 8, 2013
GitTfs/Commands/InitBranch.cs
- var result = CreateBranch(defaultRemote, tfsBranchPath, allRemotes);
+ do
+ {
+ branchesWithExpectedNames = GetBranchNames(childBranchPathsNotAlreadyFetched);
+ if (!ControlBranchNaming(branchesWithExpectedNames, allRemotes))
+ continue;
+ Console.WriteLine("Names that will be used for the tfs branches :");
+ foreach (var tfsBranchPath in branchesWithExpectedNames.Keys)
+ {
+ _stdout.WriteLine("- " + tfsBranchPath + " => " + branchesWithExpectedNames[tfsBranchPath]);
+ }
+ Console.Write("Are name ok?(Yes/No/Quit)");
+ var answer = (string.Empty + Console.ReadLine()).ToLower();
+ if (answer == "q") return GitTfsExitCodes.OK;
+ if (answer == "y") break;
+ } while (true);
@sc68cal
sc68cal added a line comment Mar 8, 2013

Ugh - I hate do/while loops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sc68cal sc68cal commented on the diff Mar 8, 2013
GitTfs/Commands/InitBranch.cs
if (result < 0)
return result;
}
return GitTfsExitCodes.OK;
}
+ private bool ControlBranchNaming(Dictionary<string, string> branchesWithExpectedNames, IEnumerable<IGitTfsRemote> allRemotes)
+ {
+ var branchesNames = branchesWithExpectedNames.Values.Select(b => b.ToLower());
+ var duplicates = branchesNames.GroupBy(b => b).Where(g => g.Count() > 1).Select(g => g.Key);
+ if (duplicates.Any())
@sc68cal
sc68cal added a line comment Mar 8, 2013

why not if( duplicates.Any() || conflicts.Any()) ? They share a common body.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sc68cal sc68cal commented on the diff Mar 8, 2013
GitTfs/Core/GitRepository.cs
throw new GitTfsException("The name specified for the new git branch is not allowed. Choose another one!");
return gitBranchName;
}
+ public bool IsValidBranchName(string gitBranchName)
@sc68cal
sc68cal added a line comment Mar 8, 2013

This method is not very useful - it just obfuscates. I'd prefer to keep the _repository.Refs.IsValidName call.

@pmiossec
git-tfs member
pmiossec added a line comment Mar 8, 2013

Perhaps, but that method is there to be called in the init-branch command...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pmiossec
git-tfs member

Hope that's good...

@spraints
git-tfs member

👍 from me.

@sc68cal

Bumping this - @pmiossec - I think we're good for merging this - though you may need to rebase / fix conflicts.

@pmiossec
git-tfs member
@spraints
git-tfs member

Sounds good. Feel free to rebase or merge, whichever is easier.

@pmiossec
git-tfs member

Will never be merged...

@pmiossec pmiossec closed this Jul 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment