Don't assume master branch exists when reinstalling editable package from Git

[di]

Fixes #4448, fixes #647.

[cjerdonek]

Something seems suspicious about what's being proposed because --force-reinstall doesn't actually appear anywhere in the test case.

[cjerdonek]

Are you fixing a different, but related issue?

[cjerdonek]

If this is a bug in vcs/git.py, it seems like it'd be worth adding a lower-level / finer-grained test inside test_install_vcs_git.py instead of only checking via a high-level end-to-end test. A possible example to compare to is test_obtain_should_recognize_auth_info_url().

[di]

@cjerdonek I'm not sure I agree. I think this is perhaps close to what you're suggesting:

@patch('pip.vcs.call_subprocess')
@patch('pip.vcs.os.path.exists', lambda _: True)
@pytest.mark.network
def test_obtain_reset(call_subprocess_mock, script):
    git = Git('git+file:///somewhere')
    git.compare_urls = lambda *args: True
    git.check_version = lambda *args: False
    git.obtain(script.scratch_path / 'test')

    assert call_subprocess_mock.call_args_list[4][0][0] == [
        'git', 'reset', '--hard', '-q', 'refs/remotes/origin/HEAD'
    ]

This test is mostly just checking that rev_options has changed, and doesn't really motivate the fix I'm proposing. Further more, it feels pretty fragile -- if we're not actually installing a package in the test, we need to do some monkeypatching to make the Git instance think the package is already installed so that it attempts to update it, and then there's a slew of sequential subprocess calls, only one of which we really care about.

I understand that such high-level tests are slow and expensive, but I think in this case it does make sense, although I'm open to further suggestions -- I'm relatively new to contributing to pip, so I'm not totally familiar with how we test it.

[cjerdonek]

It seems to me there's value in doing both (which is why I said "instead of only"): you can have the high-level test to check that the issue was fixed as reported. And you can have the lower-level, more focused test to check the function that you changed.

As far as making the lower-level test more sensible and less fragile, you'd probably know better than me what's appropriate. It seems like there'd be something worth checking about that function's behavior that makes sense. Instead of mocking the subprocess call, is there something else you can do to trigger the necessary pre-condition (e.g. call a different method on the Git class)?

Another suggestion: would it make sense to do a real subprocess call to check the state of the repo after the method call, instead of using mocks?

[cjerdonek]

One more thing: my impression from digging into pip's code base and following some of the issues and conversation is that while most of the testing in the past has been focused on the end-to-end tests, there is some movement and desire to move towards finer-grained tests (one of the reasons being the slowness you mentioned).

So I think there's value in thinking about how we can better leverage finer-grained tests, even if the patterns aren't yet widely established.

[di]

@cjerdonek Contrary to the issue, the --force-reinstall flag is not actually necessary to reproduce this. It occurs whenever one is attempting to reinstall an editable package.

[cjerdonek]

Okay, but the test is called test_force_reinstalling_works...() and links to the issue which is about using the --force-reinstall flag. If the flag is a red herring, you might want to say that somewhere.

There's also the issue that --force-reinstall can trigger special logic / a different code path, so there can still be value in explicitly testing that an issue was fixed as originally seen and reported.

[di]

@cjerdonek Good catch, I will update the test name.

I added a comment to #4448 indicating that the --force-reinstall flag is a red herring.

[cjerdonek]

Since first commenting on this PR, I've become a lot more familiar with this area of the code. I'd like to make the following suggestion.

Rather than setting rev_options to ['refs/remotes/origin/HEAD'] early on inside obtain(), I'd like to suggest setting it instead to [] and then changing update() here to something like:

if rev_options:
    rev_options = self.check_rev_options(
        rev_options[0], dest, rev_options,
    )
else:
    rev_options = ['refs/remotes/origin/HEAD']
self.run_command(['reset', '--hard', '-q'] + rev_options, cwd=dest)

There are a few reasons for this. One is consistency. The other VCS classes set rev_options to the empty list if no rev is specified in the URL. Also, inside obtain() in the "fresh install" case, check_rev_options() is called only if an explicit rev is specified:

if rev:
    rev_options = self.check_rev_options(rev, dest, rev_options)

But in update(), since rev isn't passed (only rev_options), update() is using rev_options as a proxy for an explicit revision:

if rev_options:
    rev_options = self.check_rev_options(
        rev_options[0], dest, rev_options,
    )

One consequence of setting rev_options to a non-empty list is that users would start seeing the following warning with this PR:

Could not find a tag or branch 'refs/remotes/origin/HEAD', assuming commit or ref

It seems like this warning should be displayed only if a revision is explicitly provided in the URL, as in the fresh-install case.

Another way of modifying this PR would be to change update()'s signature so that rev is also passed, and then change update() to look like the following, similar to how it looks in obtain():

if rev:
    rev_options = self.check_rev_options(
        rev_options[0], dest, rev_options,
    )

[cjerdonek]

PS - it looks like this PR will also resolve issue #647 (I believe #4448 is a duplicate of #647).

[pradyunsg]

Thanks @cjerdonek. It is indeed. I was looking for #647 since I remembered seeing it but wasn't sure where it is because there's like ~300 open issues.

@di could you update the description so that it closes that issue?

[di]

@pradyunsg Done.

@cjerdonek Thanks for the review. I agree with your suggestion, I will try to make that change shortly.

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

[di]

I rebased this post-#4700.

@cjerdonek I took a shot at doing what you're proposing here, but I think you missed that check_version is currently assuming that rev_options is always a non-empty list:

def check_version(self, dest, rev_options):
    return self.get_revision(dest).startswith(rev_options[0])

I'm hesitant to do the refactoring necessary to make that work (although I do agree that it could be a good idea) simply because it isn't necessary to solve the problem this PR is trying to fix. I think it's a good candidate for one of your future refactoring PRs though.

One consequence of setting rev_options to a non-empty list is that users would start seeing the following warning with this PR:

Thanks for pointing this out -- I realized that this would actually happen with this PR as-is, since check_rev_options uses get_short_refs, which strips the ref/remotes from the full ref.

We don't actually have to use the full ref to solve this issue though, so I updated the test to not expect anything printed to stderr, and updated the default ref to just be origin/HEAD.

Hopefully that makes this PR a little easier to merge 😉

[cjerdonek]

Thanks for giving it some thought and for trying it out, @di. Using origin/HEAD seems like a good alternative approach (and I believe it will also fit with PR #4690 simply by changing origin/HEAD to HEAD). And yes, this will be in the pathway of some of the refactorings I'm working on / have in mind, though perhaps not the first ones.

[pradyunsg]

@di -- this PR is ready; right?

[di]

@pradyunsg Yep!

[xavfernandez]

Thanks for the PR 👍

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.