Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check commits even with shallow clone of tags #15023

Merged
merged 4 commits into from Nov 5, 2023

Conversation

jsallay
Copy link
Contributor

@jsallay jsallay commented Oct 28, 2023

Changelog: Fix: Make Git() check commits in remote server even for shallow clones.
Docs: omit

Fixes #14928

When performing a shallow clone of a tag (e.g. git clone https://github.com/conan-io/conan --depth=1 -b 2.0.13) it does not copy and of the branches or history. The commit_in_remote method of the Git class only checks local information. This PR will additionally check the remote if needed.

I tested this by cloning a repo twice - one as a normal clone and the other with a shallow clone of a tag. I created an additional commit in each repo. I ran of total of 8 tests - checking all possibilities of:

  1. Shallow vs Full clone
  2. Commit in repo
  3. Network connection available.

The git fetch performed requires a connection to the remote repo. I wanted to make sure that it failed gracefully in an offline scenario. The function performed as expected in all cases.

Without an internet connection, the function waits until the command times out and then throws an error (which is communicated to the user).

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Signed-off-by: John Sallay <jasallay@gmail.com>
@CLAassistant
Copy link

CLAassistant commented Oct 28, 2023

CLA assistant check
All committers have signed the CLA.

@memsharded memsharded self-assigned this Oct 29, 2023
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution @jsallay !

Some CI tests are failing, the new check is producing unexpected errors, it probably needs to be more conservative and raise only on very evident cases.

return True
except Exception as e:
# The error message contains the command and the commit if it can't find it.
if str(e).count(commit) == 2:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this check is not capturing all scenarios.

Signed-off-by: John Sallay <jasallay@gmail.com>
@memsharded memsharded added this to the 2.0.14 milestone Nov 2, 2023
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, and tests are passing, that is great.

It would be great to add a test that validates the new code is working fine. Do you think you could try to add it? Let us know if you need help or guidance.

@jsallay
Copy link
Contributor Author

jsallay commented Nov 3, 2023

I've added in tests for a case where it should find the commit and one where it should not. Let me know if you have any other suggestions.

@memsharded
Copy link
Member

I've added in tests for a case where it should find the commit and one where it should not. Let me know if you have any other suggestions.

It is great. I'll merge when the PR is green

@memsharded memsharded merged commit fc8640d into conan-io:release/2.0 Nov 5, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Find Commit in Remote on Shallow Clone
3 participants