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

fix: improved branch cloning logic #32

Merged
merged 3 commits into from
Nov 5, 2021

Conversation

gautamkrishnar
Copy link
Contributor

improved branch cloning logic. It now checks if the branch exists and uses git cli's --single-branch to improve cloning speed.

@jgehrcke
Copy link
Owner

jgehrcke commented Nov 5, 2021

Lovely! As you've seen in the code comment above your change I actually wanted to do so. Thanks for tackling the special case. This is tested, I assume? :)

@gautamkrishnar
Copy link
Contributor Author

gautamkrishnar commented Nov 5, 2021

Lovely! As you've seen in the code comment above your change I actually wanted to do so. Thanks for tackling the special case. This is tested, I assume? :)

Yep, I am now doing more testing. I Will let you know once I am done.

@gautamkrishnar
Copy link
Contributor Author

@jgehrcke done testing, updated code to match the existing code style you are using.

@jgehrcke
Copy link
Owner

jgehrcke commented Nov 5, 2021

Thank you very much for this contribution, and for the splendid communication! Please come back if you have more ideas :).

@jgehrcke jgehrcke merged commit 05cd4b7 into jgehrcke:main Nov 5, 2021
@gautamkrishnar gautamkrishnar deleted the fix/clone branch November 5, 2021 11:41
flaix added a commit to flaix/github-repo-stats that referenced this pull request Nov 15, 2021
After a rework of the logic that checks if the data branch exists
at the beginning of the script, the return code of `git ls-remote`
is used in a check while `errexit` is active. Which means that the
script will exit and the line checking the return code is never
used.
I might be missing something here, though, because I don't understand
how this ever worked, but PR jgehrcke#32 was declared to be tested and working.
@flaix
Copy link
Contributor

flaix commented Nov 15, 2021

I might be doing something different than you did in your tests, but this code is not working in my install.
I am also not sure how it could work, because from how I read the code, a command is run under active errexit and then the return code of said command is checked. From my understanding this check will never be reached since the script will exit before it.
But again, I might be doing something different since you said you tested it and it worked for you.
I just wanted to let you know that I had to fix it (flaix/github-repo-stats@51c6353) to make it work.

@jgehrcke
Copy link
Owner

@jgehrcke done testing

I should not have relied on only that but also should have reviewed a little more carefully. Anyway, we get that sorted out :) Thanks everyone!

@gautamkrishnar
Copy link
Contributor Author

gautamkrishnar commented Nov 17, 2021

I should not have relied on only that but also should have reviewed a little more carefully. Anyway, we get that sorted out :) Thanks everyone!

Really sorry for that. Yep, I never saw the set -e I only tested the part I had added. Glad to know that its fixed now.

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.

None yet

3 participants