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

Add RepoClient as wrapper that auto-detects and supports both git & hg #6

Merged
merged 5 commits into from
Feb 1, 2024

Conversation

eemeli
Copy link
Member

@eemeli eemeli commented Jan 30, 2024

This builds on top of #5, so at least currently these are the only proper changes here.

A new RepoClient class is added, with a minimal set of methods as required by the rest of the library for repository operations. A Mercurial repo is detected from the presence of a .hg directory, and for Git we look for .git and check that git rev-parse --is-inside-work-tree returns true.

The hglib stuff should work just as before; for git we don't need any wrapper library as the command line is plenty. For the blame I did need to write a minimal parser, but that was only about 10 lines of code.

The Blame().handleFile() function signature changed a bit through all this, but AFAIK that has no external users.

@eemeli eemeli requested review from flodolo and bcolsson January 30, 2024 16:22
@flodolo
Copy link
Collaborator

flodolo commented Jan 30, 2024

I wonder if we could have the "apply black formatting" as a completely separate PRs? It makes reviewing unnecessarily hard, even with separate commits.

I'd be OK with you landing that change directly without review, since that's the standard we use all over the place.

@eemeli
Copy link
Member Author

eemeli commented Jan 30, 2024

I'd be OK with you landing that change directly without review, since that's the standard we use all over the place.

Done, and rebased also this PR.

@eemeli
Copy link
Member Author

eemeli commented Jan 31, 2024

Rebased as #5 was merged. Also exposed git() as a utility function separately from RepoClient + added head() and log() methods to support usage from test_fluent_migrations in m-c.

Copy link
Collaborator

@bcolsson bcolsson left a comment

Choose a reason for hiding this comment

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

Reviewed this and nothing stands out to me so approving. Next time there's a migration ready I'll attempt a test with this.

import hglib


def git(root: str, *args: str) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you considered (and discarded) the idea of using a library similar to hglib for git?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. This way we avoid a layer of indirection in needing to refer to an external definition for what git() does, and have instead this minimal six-line function that's sufficient for our purposes.

@eemeli eemeli merged commit ac017ff into main Feb 1, 2024
5 checks passed
@eemeli eemeli deleted the add-git branch February 1, 2024 08:17
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