Version not working for git repositories cloned with truncated URL

[ceball]

See the 'version' parts of ioam/topographica#611. I guess the difference is that I cloned via https:

By the way, I just followed the README's clone instructions (except for using https) and git describe
gives me CNV-2014-701-g1c6dc94.

But version's looking for '.git' in the repository name (https://github.com/ioam/param/blob/master/param/version.py#L195).

[jlstevens]

I don't think it is that - I clone using https all the time. To check, I ran this in my \tmp folder...

git clone https://github.com/ioam/topographica.git
cd topographica
git submodule update --init
./topographica --version
0.9.8-1344-gc4bc3ff

The .git directory should be present in any git repository, no matter how it is cloned. Can you make sure you have such a directory? It may be hidden.

Can you remove the try/except statements in git_fetch and print the value of output. Then you should see the real error.

[ceball]

It's not about the .git directory, I meant it's about the remote's name: mine doesn't end in .git. Apparently git clone https://github.com/ioam/topographica works just as well as git clone https://github.com/ioam/topographica.git (not sure when I stopped typing the .git!).

[ceball]

I.e.

$ git remote -v
origin  https://github.com/ioam/topographica (fetch)
origin  https://github.com/ioam/topographica (push)

[ceball]

I should just start typing the .git again, right?

Or do you think it's not really necessary to check that the remote's name ends with .git? (Why do you have to verify it's the correct repository at all, by the way?)

[jlstevens]

Ah, got you.

I remember talking to Marco about this - if I recall, we thought checking for the .git suffix was safer.

Both GitHub (the 'HTTPS clone URL' box on the right) and our README include the .git suffix so things would have worked if you had followed the README. ;-p

So yes, please include the .git suffix. It is necessary to check for the repository because some people may use a PyPI install of topographica (not a topographica git repository) in some other git repository. This was an issue explicitly raised by someone that we then fixed.

[jlstevens]

Looking at the code, we could relax this restriction but I'm not sure if we should.

A repository (e.g a fork) called topographica-legacy should not report the version number as if it is topographica. It is a different repository so reponame should be set accordingly (to 'topographica-legacy'. Adding the .git suffix ensures only a repository called 'topographica' (nothing else) matches.

I am happy to change this if you think a lot of people will bump into this issue, either because they don't follow the README or don't copy the GitHub supplied URL properly. The only common case I can think of are developers who are so experienced with git, that they type out the URL manually and rely on the .git extension being optional.

[jlstevens]

I'll leave this issue open until we decide whether we should be more permissive given that truncated clone URLs do seem to work with git.

I've marked this issue as resolved:invalid as the code was working as expected using the official installation instructions.

[jlstevens]

Thinking about it, there are only two valid cases - the remote URL ending in .git or ending in reponame with nothing else.

The issue is that for simplicity, the code checks for a substring anywhere in the output returned from the command. Given that the output looks like:

git remote -v
origin  https://github.com/ioam/topographica (fetch)
origin  https://github.com/ioam/topographica (push)

I guess adding a space after reponame would work, assuming the (fetch) and (push) labels are always there. Then something like this could work:

valid_matches = ['/' + self.reponame + '.git' , '/' + self.reponame + ' ']
if not any(m in output for m in valid_matches):
   return self

I am happy to commit this. The only issue is that it makes another assumption of what git output looks like. This should be ok as git's interface is stable.

Maybe you can try out this suggestion and tell me if it works?

EDIT: One issue with this code is that there would be match if topographica is a remote that isn't origin. For instance, the repository topographica-legacy could have topographica as a remote called upstream. This shouldn't be an issue: if topographica is in the remotes, Version can assume that the correct tag format will be used and that any derived code will follow the same conventions as topographica master.

[jbednar]

I am happy with the proposed new code, and I think it's an improvement. In any case, fixing the current problem does seem important, as this was a very mysterious problem.

[jlstevens]

This should now be fixed in commit 24f7ca6

I have also updated the param submodule reference in topographica. I can update the param submodule reference in all the other projects if you wish...

[jbednar]

Thanks!

[ceball]

Maybe you can try out this suggestion and tell me if it works?

Yes, it works for me. Thanks for doing that. (I'll remove the 'invalid' label.)