Skip to content

Conversation

hazzik
Copy link

@hazzik hazzik commented Jun 19, 2015

  • Resolve TODO on Checkout command
  • Correctly cache changesets

Correctly cache changesets
Copy link
Owner

Choose a reason for hiding this comment

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

I don't like a lot this solution which consist in travelling along a lot of commit to construct a list and then filter this list with the tfs path :( Because that's not very good in terms of performance...

I'm pretty shure that we can write a private method that take an optional path and :

  • if the path is not empty, return immediatly if the commit with the good id and path is found
  • if the path is empty, continue to look for this specific changeset id through all the commits

This method could be wrapped in 2 public methods one which accept a tfs path and one another without...

Copy link
Author

Choose a reason for hiding this comment

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

Because that's not very good in terms of performance...

Most of the cases there will be only one commit in the list, so no performance issues.

Copy link
Author

Choose a reason for hiding this comment

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

if the path is not empty, return immediatly if the commit with the good id and path is found
if the path is empty, continue to look for this specific changeset id through all the commits

We can not do this, because then changesetsCache will be corrupted, eg. some commits will never be found.

@hazzik hazzik force-pushed the find_commit_in_specific_branch branch from b0fa78f to a1cb1c2 Compare June 20, 2015 06:47
@hazzik
Copy link
Author

hazzik commented Jun 20, 2015

@pmiossec I've changed the solutions. Now it has 2 methods: one which accepts tfsPath and returns single commit (the only change from your code is how cache is handled); and another one, which resolves all commits, with this changesetId.

As I understad, this is what you have suggested.

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.

2 participants