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

Replace raw git bash commands with Gitpython #4

Merged
merged 8 commits into from
Mar 20, 2019
Merged

Replace raw git bash commands with Gitpython #4

merged 8 commits into from
Mar 20, 2019

Conversation

joeleba
Copy link
Member

@joeleba joeleba commented Mar 19, 2019

Why?
We've been calling "raw" git commands with subprocess. This has a few disadvantages:

  • Requires the internal navigation to various directories with os.chdir. This is prone to obscure bugs e.g. if one forgets to change back to the correct directory.
  • Extracting the latest commit of a repository is troublesome.
    We have been using the string "latest" to represent this. It's fine in the context of running the script in the command line and immediately consume the result, but on BigQuery it's wrong since "latest" at different points in time mean different commits.

GitPython addresses these issues.

Changes:

  • Use GitPython to perform git interactions now.
  • Replacing "latest" by getting the actual latest commit's SHA.

@joeleba joeleba requested a review from meisterT March 19, 2019 13:58
@ulfjack
Copy link

ulfjack commented Mar 19, 2019

It would be good to have some text in the description on why you're making this change. Is gitpython any better than vanilla git?

@joeleba
Copy link
Member Author

joeleba commented Mar 19, 2019

@ulfjack Thanks for your comment. Updated the PR description.

Copy link
Member

@meisterT meisterT left a comment

Choose a reason for hiding this comment

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

Please don't merge the PR as but squash the commits in a way that each individual commit makes sense, e.g. no "fix commits".

benchmark.py Outdated
def _get_commits(commits_list, repo, flag_name):
"""Returns a list of commits.

If the input commits_list is empty, fetch the latest commit from repo.
Copy link
Member

Choose a reason for hiding this comment

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

really the latest commit or the latest in master?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.
As discussed offline, I always use "Squash and merge" for PRs in this repository. Disabled the "Merge commits" option in the repository settings to be sure.

@ulfjack
Copy link

ulfjack commented Mar 20, 2019

For the first point mentioned in the description: it would seem odd if there wasn't an api to set the working directory for a subprocess without calling chdir.

@joeleba
Copy link
Member Author

joeleba commented Mar 20, 2019

@ulfjack You're right, there's an option to do that. With that in mind, reason no. 2 would be the main motivation for this change.

@joeleba joeleba merged commit 2a43b12 into master Mar 20, 2019
@joeleba joeleba deleted the pygithub branch March 20, 2019 11:37
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.

3 participants