Separate VCS detection from VCS object creation.

[ajwong, 2009-05-23 02:13:42]

Hi Guido,

I'd like to separate the VCS detection routine from the VCS object creation
code.

This is because Evan's git-cl tool imports upload.py and invokes it to handle
uploading patches from a git repository into rietveld.  However, in the chromium
dev envionrment, the working directory of the source tree is pretty convoluted,
and you can have an svn checkout sitting in a subdirectory of a git checkout. 
Example:

src/.git  # Most of the source code is controlled via git.
src/third-party/Webkit/.svn  # Various subtrees maybe controleld by svn, or
other.

If you execute git-cl inside src/third-party/Webkit/..., git-cl and upload.py
end up with different ideas as to which source control system is the one to work
off of, and things get confusing.

My thought was if upload.py could expose it's source control detection routine,
git-cl could invoke it to verify that it is safe to call to the upload script.

An alternate solution could be to pass an option into upload that forced a
specific VCS.

Let me know what you think.

-Albert

[GvR, 2009-05-23 02:34:50]

This looks fine, except I would like to see the two functions renamed: there
should be a GuessVCS(options) that should behave just as before, and it should
call GuessVCSName() which is the guessing functionality.  This would avoid
breaking other people's wrapper scripts that might be calling GuessVCS(options).

[Andi Albrecht, 2009-05-23 05:14:16]

I agree to Guidos comment about renaming the functions to not break existing
wrapper scripts. Just two nits in the comments below.

http://codereview.appspot.com/68042/diff/1/2
File upload.py (right):

http://codereview.appspot.com/68042/diff/1/2#newcode1227
Line 1227: A string indiciating the VCS, or "Unknown" if the VCS cannot be
determined.
The actual return value differs from the documentation.

http://codereview.appspot.com/68042/diff/1/2#newcode1251
Line 1251: return (VCS_GIT, out.strip())
Why is out.strip() returned here? It's not needed to instantiate the git
implementation and I don't think that it is very useful for wrapper scripts as
it's just the string "true"... :)

[ajwong, 2009-05-23 05:44:33]

I shuffled the names back.

As for the out.strip() for the git return, I just figured that if we had the
theoretical output, we might as well return it.  However, since there's no
usecase, I've switched it back to None.

[GvR, 2009-05-23 16:11:59]

http://codereview.appspot.com/68042/diff/1002/1003
File upload.py (right):

http://codereview.appspot.com/68042/diff/1002/1003#newcode1227
Line 1227: One of VCS_GIT, VCS_MERCURIAL, VCS_SUBVERSION, or VCS_UNKNOWN.
This still doesn't match the actual return value -- it returns a tuple.

[ajwong, 2009-05-23 16:58:22]

http://codereview.appspot.com/68042/diff/1002/1003
File upload.py (right):

http://codereview.appspot.com/68042/diff/1002/1003#newcode1227
Line 1227: One of VCS_GIT, VCS_MERCURIAL, VCS_SUBVERSION, or VCS_UNKNOWN.
On 2009/05/23 16:11:59, GvR wrote:
> This still doesn't match the actual return value -- it returns a tuple.

Erp.  Now the comment makes sense.  Uploaded new version.

[ajwong, 2009-05-28 07:01:10]

ping

[ajwong, 2009-06-02 21:14:56]

Are there any more changes that I should make?

Thanks,
Albert

[Andi Albrecht, 2009-06-04 04:02:05]

On 2009/06/02 21:14:56, ajwong wrote:
> Are there any more changes that I should make?
> 
> Thanks,
> Albert

I think it's ready to go in if there are no other concerns.

Sorry for the delay!

[Andi Albrecht, 2009-06-06 14:15:03]

On 2009/06/04 04:02:05, Andi Albrecht wrote:
> On 2009/06/02 21:14:56, ajwong wrote:
> > Are there any more changes that I should make?
> > 
> > Thanks,
> > Albert
> 
> I think it's ready to go in if there are no other concerns.
> 
> Sorry for the delay!

Committed r420. Thanks for the patch, Albert!

Andi