Raise an exception if revision is empty in git url

[deveshks]

Fixes and closes #7402

If URL's listed in VCS Support of pip install of the type git://git.example.com/MyProject.git@master#egg=MyProject have a missing revision, e.g. master here, raise an exception.

e.g.

$ pip install -e git+https://github.com/python/mypy.git@#egg=mypy
Obtaining mypy from git+https://github.com/python/mypy.git@#egg=mypy
ERROR: The URL 'git+https://github.com/python/mypy.git@#egg=mypy' has an empty revision (after @) which is not supported. Include a revision after @ or remove @ from the URL.

[uranusjr]

I’m wondering whether it would be good enough to fallback to the default (i.e. remove the @ automatically).

[deveshks]

Hi @uranusjr

I personally think it's better being explicit to the user about the problem in the URL, rather than taking decisions on his behalf, and using for e.g. git://git.example.com/MyProject.git#egg=MyProject.

If we want to do that, we should ask the user about it beforehand, or during installation perhaps.

But if that is what would serve a user better, I can update my PR to do the same :)

[uranusjr]

I am honestly not sure which is better. I would definitely agree explicitness is better if we didn’t already allow a ref-less URL, but since there is already a default, I feel it is equally good to use that default or not. Let’s see if other maintainers have more useful opinions 😅

[deveshks]

Agreed @uranusjr ,

I will wait for other maintainers to provide their opinions, and then make changes accordingly, but assuming if we go with the explicit approach, does the PR I submitted look good, or are there issues in the code somewhere?

[uranusjr]

No, the patch looks good to me if we want to raise an error in this case.

[deveshks]

Thanks @uranusjr.

[uranusjr]

One additional though: We can probably raise an InstallationError or something instead to avoid the traceback, since some users tend to submit such output as bugs and cause unnecessary triaging overhead. But this should be discussed after we decide this is the way to go.

[sbidoul]

For the sake of keeping the pip code base slightly simpler, falling back to rev=None when rev is empty could work too.
We could then enrich one of the existing parametrized tests for get_url_rev_and_auth to cover this case.

I have no strong opinion on this though.

[deveshks]

Hi @sbidoul

In that case, we can always add a logger.warn with the message that we are defaulting to a url after removing the @ automatically.

I think that will inform the user to take a decision to fix his url if he wants to, while also not failing immediately and letting the installation fail.

[chrahunt]

I think that we should throw an explicit exception here for two reasons:

1. The issue pip understand @# as empty tag/href in git source and breaks installation #7402 is related to a URL that only pip defines the meaning of - an editable URL in a requirements.txt, but how we handle this will set expectations for how the same would be handled in direct references. PEP 440 doesn't say explicitly one way or another, so it becomes another pip-specific behavior that we're having to document after-the-fact.
2. The current behavior is to throw an exception, so the ecosystem around requirements.txt doesn't have to support this (since pip's behavior is the de-facto standard). Changing the behavior instead of throwing an exception creates an additional requirement on those tools.

That doesn't mean that we shouldn't change the behavior, but if someone feels strongly about it then I think it should be a separate issue and this PR (and the issue it addresses) can just be about making an error scenario more user-friendly.

[deveshks]

Thanks folks. So seems like @uranusjr and @sbidoul are on the fence for this, while @chrahunt and even I think that we should let it remain an error, and if needed, create another PR/issue for discussion around this to make it more user friendly.

Given this is the scenario, I think I will go ahead and keep this PR to report an error, but probably raise an InstallationError instead as per #7927 (comment)

Should I make the above change and then close out this PR to be merged?

[pradyunsg]

Should I make the above change and then close out this PR to be merged?

@deveshks Yes. However, it's possible that we end up deciding that we don't want to error out, so there's a possibility that the PR doesn't get merged.

[deveshks]

Hi @pradyunsg

Yes sure, also @chrahunt did mention that if we want to include a warning instead of erroring out, there should be a separate issue and this PR (and the issue it addresses) can just be about making an error scenario more user-friendly.

[sbidoul]

LGTM

[deveshks]

Thanks @sbidoul , Can I get it merged as well :)

[pradyunsg]

LGTM, other than the NEWS file entry.

And, I tend toward a "be strict and loosen if users would need that" approach, so I'm fine with being strict here and just plain erroring out.

[deveshks]

Hi @pradyunsg , seems like the new commit caused some checks to error out: https://github.com/pypa/pip/actions/runs/67463960