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

Resolving "refish" #922

Closed
rcoup opened this issue Jun 27, 2019 · 7 comments · Fixed by #923
Closed

Resolving "refish" #922

rcoup opened this issue Jun 27, 2019 · 7 comments · Fixed by #923

Comments

@rcoup
Copy link
Contributor

rcoup commented Jun 27, 2019

Trying to do a pygit2 equivalent of git checkout's refish resolving, and found libgit2's checkout example:

  1. It initially calls resolve_refish() which calls git_reference_dwim() which does a whole set of ref lookups (refs, heads, tags, branches, remotes, etc). That isn't available in pygit2 AFAICS. Returns an annotated commit, which seems to be a commit + the ref it was resolved from?

  2. then gets the actual commit, which is straightforward enough

  3. then does the checkout

  4. and finally updates HEAD using the annotated commit ref to decide whether the head is detached or not. This seems to be covered (a bit unintuitively) in pygit2 by passing a string vs oid to repo.set_head()

So...

Q1: I wonder whether we should be preferring/helping get annotated commits when we're looking up references so the user can get back to their ref easily. We could be more aggressive about this than libgit2? eg: make AnnotatedCommit a subclass of Commit, and automatically use the underlying git_commit for APIs that need it.

Q2: Would adding support for git_reference_dwim() be a good idea? That returns references, so at least you could maintain a (reference, commit) pair in Python and do

Q3: Would adding a Repository.resolve_refish() method (similarly to revparse_single()) be useful? I think revparse_single() resolves all "commitish" properly?

@rcoup
Copy link
Contributor Author

rcoup commented Jun 27, 2019

hmm, git_revparse_single() does call git_reference_dwim(), maybe only in specific cases.

👷 digs

@rcoup
Copy link
Contributor Author

rcoup commented Jun 27, 2019

Yeah, definitely doesn't work:

via DWIM:

reference = repo.lookup_reference_dwim('master')  # lookup_reference() raises InvalidSpecError
assert reference.name == 'refs/heads/master'

reference = repo.lookup_reference_dwim('origin/master')  # lookup_reference() raises InvalidSpecError
assert reference.name == 'refs/remotes/origin/master'

reference = repo.lookup_reference_dwim('version1')  # lookup_reference() raises InvalidSpecError
assert reference.name == 'refs/tags/version1'

API implementation options:

  1. Repository.lookup_reference_dwim('version1')
  2. Repository.lookup_refish('version1')
  3. Repository.lookup_reference('version1', strict=False) (strict=True is the default)
  4. Repository.lookup_reference('version1', dwim=True) (dwim=False is the default)
  5. ???

I think my current preferences are 2, 3, 1.

@rcoup
Copy link
Contributor Author

rcoup commented Jun 27, 2019

hmm, though naming it lookup_refish() should also add the fallback revparse_single() behaviour

@rcoup
Copy link
Contributor Author

rcoup commented Jun 27, 2019

Quick first-cut implementation of (1) with a separate lookup_refish(): rcoup@37b4017

rcoup added a commit to koordinates/kart that referenced this issue Jun 27, 2019
@jdavid
Copy link
Member

jdavid commented Jun 30, 2019

At first I was confused about the word refish, I was reading it as re-fish and not ref-ish, and wondering what a 🐟 had to do with Git 🤔 ...

Well, looks good, except for the naming.

The different lookup_xxx functions in pygit2 return a xxx object, for instance lookup_worktree returns a Worktree object, lookup_branch returns a Branch, and so on. But lookup_refish doesn't return a Refish. If it returned an AnnotatedCommit object then we may call it lookup_annotated_commit. But it returns a tuple of a reference and a commit, I prefer resolve_refish as you first proposed.

After some thought I do like the other one, lookup_reference_dwim, because it returns a reference as its name suggest, and do-what-i-mean (I had to look for dwim).

So, your commit answers both Q2 and Q3, and it looks like it's enough to implement checkout, but if you wish to implement annotated commits then go ahead 😉 Whether to provide a higher level api and be more aggressive than libgit2 (Q1), that requires more thought..

To summarise:

  1. lookup_reference_dwim and resolve_refish
  2. AnnotadedCommit
  3. Higher level API

Maybe we can do these in different PRs so they don't get too big?

@rcoup
Copy link
Contributor Author

rcoup commented Jul 1, 2019

After some thought I do like the other one, lookup_reference_dwim, because it returns a reference as its name suggest, and do-what-i-mean (I had to look for dwim).

"commitish" and "refish" are terms that appear in the git documentation, but they're not canonically defined anywhere as far as I can see 😄 I had to look up "dwim" too, wasn't particularly intuitive. But follows libgit2, and I'm all for computers doing what I mean.

lookup_refish() -> resolve_refish() makes sense 👍 I'll make those changes.

I'll change the names and then see what AnnotatedCommit and/or a higher-level API might look like. Though I'm heading on paternity leave for a few weeks and chances of me getting to it before I disappear are virtually zero.

@jdavid
Copy link
Member

jdavid commented Jul 3, 2019

Merged, thanks!

And congratulations for the 👶 !!

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 a pull request may close this issue.

2 participants