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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ObjectDatabase.ShortenObjectId() #677

Merged
merged 2 commits into from Jun 7, 2014
Merged

Conversation

nulltoken
Copy link
Member

Libgit2 exposes git_object_short_id() method. Let's make this available to LibGit2Sharp.

Proposed signature would be

string ObjectDatabase.ShortenObjectId(GitObject object)

Of course, if anyone comes up with a better name for this method, we'll be very grateful 馃槈

@Aimeast
Copy link
Contributor

Aimeast commented Apr 14, 2014

  • extension method, GitObject.ShortenId()
  • property, GitObject.ShortenId as lazy loading

@dahlbyk
Copy link
Member

dahlbyk commented Apr 14, 2014

馃憤 for either extension method or lazy property, perhaps just ShortId?

@nulltoken
Copy link
Member Author

Actually I'm unsure about this. Below my rationale.

  • The shortened id doesn't actually "belongs" to the GitObject. It's a short representation that depends on the whole content of the odb.
  • The shortened id could actually change during the lifespan of a GitObject. (for instance, see those tests in libgit2). This means that a shortened Id may be valid for a certain amount of time, then suddenly become ambiguous once some other objects bearing similar prefixed id have been added to the odb.
  • The way the shortened id is calculated may be quite time consuming (see libgit2/libgit2@13f7ecd). Making it easy to bound from a ViewModel may not be that wise.

@dahlbyk
Copy link
Member

dahlbyk commented Apr 14, 2014

In my view, that it's dependent on the entire object database is a bit of an implementation detail. It's natural to ask a GitObject what its short representation is; no reason to fight it.

How about a real method (virtual rather than extension, for testability) on GitObject called CalculatedShortId() or similar?

@nulltoken
Copy link
Member Author

How about a real method (virtual rather than extension, for testability) on GitObject called CalculatedShortId() or similar?

@carlosmn @ethomson @jamill Thoughts?

@carlosmn
Copy link
Member

I'd have to contend the assertion that the dependance on the odb is an implementation detail. The only reason you're asking the object/library instead of taking an arbitrary prefix is this dependence.

As this is something that needs to be calculated, rather than accessed, I don't think we should have something that looks like it's accessing underlying data.

Something like CalculateShortId() would be fine, but CalculatedShortId() sounds to me too much like "grab this one I have from earlier" which is not what we're doing.

Maybe something like UniquePrefix() with a minimum length? And document that this is for human consumption and not guaranteed to be unique five minutes from now, but there's a good chance if you add a couple extra letters.

@jamill
Copy link
Member

jamill commented Apr 15, 2014

Given that all the other properties on these objects are immutable, it seems a bit weird to have a method on the object that returns a calculated value that can change. Having a minimum prefix length might be nice for consumers who want to show at least N characters as long as the prefix is unique.

Here would be my suggestions based on the suggestions above:

public static string ShortId(this GitObject object, int minLength)

public static string UniquePrefix(this GitObject object, int minLength)

@nulltoken
Copy link
Member Author

I took some time... And I'm still not convinced about making GitObject expose this behavior.

I'd rather go with

string ObjectDatabase.ShortenObjectId(GitObject object, int? minLength)

This wouldn't prevent the consumer from creating an extension method by himself.

nulltoken added a commit that referenced this pull request May 18, 2014
@nulltoken
Copy link
Member Author

I've started to work on this.

@nulltoken
Copy link
Member Author

Travis looks happy. @jamill are you?

@phkelley Could you please take a peek at the OdbBackend change?

@@ -619,11 +664,15 @@ protected enum OdbBackendOperations
/// This OdbBackend declares that it supports the Exists method.
/// </summary>
Exists = 64,
/// <summary>
Copy link
Member

Choose a reason for hiding this comment

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

You could add a blank line here to be consistent.

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!

@jamill
Copy link
Member

jamill commented May 19, 2014

馃憤

@Therzok
Copy link
Member

Therzok commented Jun 1, 2014

bump.

@phkelley
Copy link
Member

phkelley commented Jun 7, 2014

Sorry for the delay, @nulltoken. I have read this over (but not personally run it) and it looks completely correct to me. I do not see any defects.

@nulltoken nulltoken added this to the v0.19.0 milestone Jun 7, 2014
@nulltoken nulltoken merged commit e24b4d0 into vNext Jun 7, 2014
@nulltoken nulltoken deleted the ntk/shortenization branch June 7, 2014 11:51
@nulltoken
Copy link
Member Author

@phkelley 鉂わ笍 for the feedback

@nulltoken nulltoken added Bug and removed Bug labels Aug 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants