New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NeedReview]Rename branch and add file #663

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@pmiossec
Member

pmiossec commented Oct 12, 2014

Should fix #660 and #650...

The main goal of this PR, is to manage a rename changeset
as a changeset belonging to the new renamed branch and not
as belonging to the old branch (as done before).

This way, other changes than the rename could be included in the
newly created commit (and not be ignored as done before
because the changes are made inside the $/new/tfs/branch/path)

@pmiossec pmiossec changed the title from [WIP]Rename branch and add file to [NeedReview]Rename branch and add file Oct 29, 2014

@@ -304,6 +304,12 @@ public class FetchResult : IFetchResult
public string ParentBranchTfsPath { get; set; }
}
public struct RenamingData
{

This comment has been minimized.

@pmiossec

pmiossec Oct 29, 2014

Member

@spraints Can you review that PR and tell me if it suits you?
Especially the use of this static struct that contains the data
for handling the rename.

It's, for the moment, the best thing have found to not complexify too much
the source code.

The idea behind that is:

  • detect a rename changeset when fetching the old branch
  • do not process this changeset as a changeset of the old branch but store in the struct the data that will be needed to process the rename changeset
  • fetch the new branch and process the first rename changeset as a changeset of the new branch.

I used a static struct to not be obliged to return the data with the fetchResult (that's not a problem) but also to modify a lot of method signature to pass these data.

I'm not 100% happy but the resulting source code is perhaps much clearer....

This comment has been minimized.

@spraints

spraints Nov 5, 2014

Member

Sorry for the long delay.

I'd really like to avoid using a static struct. It's basically a global variable. 😝 Even though it's not too much different in practice, I'd much prefer an object in the structuremap container that's marked as a "Singleton". There are a couple other places where that's done.

}
RenamingData.IsProcessingRenameChangeset = false;
RenamingData.LastParentCommitBeforeRename = null;
}

This comment has been minimized.

@spraints

spraints Nov 5, 2014

Member

It seems like this should be able to work without any globals. Why not add the rename data to fetchResult, and have the caller aggregate the information and include it in future calls to Fetch?

This comment has been minimized.

@pmiossec

pmiossec Nov 5, 2014

Member

It's what I planned to do at the beginning but it will need to modify a lot of API adding a parameter that won't make big sense for most of the uses.

I think it will complicate a lot the code...

That's why I wanted your review (perhaps you would have a great idea about it...) because I was not very pleased by this static struct :(

I will try to modify the API and see if I could make something beautiful....

pmiossec added some commits Oct 12, 2014

Process changeset when with new branch (instead with old one)
Ignore rename changeset when discovered because
we want to process it as a changeset of the renamed branch.

This way, if changes are made (other than the branch renaming),
there will be fetched in TfsHelperBase.GetChangesets()!

Before that change, there would have been not fetched
because the tfs path was the one of the old branch but
the changes belong to the new renamed branch path!
@pmiossec

This comment has been minimized.

Member

pmiossec commented Jun 1, 2015

#692 is the way to go...I hope really soon!

@pmiossec pmiossec closed this Jun 1, 2015

@pmiossec pmiossec deleted the pmiossec:rename_branch_and_add_file branch Jun 5, 2015

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