pip install -e inconsistent checkout with git url and branch

[hpointu]

Hi,

The way pip install -e works is inconsistent when you specify the branch name in a git url.

Look at the following behaviours :

Specifying a branch, example master on pip source repository:

$ pip install -e git+https://github.com/pypa/pip.git@master#egg=pip
$ cd src/pip/
$ git status
HEAD detached at 08ebc0a
nothing to commit, working directory clean

Specifying a branch again, develop from the same repo this time :

$ pip install -e git+https://github.com/pypa/pip.git@develop#egg=pip
$ cd src/pip/
$ git status
On branch develop
Your branch is up-to-date with 'origin/develop'.
nothing to commit, working directory clean

I would not expect to have 2 different states for the same command, depending on the branch I specified is the default one or not. Because I specified it in both cases, this should behave in the same way.
Any further work with this editable installation becomes dependant on whether I am in the first case, or the second one.

[xavfernandez]

I cannot reproduce this behavior with latest develop/master branches. Could you confirm ?

[hpointu]

I am still reproducing it with the latest develop version.
Detached HEAD vs. branch depending on default branch specified or not.

Please note there was a typo in my issue, I forgot the git+ in front of the first URL.

[xavfernandez]

Ok, I reproduce the behavior.
You have to do it in two different venv since the first install command will decide how pip behaves.

If you perform:

pip install -e git+https://github.com/pypa/pip.git@master#egg=pip
pip install -e git+https://github.com/pypa/pip.git@develop#egg=pip

you'll be in detached HEAD after both commands.

If you perform:

pip install -e git+https://github.com/pypa/pip.git@develop#egg=pip
pip install -e git+https://github.com/pypa/pip.git@master#egg=pip

you'll be on branch develop after both commands.

pip however always checkout the correct commit (via a branch or a detached HEAD), and you still have access to origin/develop and origin/master.

[hpointu]

I guess I chose a not very clear example by trying on pip repo.

Try by performing these steps on any other python package repository you have access to :

• in a venv, pip install -e <repo> by specifying the default branch
• in an other venv, same command but by specifying a non default branch which is at a different commit than the first.

It appears that you don't end up with the same states in the two different checkouts.

I appreciate that using the pip repository as an example of repository was not a good idea as it's mixing several things at once.

[xavfernandez]

Well the remaining questions are:

• is this inconsistency an issue ?
• what should the consistent solution be ?
  since we can specify a commit, maybe 'detached HEAD' ?

[hpointu]

• I consider it's an issue : having inconsistent states relying on the git repository configuration (default branch) prevents automatic work to be applied on the editable installation in a consistent way. This is easy to address in a manual scenario, but for automatic workflows this becomes a pain.
• I am not sure I am able to decide what should be the consistent behaviour. As a naive approach, I would say detached HEAD for the same reason as you : consistency between commits and branches. But then it becomes not natural regarding to the git behaviour : a checkout of branch name doesn't lead to a detached HEAD in git. But a checkout on a commit does. So reproducing the git behaviour of "checkout" might be the right answer. Personally, I am open to both, as long as it is consistent.

[xavfernandez]

Finally I ended up choosing detached HEAD since the update from one branch to an other is currently performed via a reset --hard.

[pradyunsg]

Just noting that there's an existing PR for this: #3749.

If someone picks this up, you can use that as a reference of how to go about trying to resolve this. :)

[cjerdonek]

I've been doing a lot of work and studying around this area of the code base recently and see a number of things that can be simplified. I really don't think installing a branch should result in a detached HEAD.

Naming a branch to install should result in the branch being checked out, and naming a commit should result in a detached HEAD. That seems the most natural, and that's also the way Git behaves.

[hpointu]

I kind of agree with @cjerdonek in reproducing git behaviour. It seems more natural. I think that's what anyone would expect.

[cjerdonek]

FWIW, I'm planning to work on this eventually, but I want to do some refactoring first. There's a bunch of code in this area that can be simplified, and this issue will fall out of that.

[pradyunsg]

I hadn't really put in a lot of thought into that comment. What @cjerdonek says makes sense to me too.

I'm planning to work on this eventually

🎉

[pradyunsg]

This is still as good first contribution issue if someone comes around - correct?

[cjerdonek]

No, I wouldn't consider this an easy fix or good for first-time contributors. One reason is that there are actually three distinct code paths that need to be looked at and possibly modified: the initial install, updating, and what the code calls "switch." Also (and I'm not in front of a computer right now to double-check), but I'm pretty sure the "rev_options" being passed around aren't passing the right data for this to be fixed only with "local" changes to the code (e.g. just by changing the arguments to the Git invocation). So this likely involves touching more parts of the code. This is why I'm approaching this by doing some refactoring first -- to make it easier for reviewers to see what is going on and to simplify / DRY some things up, etc.

…

On Mon, Aug 21, 2017 at 11:42 AM Pradyun Gedam ***@***.***> wrote: This is still as good first contribution issue if someone comes around - correct? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2037 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVt7pw8wZimcw9mH_t8DX-cgUc0G4ZUks5sac-egaJpZM4CiC0Z> .

[cjerdonek]

Hi, if people don't mind I'm going to assign myself to this issue as it's something I've had my eye on for a while and had put a fair amount of thought into. Also, the refactoring I had been wanting to do before working on this issue (and that I mentioned above) is now pretty much done. (Thank you, @pradyunsg and @xavfernandez for the reviews!)

The main prerequisite to this issue is PR #5623, which I'm still waiting for a review on. The new resolve_revision() method that PR adds will be very useful in addressing this issue.

For this issue, I was planning to address just the fetch_new() (aka "clone") case as it's simpler than the update() case and can be addressed separately. The update() case can be addressed in a later PR.

[cjerdonek]

I just posted a fix (PR #5723) for the fetch_new() case of this issue, as described in my previous comment.

[deveshks]

I think this issue can be closed, since #5723 has already addressed this issue.

[uranusjr]

Thanks @deveshks!