Allow both initial and up-to changeset IDs #1057
Conversation
GitTfs/Commands/Fetch.cs
Outdated
| @@ -146,6 +146,9 @@ private void FetchRemote(bool stopOnFailMergeCommit, IGitTfsRemote remote) | |||
|
|
|||
| protected virtual void DoFetch(IGitTfsRemote remote, bool stopOnFailMergeCommit) | |||
| { | |||
| if (upToChangeSet > 0 && InitialChangeset.HasValue && InitialChangeset.Value > upToChangeSet) | |||
There was a problem hiding this comment.
The other code checks for x == -1 or x != -1. Can this change to check for that, too? It looks like 0 will not get caught by this check, but be processed as a desired stop version.
As an aside, I'm not thrilled about using -1 like this, as a magic number, even though I probably had a hand in doing that initially. Do you think it would make sense to make it a nullable int and null instead of -1? Or maybe add an extension method that encapsulates the check for -1 vs not?
There was a problem hiding this comment.
-1 is a pretty common way to indicate lack of something (index of a non-existent array element, range without upper bound, etc.) so I don't think it's [really] necessary to replace it with a nullable value or anything like that.
It probably would be easier/better to use Int32.MaxValue as the internal representation of "all changesets up to the latest one" — extremely low probability of finding a repository with 2 billion changesets means that upToChangeSet == Int32.MaxValue is pretty much the same thing as the current upToChangeSet == -1, but can be used without any special checks.
Replace the "upToChangeSet > 0" with "upToChangeSet != -1" for consistency with other code.
|
Thanks a lot for the fix! @spraints I don't know why but I had the notifications turned of. So I missed more than 2 months of movements on the repository :( I have to catch up... |
Currently "--changeset=X" has precedence over "--up-to=Y" if both options are specified. This PR fixes this behavior, making it possible to fetch a specific range of TFS changesets. 'X' being greater than 'Y' is considered an error so no fetching changesets in reverse order. :)
The change is so trivial that I'm not even sure it deserves being mentioned in the release notes.