Closed larsks closed 6 years ago
I'm not sure exactly why that code was added as a comment instead of patched in outright, I think @derimagia would be able to answer that?
I do think this is something that should be brought up to the team. :+1:
@4U6U57 I added a unit test; let me know if that's sufficient.
Thanks for the patch! Been meaning to switch this over - I wasn't 100% on git versions it was available. We may need to figure this out in a This looks good to me 👍
If you were really concerned about older git versions, you could do something terrible like this...
# check git version
git_version=($(git version | cut -f3 -d' ' | tr '.' ' '))
if [ "${git_version[0]}" -lt 2 -o "${git_version[1]}" -lt 12 ]; then
giturl=$(git config --get "remote.${remote}.url")
else
giturl=$(git ls-remote --get-url "$remote")
fi
Instead of checking for the version number, why not check the git help
command for the presence of the --get-url
parameter?
git help ls-remote | grep '\-\-get-url' | wc -l
Sure, that works.
@paulirish Ideas on how you wanted to deal with versions of git? Another option is just to fall back to the other way in an or statement.
Actually may be a non-issue, looks like it was documented here https://github.com/git/git/commit/2303cad242a6ccc4745d7994ecc0425bd8513a0d and actually added here https://github.com/git/git/commit/45781adb9a89c0c47f61ccf8062be26b86a38a54 - that's long enough ago that I don't think we care - thought it was added more recent than that
wow. nice digging!
Yeah I just tried --get-url
on an september 2015-era release of git 2.3.7 and it worked great. So yeah 👍 to not worrying about compat on this one.
As noted in a comment, 'git ls-url --get-url' will expand insteadOf mappings. I use a number of shortcuts (like gh: for git on github, lp: for git on launchpad, me: for my github repositories, etc), so I was constantly hitting situations in which git-open would fail to do the right thing.
It looks like '--get-url' has been available since git 2.12.0.
This has been bugging me for a while, but the fact that it was listed in a comment in the script and not implemented makes me wonder if there was in fact a reason for that.